Skip to content

Expose Spring Integration global properties #25377

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

Conversation

artembilan
Copy link
Member

Spring Integration comes with some global properties
which can be configured via META-INF/spring.integration.properties.
The framework then provides an integrationGlobalProperties bean as an
org.springframework.integration.context.IntegrationProperties instance

  • Expose those properties via an
    org.springframework.boot.autoconfigure.integration.IntegrationProperties
    for end-user convenience to have everything in a single application.properties
  • Map respective org.springframework.boot.autoconfigure.integration.IntegrationProperties
    props into an org.springframework.integration.context.IntegrationProperties and
    expose the last one as an integrationGlobalProperties bean for further
    Spring Integration framework considerations

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 19, 2021
@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 20, 2021
@snicoll snicoll added this to the 2.5.x milestone Feb 20, 2021
@snicoll
Copy link
Member

snicoll commented Feb 20, 2021

@artembilan thanks for the PR but I am not sure I understood the migration path for users. Let's say they specify this file in their application with some customization. From a quick look at the code, given that they are defaults in the @ConfigurationProperties bean, these will be overwritten. Have I missed anything?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Feb 21, 2021
@artembilan
Copy link
Member Author

Thanks, @snicoll , for heads up!

You are right: we definitely need to explain a migration path for end-users.
I was so concentrated how to implement this in Spring Boot and how to write test-cases to validate that I completely forgot the fact that this feature is really a breaking change for target applications 😄 .

Here is how it works right now:

  • If there is no META-INF/spring.integration.properties file in classpath, Spring Integration creates an org.springframework.integration.context.IntegrationProperties bean with default values.
  • If there are some META-INF/spring.integration.properties files, Spring Integration merges them in the order they are read from classpath and creates org.springframework.integration.context.IntegrationProperties bean based on properties with defaults for the rest.
  • If there is an integrationGlobalProperties bean, the presence of META-INF/spring.integration.properties is fully ignored. I would say similar to "ConditionalOnMissingBean".

The idea with this fix is to let end-users to migrate from custom META-INF/spring.integration.properties to common application.properties. I believe my problem is that I have missed the presence of META-INF/spring.integration.properties in the target application fully falling back to default values. Breaking change as we call it for the current minor release.

So, let me play with something like "ConditionalOnMissingResource" to let the target application to stay stable after upgrading to Spring Boot 2.5. In the end we probably need to mention this feature for Spring Integration in the release notes to give end-users a chance to migrate their properties to Spring Boot style.

Thank you again for spotting the flaw in my PR!

@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 Feb 22, 2021
@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Feb 27, 2021
@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Mar 5, 2021
@snicoll
Copy link
Member

snicoll commented Mar 6, 2021

We discussed this one at the team meeting and we felt that making things a little bit more explicit would be better to let users debug where things are coming from. One thing that doesn't feel right is that one ways is winning over the other while we could do this on a property per property basis.

Our idea was to change this so that META-INF/spring.integration.properties is read by an EnvironmentPostProcessor and added in the Environment with the right precedence order. Then we'd have a single way of building the IntegrationProperties based on the content of the environment. This approach has several advantages:

  • The rule of what wins on what is clearly defined in the environment and can be debugged with /actuator/env if necessary
  • Rather than having the integration-specific file win over everything , we can still use application.properties if the property is not defined there
  • Thanks to our Origin support, it is easier to know what contributed to a given setting.

@artembilan Would you have the time to update your PR in that direction?

@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Mar 6, 2021
@artembilan
Copy link
Member Author

Thanks, @snicoll , for feedback!

Even if my preference would be do not encourage end-users to use META-INF/spring.integration.properties when they build Spring Boot application, I would say during the migration path, when they already have that file, it would be really beneficial to see those properties in the /actuator/env.

So, yeah, I will add respective EnvironmentPostProcessor today to re-map META-INF/spring.integration.properties to the appropriate IntegrationProperties entries to have a proper merged instance in the end.

@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 Mar 8, 2021
@snicoll
Copy link
Member

snicoll commented Mar 8, 2021

Even if my preference would be do not encourage end-users to use META-INF/spring.integration.properties when they build Spring Boot application.

This has my preference as well and the reason why we're trying to build some sort of middle ground solutions here. I would add this file with lowest precedence then, to encourage the use of the regular Environment. The problem here, I think, is that it'll be hard to deprecate it if Spring Integration doesn't do it. Food for thoughts.

@artembilan
Copy link
Member Author

Please, see an update in the PR about an IntegrationEnvironmentPostProcessor.
I'm not fully sure that I understood an OriginLookup contract correctly.
Plus it is not clear how to test with EnvironmentPostProcessors in the ApplicationContextRunner.

Thanks

Spring Integration comes with some global properties
which can be configured via `META-INF/spring.integration.properties`.
The framework then provides an `integrationGlobalProperties` bean as an
`org.springframework.integration.context.IntegrationProperties` instance

* Expose those properties via an
`org.springframework.boot.autoconfigure.integration.IntegrationProperties`
for end-user convenience to have everything in a single `application.properties`
* Map respective `org.springframework.boot.autoconfigure.integration.IntegrationProperties`
props into an `org.springframework.integration.context.IntegrationProperties` and
expose the last one as an `integrationGlobalProperties` bean for further
Spring Integration framework considerations
missing `META-INF/spring.integration.properties`.
Otherwise end-user existing environment wins to properly mitigate
a migration pass
…ring.integration.properties`

file and register it as an origin resource for mapped `IntegrationProperties` from it and as
the last one letting the properties from a general Spring Boot environment to win
@artembilan artembilan force-pushed the IntegrationProperties branch from 16a17a5 to 280429f Compare March 19, 2021 17:38
@artembilan
Copy link
Member Author

Hi there!

Is there anything I need to do for this PR?
Looks like we are heading to RC1 in April, so would be great to have this merged.

Thank you!

@snicoll
Copy link
Member

snicoll commented Mar 22, 2021

Is there anything I need to do for this PR?

I am not sure. We haven't got the chance to review the updated PR. Thanks for the follow-up.

@snicoll snicoll self-assigned this Mar 23, 2021
@snicoll snicoll modified the milestones: 2.5.x, 2.5.0-RC1 Mar 23, 2021
snicoll pushed a commit that referenced this pull request Mar 24, 2021
Spring Integration comes with some global properties which can be
configured via `META-INF/spring.integration.properties`. The framework
then provides an `integrationGlobalProperties` bean as an
`org.springframework.integration.context.IntegrationProperties`
instance.

This commit allows users to configure these using regular
`application.properties`. If a `META-INF/spring.integration.properties`
file exists, the values are used as fallback.

See gh-25377
snicoll added a commit that referenced this pull request Mar 24, 2021
@snicoll snicoll closed this in 160cd0d Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants