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

Switch to Yaml 1.2 with snakeyaml-engine #28349

Closed
wants to merge 6 commits into from

Conversation

michaldo
Copy link

This PR replace snakeyaml with snakeyaml-engine in order to support YAML 1.2
Version 1.2 solves among others Norway problem https://hitchdev.com/strictyaml/why/implicit-typing-removed, mentioned in spring-projects/spring-boot#17113

Please note that to mitigate the Norway problem, Spring Boot team decided to use apostrophe in every documentation snippet: spring-projects/spring-boot#28709. That decreases documenation readability.

The following page compares versions 1.1 and 1.2: https://yaml.readthedocs.io/en/latest/pyyaml.html

I drop off 'supportedTypes' support. The feature was added in #25152. I asked question for purpose, but I didn't get answer.

I decide to drop because:

  1. Supported types are not directly provided by snakeyaml-engine
  2. Clients - for example Spring Boot OriginTrackedYamlLoader - still may modify yaml setup and implement supported types
  3. Supported types are not documented
  4. I think that supported types is wrong idea. Parsing YAML should be simple, conversion should be implemented in application code.

This PR replace snakeyaml with snakeyaml-engine in order to support YAML 1.2
Version 1.2 solves among others Norway problem https://hitchdev.com/strictyaml/why/implicit-typing-removed, mentioned in spring-projects/spring-boot#17113

The following compares versions 1.1 and 1.2: https://yaml.readthedocs.io/en/latest/pyyaml.html

I drop off 'supportedTypes' support. The feature was add in spring-projects#25152. I asked question for purpose, but I didn't get answer.

I decide to drop because:
1. Supported types are not directly provided by snakeyaml-engine
2. Clients - for example Spring Boot OriginTrackedYamlLoader - still may modify yaml setup
3. Supported types are not documented
4. I think that supported types is wrong idea. Parsing YAML should be simple and conversion may be implemented in application code.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 15, 2022
@linghengqian
Copy link

YAML 1.1 and YAML 1.2 are two different sets of specifications, which is why SnakeYAML separates the two components. Is there an issue to discuss such a big change in advance?

@michaldo
Copy link
Author

Main reason to change in ambigous conversion unquoted Yes, No, On, Off.
I do not think the change is big. Contrary, I think the change will be transparent to almost all users.

@linghengqian
Copy link

Overall, there are too many unit tests to fix – a change that completely destroys downstream use.

@michaldo
Copy link
Author

michaldo commented Jun 7, 2022 via email

@michaldo
Copy link
Author

michaldo commented Jun 8, 2022

@linghengqian I merged main branch to PR and failing tests disappeared

@linghengqian
Copy link

CC @sbrannen

@rstoyanchev rstoyanchev added in: core Issues in core modules (aop, beans, core, context, expression) type: dependency-upgrade A dependency upgrade labels Feb 9, 2023
@asomov
Copy link
Contributor

asomov commented Feb 28, 2023

@michaldo I tried to move to SnakeYAML Engine also, but I found a dependency with Spring Boot. I think it should be taken together.

@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement and removed type: dependency-upgrade A dependency upgrade status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 28, 2023
@bclozel
Copy link
Member

bclozel commented Feb 28, 2023

Since there is no so much community demand for this, we're declining this PR for now; it's been declined in Spring Boot as well, see spring-projects/spring-boot#17113. The behavior changes are likely to break configuration of existing applications.

We could offer Yaml 1.2 as an alternate strategy in the future, but we don't feel like we should invest time in this right now. We might reconsider this decision in the future. Thanks!

@bclozel bclozel closed this Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants