Skip to content

Conversation

schulzh
Copy link
Contributor

@schulzh schulzh commented Nov 7, 2019

This PR changes the handling of JSON keys in the CloudFoundry VCAP_SERVICES environment variable so that they represent only a single path segment at a time.

Before this change, if the JSON contained an object key with dots, the key would be treated as individual path segments, instead of a single one.
E.g. the VCAP_SERVICES variable

{
  "user-provided": [
    {
      "name": "test",
      "label": "test-label",
      "credentials": {
        "key.with.dots": {
          "username": "foo",
          "password": "bar"
        }
      }
    }
  ]
}

Would lead to the spring property:
vcap.services.test.credentials.key.with.dots.username=foo
Instead of the escaped version:
vcap.services.test.credentials[key.with.dots].username=foo

This causes problems e.g. when trying to bind these properties to a map, as it would not be filled with any values:

@ConfigurationProperties("vcap.services.test")
public class CfEnvCredentials {
    public final Map<String, BasicAuthentication> credentials;
    [...]
}

@pivotal-issuemaster
Copy link

@schulzh Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@schulzh Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 7, 2019
@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Nov 7, 2019
@philwebb
Copy link
Member

philwebb commented Nov 8, 2019

@schulzh What sort of generic types do you have on your Map? We should automatically deal with this for String and scalar types (see this test). Do you have a small sample that show the problem that lead to you creating this PR?

@philwebb philwebb added status: waiting-for-feedback We need additional information before we can continue and removed for: team-attention An issue we'd like other members of the team to review labels Nov 8, 2019
@schulzh
Copy link
Contributor Author

schulzh commented Nov 11, 2019

I have a Map<String, BasicAuthentication>, with this BasicAuthentication class:

public class BasicAuthentication extends Authentication {
    public final String username;
    public final String password;
}

This is meant to supply Authentication information for arbitrary hosts, which will be the keys of the map, therefore the dots in the keys.

I just tested a Map<String, String> example and indeed, it worked. Sorry, I should have tested the actual example before. I'll edit the PR with the actual example.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 11, 2019
@philwebb philwebb added type: bug A general bug and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Dec 2, 2019
@philwebb philwebb added this to the 2.1.x milestone Dec 2, 2019
if (key.startsWith("[")) {
return path + key;
}
if (key.contains(".")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'd have to do this for any characters that are not alpha-numeric or -.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Dec 9, 2019
@snicoll
Copy link
Member

snicoll commented Dec 9, 2019

@schulzh would you have time to revisit the PR based on Madura's feedback?

mbhave pushed a commit that referenced this pull request Feb 11, 2020
mbhave added a commit that referenced this pull request Feb 11, 2020
Along with surrounding map keys with dot from VCAP_SERVICES with `[ ]`,
this commit also does that for non-alphanumeric and `-` characters so that
they are not stripped off later.

See gh-18915
@mbhave mbhave closed this in 3917968 Feb 11, 2020
@mbhave
Copy link
Contributor

mbhave commented Feb 11, 2020

@schulzh Thank you for making your first contribution to Spring Boot. This has now been merged along with this addition.

@mbhave mbhave removed the status: waiting-for-feedback We need additional information before we can continue label Feb 11, 2020
@mbhave mbhave modified the milestones: 2.1.x, 2.1.13 Feb 11, 2020
@wilkinsona wilkinsona changed the title Handle JSON keys containing a dot from CF environment as a single path segment JSON keys containing a dot from CF environment are not handled as a single path segment Feb 12, 2020
@wilkinsona
Copy link
Member

Unfortunately, this change caused some unanticipated problems and we're going to have to revert it. Please see #20343 for details. We've opened #20400 to take another look at the problem. It's tentatively scheduled for 2.3.

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

Successfully merging this pull request may close these issues.

7 participants