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

SpringProfileDocumentMatcher does not work as expected #8011

Closed
michael-simons opened this issue Jan 17, 2017 · 16 comments
Closed

SpringProfileDocumentMatcher does not work as expected #8011

michael-simons opened this issue Jan 17, 2017 · 16 comments
Assignees
Labels
theme: config-data Issues related to the configuration theme type: bug A general bug
Milestone

Comments

@michael-simons
Copy link
Contributor

The following issue appears with Spring Boot 1.4.3:

Given a YAML configuration like

someValue: 4711

example:
    greeting: Hello, World!
---
spring:
    profiles: demo1
example:
    greeting: Hello, Demo1!

---
spring:
    profiles: demo2
example:
    greeting: Hello, Demo2!

---
spring:
    profiles: "!demo2"
someValue: 2109

(Full configuration and properties class in example project extconfig see: application.yml and ExampleConfiguration)

and run with java -jar target/extconfig.jar I expect someValue to be 2109: The negated profile !demo2 overwriting the value of 4711.

run with java -jar target/extconfig.jar --spring.profiles.active=demo2 I expect someValue to be 4711 as it is set in the default, not overwritten in demo2 (not set) and not overwritten bei !demo2. The same is expected for the other parts of the configuration.

In contrast to my expectations, I get in both cases 2109.

The project linked above contains a test class NegatedProfilesTest that fails.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 17, 2017
@wilkinsona
Copy link
Member

wilkinsona commented Jan 17, 2017

@michael-simons and I discussed this a bit already. The problem appears to be that SpringProfileDocumentMatcher isn't correctly configured with the active profiles. Instead, it only considers a profile to be active if it's processing a configuration file that's specific to that profile.

@mbenson
Copy link
Contributor

mbenson commented Feb 3, 2017

I've attacked this a couple of ways and it seems a pretty tough nut to crack within the confines of the existing PropertySourcesLoader/PropertySourceLoader API. I don't have anything helpful yet, but anything that can get folks thinking about the problem. The SpringProfileDocumentMatcher works pretty well in a vacuum. :|

@mbenson
Copy link
Contributor

mbenson commented Feb 4, 2017

Okay, on my third attempt I have some code that seems to be working. I'm not submitting a PR just yet because I think my approach is invasive enough that it bears discussion, but as I said it's not a simple problem to solve within the confines of the existing APIs: https://github.com/mbenson/spring-boot/tree/yaml-negation

@philwebb philwebb added priority: normal type: bug A general bug theme: config-data Issues related to the configuration theme and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 6, 2017
@mbenson
Copy link
Contributor

mbenson commented Feb 6, 2017

Over the weekend it occurred to me that it was only sensible to handle spring.profiles.include in this code as well. After a bit of work I came to the conclusion that it was necessary to determine the active profile information in a first pass before coming back to actually load the profile-specific subdocuments; the branch referenced above now reflects this.

@mbenson
Copy link
Contributor

mbenson commented Feb 10, 2017

I thought of another way to break the code I had in place, e.g.:
application.yml:

spring.profiles:
  - "!foo"

application-bar.yml

spring:
  profiles:
    include:
      - foo

My branch https://github.com/mbenson/spring-boot/tree/yaml-negation now accounts for this case, whenever the team is ready for a discussion. :)

@michael-simons
Copy link
Contributor Author

Thanks @mbenson for your work on this!

@mbenson
Copy link
Contributor

mbenson commented Feb 14, 2017

You're welcome--Having been AFAIK the first person who requested/PR'd the YAML profile negation feature, I did feel responsible. Hopefully this gives the team enough to go on to bang out a solution they are comfortable with.

@leonard84
Copy link
Contributor

@mbenson will your PR also fix the issue with

spring:
  profiles:
    include: workstation
    active: local

which should only activate workstation if local is active, however with Spring Boot 1.5.2 and prior this just unconditionally activates workstation.

This would be better but it seems to be not possible in YAML

spring:
  profiles: local
    include: workstation

@mbenson
Copy link
Contributor

mbenson commented Mar 13, 2017

I don't agree that your first example should behave as you describe. I read this as activating local and including workstation (thus more or less activating both). What you describe as "would be better" should work with my PR, but you are right that YAML won't support the syntax you've shown. It is necessary to "trick" the YAML parser with:

spring.profiles:
  - local

spring:
  profiles:
    include:
      - workstation

@leonard84
Copy link
Contributor

@mbenson with my first example I'm referring to http://stackoverflow.com/a/34699686/2145769 which gives a false solution it seems.

Your "trick" might work, but it is more a hack than a good solution at least from a DX perspective. IMHO there should be another property to denote profiles in multi-doc YAML file that works for this case, e.g., spring.profiles.names, spring.profiles.ids. Maybe @wilkinsona could help with this?

spring:
  profiles:
    ids: local
    include: workstation

@mbenson
Copy link
Contributor

mbenson commented Mar 15, 2017

Certainly it would take a member of the Spring (Boot) team to decide to honor some other property for this purpose. Also, I'm not sure to what "DX" refers.

@leonard84
Copy link
Contributor

DX=developer experience

And sure the Spring (Boot) team would have to agree to a change.

michael-simons added a commit to springbootbuch/extconfig that referenced this issue Jul 12, 2017
Does not make sense to have in the book as long as spring-projects/spring-boot#8011 is open.
@leonard84
Copy link
Contributor

@wilkinsona / @philwebb as Spring 2.0 is on the horizon this would be an ideal way to straighten out the kinks of the profile handling, since it would be a breaking change (small changes required but still).

shakuzen added a commit to shakuzen/spring-boot that referenced this issue Dec 19, 2017
Documenting this feature misleads users into thinking it should work. The documentation can be added back once spring-projectsgh-8011 is fixed.
@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Dec 23, 2017
@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Jan 10, 2018
@wilkinsona wilkinsona added this to the 2.0.0.RC2 milestone Jan 10, 2018
@mbhave mbhave self-assigned this Feb 2, 2018
@mbhave
Copy link
Contributor

mbhave commented Feb 5, 2018

@mbenson thanks for your work on this. Would you like to contribute a PR with the suggested approach of adding another property source if negated profiles are present? I wonder if it's possible to have just one property source corresponding to the negated profile. The code in the branch seems to add the same ActiveProfileConditionalYamlPropertySource per active profile and I'm not sure if that's necessary.

@mbenson
Copy link
Contributor

mbenson commented Feb 5, 2018

@mbhave I will try to take a look at that this week and get back up to speed. Thanks for the attention!

@mbhave mbhave removed their assignment Feb 12, 2018
@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Feb 13, 2018
@mbhave mbhave self-assigned this Feb 15, 2018
@mbhave mbhave removed the status: waiting-for-feedback We need additional information before we can continue label Feb 16, 2018
@philwebb philwebb assigned philwebb and unassigned mbhave Feb 21, 2018
philwebb pushed a commit that referenced this issue Feb 21, 2018
@philwebb
Copy link
Member

I've fixed this in a slightly different way in the end (at least I think it's fixed). The SpringProfileDocumentMatcher has quite a lot of complex logic so I've decided to drop it entirely in 2.0. I'm pretty sure that we're the only people really using it. I've replaced it with two new matchers that are much simpler in design. This approach doesn't introduce too much complexity, but it also doesn't quite work if profiles are activated in other profiles. The ...cascade... test in Madhura's branch now fail.

The reason for this is that on the first call, there are no profiles set. This means that any negative sections will match. So given this file:

spring:
  profiles:
    active:
      - A
      - B

+---
spring.profiles: "!A"
not-a: true      

You will get a not-a property because the at the time the file is loaded, profile A isn't active.

I think that's actually pretty consistent with other yaml loading that we do and I prefer it to checking for active profiles when properties are resolved. For the sake of simplicity, I think we should live with that restriction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: config-data Issues related to the configuration theme type: bug A general bug
Projects
None yet
Development

No branches or pull requests

8 participants