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

Clarify when retaining . characters in property keys requires bracket notation to be used #23390

Closed
wilkinsona opened this issue Sep 17, 2020 · 7 comments
Assignees
Labels
type: documentation A documentation update
Milestone

Comments

@wilkinsona
Copy link
Member

The documentation currently states the following:

When binding to Map properties, if the key contains anything other than lowercase alpha-numeric characters or -, you need to use the bracket notation so that the original value is preserved. If the key is not surrounded by [], any characters that are not alpha-numeric or - are removed.

This isn't entirely accurate as . characters do not always require the use of bracket notation to be retained. Consider the following application.properties:

example.properties.alpha.bravo=one
example.properties.[charlie.delta]=two

And the following @ConfigurationProperties

@ConfigurationProperties("example")
class ExampleProperties {
	
	private final Properties properties = new Properties();

	public Properties getProperties() {
		return properties;
	}

}

Binding results in properties having the following content:

{charlie.delta=two, alpha.bravo=one}

Note that the . in the property key has been retained in both cases.

@kdvolder
Copy link
Member

I guess the question here is really whether people should really be relying on some potential special case behaviors where the '.' doesn't need to be escaped. I.e. is this something one should really rely on? It seems to me (as far as I understand things now)... while there may be some cases where the '.' do not need to escaped, it is safer to just escape them as that will work correctly always.

It might still be worth thinking about how to reword / explain things (clearly the wording about 'removing' is wrong since I do not think '.' are ever removed, though I guess in some situations they are going to be interpreted as 'step into a nested structure' and the reason for escaping is not to avoid removal, but to avoid splitting the key into parts.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Oct 3, 2020
@wilkinsona
Copy link
Member Author

@snicoll recalled that this is probably due to special handling for a Map with string keys. Log levels are another example of the . characters not needing to be escaped.

@wilkinsona wilkinsona self-assigned this Oct 12, 2020
@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Oct 12, 2020
@kdvolder
Copy link
Member

kdvolder commented Oct 13, 2020

I may be wrong about this, (w.r.t. how this actually works in spring boot).

But I seem to remember something about the log level case and how I implemented its special case in the past for STS. I assumed what makes it special is, not that the key of the map is a String, but rather that the value of the map is something 'atomic' (an enum in this case). So because the values bound to the map are not things that can be 'stepped into' using a '.', it makes some sense that the '.' do not need to be escaped. That's how STS makes the decision that 'logging.level' is a special case where it is okay to not escape the '.'. What I'm not sure about now though is whether that rule is an accurate reflection of what spring boot actually does. The 'key is a string' rule makes less logical sense to me, but if that is how it actually works, maybe I have to revisit how STS implemented this. So in any case it would be good to get some clarity on this in the docs.

I think it may also be good to consider leaving this behavior unspecified and instead discourage people from relying on these special cases. (I.e. in my opinion it is fine to just recommend that a '.' that is part of a key should always be escaped. Simply think of the special cases that makes this escaping not always necessary as an implementation detail that should not be relied upon.

@philwebb
Copy link
Member

The binding logic that determines how much of the property name is used as the key is in the MapBinder class. I think it's generally safe to assume that values binding to Strings, Numbers and Enums can all be specified without the [...] notation.

@kdvolder
Copy link
Member

kdvolder commented Oct 13, 2020

@philwebb I don't quite know how to read/understand all that code in your link. However, I did a little experiment and it seems to work more in the way that I thought it would. I.e. it seems to be based on the type of the value of the map rather than the type of the key.

I created an example:

@ConfigurationProperties("my")
public class MyProps {

	private Map<String, Person> people;

With:

public class Person {
	
	private String name;
	private int age;
       ... getter and setters..

And property file:

my.people.boss.name=Freddy
my.people.boss.age=55

This works as I would expect. Even though the map key is a String, it does not behave like logging.level.. The . between boss and name and age does not get taken as part of the key. The . is instead interpreted as a step inside the pojo. So when we print out the entries of the map we get this:

boss=Person [name=Freddy, age=55]

If it worked like logging.level special case then instead I'd expect the key for the map to be boss.name and the value to be Freddy which it would then need to parse as a Person value. So probably that would result in a error of some kind, but it doesn't.

So it is working the way that I (and therefore STS) assumed, not based on whether key is a String, but whether value is something you can navigate into?

@wilkinsona
Copy link
Member Author

@kdvolder Apologies. My comment above was written quickly during a meeting as a note to myself. It is indeed a combination of string keys and scalar values that means there's no need for the . characters to be escaped. I believe Phil was clarifying my comment when he said "values binding to Strings, Numbers and Enums can all be specified without the [...] notation".

@kdvolder
Copy link
Member

kdvolder commented Oct 14, 2020

@wilkinsona There's really no need to apologise. I'm just trying to make sure we can come to a shared understanding of how things works and a phrasing that is clear and unambiguous, as I am sure, so are you. This discussion only helps to further that purpose :-)

values binding to Strings, Numbers and Enums can all be specified without the [...] notation

I guess my problem is this phrasing is ambiguous because 'binding to' is basically a relationship between some key and some value, and the phrasing does not make it clear which of the two we are referring w.r.t. it needing to be of a certain type to trigger the 'special case rule'. To confuse matters, keys of maps are actually also values. I.e. if a map's keys are of type 'String' than we can say that the map's keys are values of type String. So when we say 'binding to a value' then it could actually mean that we are referring to the key in that phrase. Since you explicitly mentioned keys needing to be of type String before, it sounded to me like Phil was just repeating and clarifying what you already said and merely elaborating that it's not just Strings keys but also other scalar keys that trigger the rule.

@philwebb philwebb modified the milestones: 2.2.x, 2.3.x Dec 16, 2020
@wilkinsona wilkinsona removed their assignment Jan 12, 2021
@philwebb philwebb modified the milestones: 2.3.x, 2.3.9 Jan 31, 2021
@philwebb philwebb self-assigned this Jan 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation A documentation update
Projects
None yet
Development

No branches or pull requests

3 participants