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.

Sign up for GitHub

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

Jump to bottom

Allow common messages to be specified for message sources #42472

Closed
wants to merge 5 commits into from
Closed

Allow common messages to be specified for message sources #42472

wants to merge 5 commits into from

Conversation

mmoayyed
Copy link
Contributor

This change extends the configuration schema to support the specification of common messages, when defining a messageSource bean. Common messages are described here:

Specify locale-independent common messages, with the message code as key and the full message String (may contain argument placeholders) as value.

Still in the process of testing this, and figured it would be good to post this early to get feedback.

Thank you!

@mmoayyed
Allow common messages to be specified for message sources
a9b64dc 
Extend the configuration schema to support the specification of common messages, when defining a messageSource bean
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 29, 2024
@snicoll snicoll mentioned this pull request Sep 30, 2024
Closed
@mmoayyed
Copy link
Contributor Author

mmoayyed commented Oct 4, 2024

Hello all, I see this PR has been "partially" reviewed. In concept, does this pass muster, and can it be included in the coming RC? Is the idea sound here?

philwebb
philwebb reviewed Oct 16, 2024
View reviewed changes
Copy link
Member

@philwebb philwebb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @mmoayyed. I've left some comments for your consideration.

...re/src/main/java/org/springframework/boot/autoconfigure/context/MessageSourceProperties.java Outdated
/**
* Comma-separated list of locale-independent common messages.
*/
private String commonMessages;
Copy link
Member

Choose a reason for hiding this comment

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

I think we might be better off using a List<Resource> here, that way the Binder can take care of creating the resource instances.

...main/java/org/springframework/boot/autoconfigure/context/MessageSourceAutoConfiguration.java Outdated

try {
if (StringUtils.hasText(properties.getCommonMessages())) {
PropertiesFactoryBean propertiesFactory = new PropertiesFactoryBean();
Copy link
Member

Choose a reason for hiding this comment

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

We might be better off using PropertiesLoaderUtils.fillProperties here rather than PropertiesFactoryBean. Using a FactoryBean isn't ideal since we need to call afterPropertiesSet etc.

...main/java/org/springframework/boot/autoconfigure/context/MessageSourceAutoConfiguration.java Outdated
.toList();
propertiesFactory.setLocations(commonResources.toArray(Resource[]::new));
propertiesFactory.setSingleton(true);
propertiesFactory.setIgnoreResourceNotFound(true);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should fail hard if the resource is not found.

@mmoayyed
updates after code review
7d70b39
@mmoayyed
Merge branch 'main' into message-source-common-messages
2aea08f 
# Conflicts:
#	spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/context/MessageSourceProperties.java
@mmoayyed
Copy link
Contributor Author

Thank you very much @philwebb . Updated PR based on your comments and resolved conflicts with upstream. Please resume.

@mmoayyed
Copy link
Contributor Author

Side note: I think it would be good to disable the Gradle configuration cache for the project explicitly (in case someone has it globally enabled). otherwise things like this show up:

* What went wrong:
Could not load the value of field `b` of `com.gradle.scan.plugin.internal.service.d` bean found in field `pluginServiceFactory` of `org.gradle.internal.enterprise.impl.DefaultGradleEnterprisePluginAdapter` bean found in Gradle runtime.
> Could not load the value of field `onAccess` of `org.gradle.internal.configuration.inputs.AccessTrackingEnvMap` bean found in field `env` of `io.spring.develocity.conventions.gradle.DevelocityConventionsPlugin$2` bean found in field `capturedArgs` of `java.lang.invoke.SerializedLambda` bean found in field `capturedArgs` of `java.lang.invoke.SerializedLambda` bean found in field `a` of `com.gradle.develocity.agent.gradle.internal.scan.BuildScanConfigurationInternal$1` bean found in field `a` of `com.gradle.scan.plugin.internal.service.a.e$a` bean found in field `a` of `com.gradle.scan.plugin.internal.service.a.g` bean found in field `m` of `com.gradle.scan.plugin.internal.service.a.e` bean found in field `m` of `com.gradle.scan.plugin.internal.service.n` bean found in field `a` of `com.gradle.scan.plugin.internal.service.PluginServiceFactory` bean found in field `b` of `com.gradle.scan.plugin.internal.service.d` bean found in field `pluginServiceFactory` of `org.gradle.internal.enterprise.impl.DefaultGradleEnterprisePluginAdapter` bean found in Gradle runtime.

...which can be bypassed with an explicit --no-configuration-cache parameter but of course are less than pleasant.

@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 16, 2024
@philwebb philwebb added this to the 3.4.0-RC1 milestone Oct 16, 2024
@philwebb philwebb self-assigned this Oct 16, 2024
philwebb pushed a commit that referenced this pull request Oct 16, 2024
@mmoayyed @philwebb
Allow common messages to be specified for message sources
573ccc5 
Extend message source configuration properties and auto-configuration to
support common messages.

See gh-42472
philwebb added a commit that referenced this pull request Oct 16, 2024
@philwebb
Polish 'Allow common messages to be specified for message sources'
06569af 
See gh-42472
@philwebb philwebb closed this in 4f96bea Oct 16, 2024
@philwebb
Copy link
Member

Thanks very much @mmoayyed, this has been merged to main.

philwebb added a commit that referenced this pull request Oct 16, 2024
@philwebb
Add @author attribution
5f84e78 
See gh-42472
@mmoayyed mmoayyed deleted the message-source-common-messages branch October 17, 2024 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philwebb philwebb philwebb left review comments

Assignees

@philwebb philwebb

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
3.4.0-RC1
Development

Successfully merging this pull request may close these issues.
3 participants
@mmoayyed @philwebb @spring-projects-issues