Skip to content

Reject binding non-uniform property key as map key #42904

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

Closed
wants to merge 2 commits into from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Oct 28, 2024

Given:

my:
  map:
    "/key": "value"

Before this commit:
It's equivalent to

my:
  map:
    "[key]": "value" # "[/key]" is expected

Such counter-intuitive behavior will confuse developers, there are several reported issues, incomplete list: GH-41099 GH-29582 GH-24548


After this commit:

***************************
APPLICATION FAILED TO START
***************************

Description:

Failed to bind properties under 'my.map' to java.util.Map<java.lang.String, java.lang.String>:

    Reason: java.lang.IllegalArgumentException: Please rewrite key '/key' to '[/key]' and surround it with quotes if YAML is using

Action:

Update your application's configuration


See GH-42802

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 28, 2024
@quaff quaff marked this pull request as draft October 28, 2024 04:30
@quaff quaff force-pushed the patch-87 branch 2 times, most recently from bc89cd6 to bbfe04d Compare October 28, 2024 04:45
Given:
```yaml
my:
  map:
    "/key": "value"
```
---
Before this commit:
It's equivalent to
```yaml
my:
  map:
    "[key]": "value" # "[/key]" is expected
```
Such counter-intuitive behavior will confuse developers, there are several reported issues, incomplete list: spring-projectsGH-41099 spring-projectsGH-29582 spring-projectsGH-24548

---
After this commit:
```
***************************
APPLICATION FAILED TO START
***************************

Description:

Failed to bind properties under 'my.map' to java.util.Map<java.lang.String, java.lang.String>:

    Reason: java.lang.IllegalArgumentException: Please rewrite key '/key' to '[/key]' and surround it with quotes if YAML is using

Action:

Update your application's configuration

```
---

See spring-projectsGH-42802
@philwebb
Copy link
Member

Thanks @quaff. I read your comment on the other issue but didn't have a chance to reply due to last minute work for the release. We'll discuss this one when we can.

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Oct 28, 2024
@quaff quaff marked this pull request as ready for review October 29, 2024 00:40
@philwebb
Copy link
Member

philwebb commented Nov 4, 2024

Thanks for the PR @quaff. I agree that it would be nice to fix this, but I don't think we should put so much YAML knowledge into the MapBinder. I think we probably need to do something with ConfigurationPropertyName and possibly add a failure analyzer. I'm not too sure on the design details yet, that will have to be something we look at after 3.4 is out of the door.

@philwebb philwebb closed this Nov 4, 2024
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Nov 4, 2024
@philwebb
Copy link
Member

philwebb commented Nov 4, 2024

I've opened #42984 so we don't forget about the issue.

@quaff
Copy link
Contributor Author

quaff commented Nov 5, 2024

but I don't think we should put so much YAML knowledge into the MapBinder

Just for friendly error message, it doesn't couple YAML and MapBinder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants