Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not log ClassCastException in MavenParser in case of malformed POM #3701

Conversation

api-from-the-ion
Copy link
Contributor

@api-from-the-ion api-from-the-ion commented Nov 17, 2023

What's changed?

Wrote the unit test for the case #3699 and proposed fix

What's your motivation?

Fixes #3699

Anything in particular you'd like reviewers to focus on?

No

Anyone you would like to review specifically?

No

Have you considered any alternatives or workarounds?

I don't think there are any, but I'm glad to hear about them.

Any additional context

After this fix, there is no ClassCastException. But the test asserts fails anyway because the POM is changed here - see the results of the tests. And now I don't know what to do with this. Should we suppress it or ignore it in the code itself? So that test would expect such changes or ignore them? Or is here some other way? Correct the parser, so these changes wouldn't appear and there would be no difference?

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ IDEA auto-formatter on affected files

@api-from-the-ion api-from-the-ion marked this pull request as ready for review November 17, 2023 02:36
@api-from-the-ion api-from-the-ion marked this pull request as draft November 17, 2023 02:45
@timtebeek timtebeek marked this pull request as ready for review November 17, 2023 12:49
@timtebeek timtebeek self-requested a review November 17, 2023 12:57
@timtebeek
Copy link
Contributor

Thanks a lot for this improvement! I've gone ahead and updated your test to use the parser directly, as that's more convenient when asserting ParseErrors. Before your code change we returned a different ParseError from the catch(Throwable), which we entered because of the ClassCastException. As a side effect that also logged the stacktrace you mentioned in

In the new situation we still return a ParseError, but now the one created by the MavenXmlParser which extends XmlParser. That in turn might still log an exception, but hopefully with more details that help pinpoint and resolve the issue.

} catch (Throwable t) {
ctx.getOnError().accept(t);
return ParseError.build(this, input, relativeTo, ctx, t);

@timtebeek timtebeek changed the title Bug/3699 class cast exception maven parser Do not log ClassCastException in MavenParser in case of malformed POM Nov 17, 2023
@timtebeek timtebeek merged commit eee01dd into openrewrite:main Nov 17, 2023
1 check passed
@api-from-the-ion
Copy link
Contributor Author

Thank you for review and merge. I have couple of questions. So now the exception from the XML parsers itself should be logged and could be seen by the end user? Because this was a major PITA ;-) to understand and find out where this came from and which POM should be corrected and where. It the shame that we have a ParserError with a diff with the exact description where this went wrong. But it was obscured by the ClassCastException. Because I can't test it right now - are you sure that this is this way now?

Another question: you mentioned and closed another issue with this PR; are you sure that there we have the same cause from the same place in code?

The final question is like in my another ticket. The cause of all of this was that the parser parsed the original XML, but after the comparing the result with original XML some differences were found. Which causes a ParseError. And the question is: could we also change the behavior of this parser to behave more like the Maven one? So such errors wouldn't appear?

@timtebeek
Copy link
Contributor

Hi @api-from-the-ion ; You were right, the fix was incomplete; it's now been fixed in openrewrite/rewrite-maven-plugin@f6ffdc3 such that you should, finally, be able to see the original parser errors when problems occur. Thanks again!

The final question is like in my another ticket. The cause of all of this was that the parser parsed the original XML, but after the comparing the result with original XML some differences were found. Which causes a ParseError. And the question is: could we also change the behavior of this parser to behave more like the Maven one? So such errors wouldn't appear?

That'll depend on what errors occur, and I hope my changes will help surface and resolve those errors. We have our own parser based on our XmlParser, which differs from the implementation Apache Maven uses. Without knowing the details of the errors you had been getting I can't say for certain which is better.

I hope the above change help surface your issues, and please keep reporting any problems that you find; really helpful to have detailed feedback.

@timtebeek timtebeek added the bug Something isn't working label Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

ClassCastException in MavenParser in case of malformed POM
2 participants