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

support overriding the codePointLimit #339

Merged
merged 3 commits into from
Sep 24, 2022

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Sep 20, 2022

#337

Work in progress. Will add tests before merging but would like to open a discussion on the APIs.

Another option is to support setting a snakeyaml LoaderOptions instance on YamlFactoryBuilder, so users would have full control. There are other settings in LoaderOptions that might be useful to support. https://www.javadoc.io/doc/org.yaml/snakeyaml/latest/org/yaml/snakeyaml/LoaderOptions.html

@pjfanning pjfanning marked this pull request as draft September 20, 2022 10:51
@cowtowncoder
Copy link
Member

I think that although in many ways it's good to hide details of the underlying implementation, in this case benefits probably outweigh drawbacks.

One particularly compelling reason here is this: as I understand, this new setting is only available in SnakeYAML 1.32. If so, we either have to:

  1. Require baseline of SnakeYAML from users. This will almost certainly gain as many bug reports for breakages from users with different version of SnakeYAML in their classpath
  2. Add dynamic loading of piece of code that handles with this setting, to only fail if someone tries to set the limit against older SnakeYAML

But if we only expose functionality for interacting with LoaderOptions (when was it added?), we can avoid this problem altogether.

If it wasn't for this compatibility issue, I might suggest doing both (simple int setting and LoaderOptions), but due to it I suggest exposing LoaderOptions. This assumes it was added in a version that is not "too recent" (like, something that was the baseline for... maybe latest Jackson 2.9.x release?).

@pjfanning pjfanning marked this pull request as ready for review September 21, 2022 20:37
@pjfanning
Copy link
Member Author

@cowtowncoder I have reworked to this to allow users to set the LoaderOptions instance explicitly

@cowtowncoder
Copy link
Member

Looks good, will merge.

One thing that might make sense -- and I can try to add it -- would be an example of how to expand maximum size limit, on YAML module's README.

@cowtowncoder cowtowncoder merged commit 444e62c into FasterXML:2.14 Sep 24, 2022
@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Sep 24, 2022
@cowtowncoder
Copy link
Member

@pjfanning Was able to merge to master, but it looks as if snakeyaml-engine did not have equivalent setting: while LoaderOptions became LoadSettings, I did not see an equivalent to "max code points" in latter.

@asomov Do you know if I missed something obvious (setting is maybe somewhere else), or if omission is intentional?

@asomov
Copy link
Contributor

asomov commented Sep 25, 2022

@cowtowncoder it does not have it yet. But it will get it soon

@cowtowncoder
Copy link
Member

@asomov Excellent: no super hurry as this only affects Jackson 3.0.0-SNAPSHOT, not yet released. So just let us know when it is added. Thank you!

@asomov
Copy link
Contributor

asomov commented Oct 4, 2022

@cowtowncoder SnakeYAML Engine version 2.5 with the support for codePointLimit is released

@cowtowncoder
Copy link
Member

Thank you @asomov ! I enabled test for this in master for JAckson YAML module against snakeyaml-engine too.

/cc @pjfanning

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