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

Harmonize configuration properties that accept a comma-separated list of values #42478

Closed
snicoll opened this issue Sep 30, 2024 · 5 comments
Closed
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Sep 30, 2024

Looking at #42472, I found odd that we'd use a String for a property that's declared as a "comma-separated list of". Then I noticed that MessageSourceProperties has the same pattern for the basename property.

Looking at our code base, most of these properties are a List or an array, with a few exceptions. I am wondering if we shouldn't harmonize this across the code base so that it's always a collection.

@snicoll snicoll added status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress for: team-attention An issue we'd like other members of the team to review and removed for: team-meeting An issue we'd like to discuss as a team to make progress labels Sep 30, 2024
@wilkinsona
Copy link
Member

I had the same thought when briefly looking at #42472.

Consistently using List or similar sounds good to me, particularly where the target of the property is a collection-like type and we're having to do something like StringUtils.commaDelimitedListToStringArray when setting it.

@philwebb
Copy link
Member

I think this code probably pre-dates the binder rewrite so +1 to moving to a List<String>. That will also allow the YAML list syntax to work.

@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 30, 2024
@philwebb philwebb added this to the 3.4.x milestone Sep 30, 2024
@snicoll snicoll removed the for: team-attention An issue we'd like other members of the team to review label Sep 30, 2024
@wilkinsona wilkinsona self-assigned this Oct 2, 2024
@wilkinsona
Copy link
Member

As part of this, I'm going to revise the property descriptions to just say "list" rather than "comma-separated list". My hope is that this will make it a bit more clear that YAML's list syntax can be used.

@wilkinsona wilkinsona modified the milestones: 3.4.x, 3.4.0-RC1 Oct 2, 2024
@defaultbranch
Copy link

defaultbranch commented Nov 29, 2024

Thanks guys for keeping improving Spring/Spring Boot! This change sadly was not fully explained in Spring Boot 3.4 Release Notes.

Looking at commit fae3cd1 I understand why my previous setup fails with SpringBoot 3.4.0:

    private SpringLiquibase springLiquibase(DataSource dataSource, LiquibaseProperties properties) {
        var liquibase = new SpringLiquibase();
        liquibase.setDataSource(dataSource); 
        // ^--- the one thing I want to change, everything below is just keeping the default
        liquibase.setChangeLog(properties.getChangeLog());
        liquibase.setContexts(properties.getContexts());
        // ^--- now an ERROR; need to convert list to comma-sep string now
        liquibase.setDefaultSchema(properties.getDefaultSchema());
        liquibase.setDropFirst(properties.isDropFirst());
        liquibase.setShouldRun(properties.isEnabled());
        liquibase.setLabelFilter(properties.getLabelFilter());
        // ^--- now an ERROR; need to convert list to comma-sep string now
        liquibase.setChangeLogParameters(properties.getParameters());
        liquibase.setRollbackFile(properties.getRollbackFile());
        return liquibase;
    }

But maybe my way of setting up Liquibase is odd, so I guess I should instead use a Customizer<Liquibase> bean, mentioned in the Spring Boot 3.4 Release Notes...

@wilkinsona
Copy link
Member

We don't consider the getters and setters on Boot's @ConfigurationProperties classes to be public API. This is noted in the documentation:

The properties that map to @ConfigurationProperties classes available in Spring Boot, which are configured through properties files, YAML files, environment variables, and other mechanisms, are public API but the accessors (getters/setters) of the class itself are not meant to be used directly.

But maybe my way of setting up Liquibase is odd, so I guess I should instead use a Customizer<Liquibase> bean, mentioned in the Spring Boot 3.4 Release Notes...

A Customizer<Liquibase> won't meet your needs as the DataSource needs to be set on the SpringLiquibase instance not the Liquibase instance that it creates. You can use @LiquibaseDataSource to define a DataSource @Bean bean that will be used by Liquibase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants