Skip to content

Improve document to elaborate why the slash in the map key will be removed #42802

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
quaff opened this issue Oct 21, 2024 · 7 comments
Closed
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@quaff
Copy link
Contributor

quaff commented Oct 21, 2024

When binding to `Map` properties you may need to use a special bracket notation so that the original `key` value is preserved.
If the key is not surrounded by `[]`, any characters that are not alpha-numeric, `-` or `.` are removed.

It would be nice if more words here to show the motivation.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 21, 2024
@quaff
Copy link
Contributor Author

quaff commented Oct 21, 2024

Further more, if possible, I think it should raise error with meaningful message instead of removing characters silently.

@wilkinsona
Copy link
Member

We documented when and why the special bracket notation is needed in #13506. This was then refined in #23390. I don't think we should get into the why in the reference documentation. My feeling is that it's a level of detail that the vast majority of users don't need and it will make the documentation unnecessarily verbose. Let's see what the rest of the team thinks.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Oct 21, 2024
@mhalbritter
Copy link
Contributor

I agree with Andy. I think we should document the observable behavior, and not necessarily the why.

@quaff
Copy link
Contributor Author

quaff commented Oct 21, 2024

We documented when and why the special bracket notation is needed in #13506. This was then refined in #23390. I don't think we should get into the why in the reference documentation. My feeling is that it's a level of detail that the vast majority of users don't need and it will make the documentation unnecessarily verbose. Let's see what the rest of the team thinks.

How about adding a link to #23390?

@snicoll
Copy link
Member

snicoll commented Oct 21, 2024

We shouldn't link the issue tracker from our reference doc. I also think that it would make the doc unnecessarily verbose.

@bclozel
Copy link
Member

bclozel commented Oct 21, 2024

With my vote, this makes the majority of the team. Sorry @quaff but the reference docs are not meant to track design decisions and tradeoffs but rather describe how things work.

@bclozel bclozel closed this as not planned Won't fix, can't repro, duplicate, stale Oct 21, 2024
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Oct 21, 2024
@quaff
Copy link
Contributor Author

quaff commented Oct 25, 2024

Further more, if possible, I think it should raise error with meaningful message instead of removing characters silently.

@philwebb How about this proposal?

The current behavior is counter-intuitive, there are several issues that reporter is confused by my.map./key3=value3 is mapped as key3=value3, exception should be thrown to state that last element of the ConfigurationPropertyName should be written as ElementType.INDEXED instead of ElementType.NON_UNIFORM, better than documenting that, not everyone read document before writing code.
It's indeed a breaking change, but I doubt someone would rely on such quirk behavior, it's not worthy to make it as an opt-in feature.

EDIT: I created prototype https://github.com/spring-projects/spring-boot/compare/main...quaff:spring-boot:patch-87?expand=1

quaff added a commit to quaff/spring-boot that referenced this issue Oct 28, 2024
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
quaff added a commit to quaff/spring-boot that referenced this issue Oct 28, 2024
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
quaff added a commit to quaff/spring-boot that referenced this issue Oct 28, 2024
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
quaff added a commit to quaff/spring-boot that referenced this issue Oct 28, 2024
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
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

No branches or pull requests

6 participants