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

Allow to use long keys in YAML #334

Merged
merged 1 commit into from
Sep 14, 2022
Merged

Allow to use long keys in YAML #334

merged 1 commit into from
Sep 14, 2022

Conversation

ShauniArima
Copy link
Contributor

@ShauniArima ShauniArima commented Sep 12, 2022

Adding a new feature (USE_LONG_KEYS) that set maxSimpleKeyLength to 1024 (the maximum available length). It is set to false by default to keep the default behaviour of SnakeYAML.

It answer this issue : #244.

@yawkat
Copy link
Member

yawkat commented Sep 12, 2022

It seems to me like this should be configurable as a number. But not sure if it's worth the effort for adding the API for that, and if it doesn't compromise the "hide snakeyaml" idea.

@ShauniArima
Copy link
Contributor Author

In my case, I have yaml-generated keys that can be very long and cause a lot of output with ? . So I need to have this kind of configuration.

In the first place, I have defined this feature as a toggle so it remains simple to use it. As you said, it can be a number. But I am not sure this is really worth it. Maybe some users really know the max lengths of their keys. If this is the main case, it will be good to use a number here. I have not found much feedback about this, do we have some?

@cowtowncoder
Copy link
Member

This sounds like a good improvement. I have just one significant concern here: backwards-compatibility. Since code as-is would have hard dependency for the new method, it would strictly require use of a specific minimum version. This may not sound like a big deal but with transitive dependency challenges chances are that it would make some upgrades more difficult.

So: which version of SnakeYAML was this added in?

Specifically I am worried about failure during runtime for cases where configuration method is NOT called -- if user does try to enable long-string support it's fair to assume version is new.
But if there's simply an upgrade in jars and no attempt is made, it'd be good not to fail.
This can be, I think, achieved by separating out code for setting the feature into separate class, which is only called when specific Feature is changed -- and in that case, catch and re-throw class loading failure if there is one (to indicate that an old dependency version exists).

This can be tested manually with older SnakeYAML version; not sure if there's a way (maybe using Maven profiles?) to add an automated test (integration test?)

But at any rate, unless this setting is available in relatively old version (Jackson 2.12 depends on 1.27 for example), I think it is necessary to consider compatibility aspect.

@@ -310,6 +320,10 @@ protected DumperOptions buildDumperOptions(int jsonFeatures, int yamlFeatures,
if (Feature.USE_PLATFORM_LINE_BREAKS.enabledIn(_formatFeatures)) {
opt.setLineBreak(DumperOptions.LineBreak.getPlatformLineBreak());
}

if (Feature.USE_LONG_KEYS.enabledIn(_formatFeatures)) {
opt.setMaxSimpleKeyLength(1024);
Copy link
Member

Choose a reason for hiding this comment

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

This is the setting that would need to be encapsulated in a separate class, with a method that gets opt passed in, sets the maximum length. Since that class is only loaded when method called (first "active use") it is possible
to avoid failure when YAMLGenerator class itself is initialized.
Class loading exception should trigger throwing of exception that suggests too old SnakeYAML dependency.

@ShauniArima
Copy link
Contributor Author

ShauniArima commented Sep 12, 2022

According to this ticket. This feature has been available since version 1.24 of SnakeYaml.

I will check on adding a wrapper to make it less error-prone as you explain it. 😄

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 12, 2022

@ShauniArima Actually if it's that old a fix, never mind -- Jackson 2.10.0 already had snakeyaml 1.24 that as the dependency version. I am not as worried in that case, wrt 2.14.0.

So the one and only thing I'd need (unless already done, apologies if I forgot) is the CLA, from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and the usual way is to print it, fill & sign, scan/photo, email to info at fasterxml dot com.
And once I get that, I can merge this and should get right into 2.14.0-rc1 that I hope to FINALLY release this week! :)

@ShauniArima
Copy link
Contributor Author

I have filled and signed the CLA. You will receive a mail soon. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants