Issue with property resolution with PlaceholderParser

[mbazos]

Hi my team has encountered an issue with the latest version of spring-core and unfortunately we don't have a fix but we can replicate it with the sample project below. Let me outline the problem.

Given application.properties like so

gsm\://one=result
key-one=one
key-two=${gsm://${key-one}}
key-three=${gsm://one}

It appears spring-core 6.1.14 would translate property keys key-two and key-three to be result but when upgrading to 6.2.1 it appears the output of these properties have changed. Unfortunately this is causing a breaking change for us and I believe it's probably a bug in spring-core. We narrowed it down to changes in how placeholders are resolved in PlaceholderParse but were unsuccessful in coming up with a reasonable fix.

In addition to the properties above we tried escaping the : as that's a Spring separator but that doesn't seem to fix the issue. The test.zip contains a test which if you switch between spring framework 6.1.14 and 6.2.1 will show the problem.

Please let me know if you have additional questions.

test.zip

[bclozel]

Duplicates #34124

[mbazos]

Thanks @bclozel I was searching for issues to see if they were related and didn't find anything. Looks like this is going to be fixed next week with Spring Framework 6.2.2.

[snicoll]

In addition to the properties above we tried escaping the : as that's a Spring separator but that doesn't seem to fix the issue.

The escaping should happen in the placehoder itself (i.e. ${gsm\://one} would resolve the gsm://one key). The key itself shouldn't be escaped since it isn't a placeholder.

While this is going to be "fixed" for backward compatible reason, such usage is strongly discouraged. Do not use a reserved character in your property key, we may revisit this in the future so that it doesn't work in a new major release.

[mbazos]

Thanks @snicoll I am going to talk to my team based on this guidance I think we will change the placeholder from ${gsm\://one} to ${gsm//one} which removed the reserved keyword.

I assume this could be stricter in future versions of spring-framework or even removed in a major version upgrade.

[mbazos]

@snicoll We are working to remove the reserved keyword : form our property placeholder lookup, but was curious why was this changed/broken in spring-core? I know regardless we needed to escape the : but was this to fix a bug? is it due to security reasons or something else?

Just looking for an explanation why this is happening so I can properly communicate that to my colleagues. Spring has been great over the years for us and breaking changes like this almost never happen unless there is a major upgrade and it's usually documented clearly.

Also I was curious and did test this with Spring Framework 6.2.2 with a property like this:

${gsm\\://${key-one}}

This does not seem to work, but this does work in 6.2.2 whereas in 6.2.1 this was broken:

${gsm\\://one}

Either way we are getting rid of the reserved keyword : just wanted to know if there is more to this change then it being a reserved keyword.

[snicoll]

why was this changed/broken in spring-core?

It wasn't changed on purpose if that's what you are asking. In 6.2 the parser was completely rewritten to only attempt to resolve a part when needed. Previously we had weird bugs where if you had something like ${key:my ${fallback} value} it would attempt to resolve fallback even if keywas present. Another weird behavior is the one that I missed (trying the full placeholder content even if it is a valid placeholder structure before parsing it). We unfortunately had no test for it so I suspect this worked by accident, and so your use of, e.g. ${gsm://one}.

Ironically enough, the escape feature was added in the new parser in case someone really has to use : in the key!

Spring has been great over the years for us and breaking changes like this almost never happen unless there is a major upgrade and it's usually documented clearly.

What are you talking about? The issue has been reported and fixed within a month. Or are you saying it doesn't work for you?

As for your example, I am not sure what you mean. You can create a new issue with a demo if you think that escaping no longer works in 6.2.2

[mbazos]

What are you talking about? The issue has been reported and fixed within a month. Or are you saying it doesn't work for you?

@snicoll Just saying over my 17+ years of using Spring these kinds of issues almost never happen. You all did a great job fixing it within a very timely manner, but I did test with Spring 6.2.2 and I think it's still an issue there. Please take a look at my zip.

6.1.14 works fine for both unit tests (remove escaping from properties because it's not required)

6.2.2 fails for this

org.opentest4j.AssertionFailedError: 
Expected :result
Actual   ://one

We are asking all the teams to change from gsm:// to gsm// which works fine with all version but wanted to be clear something is still broken in Spring 6.2.2 if the goal was to maintain full backwards compatibility.

test.zip

[snicoll]

Just saying over my 17+ years of using Spring these kinds of issues almost never happen.

I am not sure what that means or why you bring this up. To me, this can only be read as a criticism or a passive aggressive statement. Let's keep in mind that your use of Spring was incorrect to begin with: placeholders use : to separate the key from the fallback for as long as I can remember (2009 by the looks of it when configured in plain Spring Framework, a bit later with Spring Boot doing this automatically).

but I did test with Spring 6.2.2 and I think it's still an issue there. Please take a look at my zip.

Backslash in properties file (like in a String in Java) needs to be doubled. This should be changed to key-two=${gsm\\://${key-one}}.

but wanted to be clear something is still broken in Spring 6.2.2 if the goal was to maintain full backwards compatibility.

Full backward compatibility means the use of : as part of the key would still work and that's impossible to do while fixing legitimate bugs. That's why the escaping has been introduced.

[mbazos]

Thanks @snicoll for the explanation.

I didn't mean any criticism and was not being passive aggressive. My apologies if it came off like that. What I meant by it really is how great of a job you and your team have been doing over the years managing Spring with such a large community. I really do appreciate it.