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

[3.x] Adapt to SnakeYAML 2.0 changes #5793

Merged
merged 12 commits into from
Mar 14, 2023

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented Jan 6, 2023

Resolves #5792

SnakeYAML 2.0 is now released.

This PR updates Helidon's own code to work with either 2.0 or 1.32.

It includes changes to config and OpenAPI, as well as an integration test verifying that config and OpenAPI work with 1.32.

A change in SnakeYAML from 1.32 to 2.0 now logs many WARNING messages (they were FINE in 1.32). To my mind, these are due to a shortcoming in SnakeYAML, not in how our code uses it. I have asked about this in the Google group for SnakeYAML but have not heard back yet. In the meantime, I've included code in this PR to change the logging level for the relevant SnakeYAML logger to SEVERE and added a config setting with which a user could disable our warning suppression to see those and other more important warnings if necessary.

Most of the other changes are so our code uses more secure APIs in SnakeYAML and works with either 1.32 or 2.0.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jan 6, 2023
@tjquinno tjquinno marked this pull request as draft January 6, 2023 18:22
@tjquinno tjquinno marked this pull request as ready for review March 6, 2023 22:12
@tjquinno tjquinno self-assigned this Mar 6, 2023
@github-actions github-actions bot added the 3.x Issues for 3.x version branch label Mar 6, 2023
@@ -111,6 +111,7 @@ static class CustomRepresenter extends Representer {
CustomRepresenter(Map<Class<?>, ExpandedTypeDescription> types,
Map<Class<?>, ExpandedTypeDescription> implsToTypes, DumperOptions dumperOptions,
DumperOptions.ScalarStyle stringStyle) {
super(dumperOptions);
this.implsToTypes = implsToTypes;
this.dumperOptions = dumperOptions;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Do you still need this?)

Copy link
Member Author

@tjquinno tjquinno Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The no-args constructor in the superclass was removed. In fact, the private field is never used and I need to remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's what I meant (the private field).

Copy link
Member

@barchetta barchetta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be missing the upgrade to SnakeYaml 2.0?

I was able to build/run the test app with SnakeYaml 2 and it passes the unit tests (with verifySnakeYamlVersion disabled). But when I run it by hand it reports a pile of errors like these:

2023.03.07 13:00:46 WARNING org.yaml.snakeyaml.introspector Thread[main,5,main]: Failed to find field for org.eclipse.microprofile.openapi.models.media.Schema.enum
2023.03.07 13:00:46 WARNING org.yaml.snakeyaml.introspector Thread[main,5,main]: Failed to find [getExtensions(0 args)] for org.eclipse.microprofile.openapi.models.media.XML.extensions

@tjquinno
Copy link
Member Author

tjquinno commented Mar 7, 2023

I accidentally omitted the change to dependencies/pom.xml from my commit.

I've reproduced the warnings and am investigating.

@tjquinno
Copy link
Member Author

tjquinno commented Mar 7, 2023

The warnings are appearing now from the SnakeYAML PropertySubstitute class because the logging level for these messages was changed from FINE in 1.x to WARNING in 2.0 as part of converting from java.util.logging to SnakeYAML's own logging facility.

Similar warnings now appear when, for example, building our QuickStart app (which has OpenAPI support in it).

The good news is that the internal model is being built correctly, despite these warnings, but the messages are alarming. I'm in the process of looking for exactly why these messages are appearing despite the model being built properly. Once we know that, we can look for a way to avoid the messages.

@tjquinno
Copy link
Member Author

tjquinno commented Mar 8, 2023

I have found the immediate cause of the messages. It's not yet clear what the best workaround will be.

See https://groups.google.com/g/snakeyaml-core/c/JOzKbNLorHs/m/K2XO2on7AgAJ where I explained the cause and have asked about a solution.

@tjquinno
Copy link
Member Author

tjquinno commented Mar 8, 2023

The MP OpenAPI TCK fails because it uses Jackson dataformat which in turn uses SnakeYAML, in particular at least one 1.x constructor removed in 2.0:

java.lang.NoSuchMethodError: 'void org.yaml.snakeyaml.parser.ParserImpl.<init>(org.yaml.snakeyaml.reader.StreamReader)'
	at com.fasterxml.jackson.dataformat.yaml.YAMLParser.<init>(YAMLParser.java:178)

Jackson has updated to use SnakeYAML 2.0 (FasterXML/jackson-dataformats-text#390) but has not yet issued a release containing the fix. It seems targeted for 2.15 which is forecast to have a release candidate in March 2023. https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.15

In the meantime, we can override which release the TCK uses while still using SnakeYAML 2.0 to build our artifacts.

@tjquinno tjquinno changed the title Adapt to SnakeYAML 2.0 changes [3.x] Adapt to SnakeYAML 2.0 changes Mar 8, 2023
spericas
spericas previously approved these changes Mar 9, 2023
@tjquinno tjquinno merged commit 2cf63d6 into helidon-io:helidon-3.x Mar 14, 2023
@tjquinno tjquinno deleted the snakeyaml-test-3.x branch March 14, 2023 17:12
@Sineaggi
Copy link

A change in SnakeYAML from 1.32 to 2.0 now logs many WARNING messages (they were FINE in 1.32).

The mapping should be the same. SnakeYaml 2 is now an MR jar with a module-info and a custom logging implementation on java 9+ (switches from JUL to the java platform logger).

I based the change off of FINE (in JUL) having a severity of 500 and DEBUG (in JPL) having a severity of 500.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issues for 3.x version branch OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3.x] Prepare for SnakeYAML 2.0
5 participants