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

Fix section about required setters for Collections in "Type-safe Configuration Properties" #43138

Open
mhalbritter opened this issue Nov 13, 2024 · 8 comments
Labels
type: documentation A documentation update
Milestone

Comments

@mhalbritter
Copy link
Contributor

Our documentations says here:

Collections and arrays can be accessed either through an index (typically with YAML) or by using a single comma-separated value (properties). In the latter case, a setter is mandatory. We recommend to always add a setter for such types. If you initialize a collection, make sure it is not immutable (as in the preceding example).

Which isn't true for collections.

Binding this:

@ConfigurationProperties("list")
public class ListConfigProps {
    private final List<String> list = new ArrayList<>();

    public List<String> getList() {
        return this.list;
    }
}

totally works fine, with those properties:

list.list[0]=a
list.list[1]=b

and those:

list.list=a,b

Same works for Set.

For arrays, the paragraph in the documentation holds true.

@mhalbritter mhalbritter added the type: documentation A documentation update label Nov 13, 2024
@mhalbritter mhalbritter added this to the 3.2.x milestone Nov 13, 2024
@mhalbritter
Copy link
Contributor Author

@snicoll
Copy link
Member

snicoll commented Nov 15, 2024

I thought the setter was mandatory for setting the property in one go using comma separated values. Perhaps the binder has changed to not require that anymore?

@philwebb
Copy link
Member

It'll call the setter if it has one, but it doesn't need one as it'll mutate the existing List, Set etc. You do need a setter for array types.

@wickdynex
Copy link
Contributor

wickdynex commented Nov 15, 2024

maybe we should modify this doc, right?
The Collections class needn't setter() method, but Arrays needs only by using a single comma-separated value. Hope I understand it correctly🥺.

Before

  • Collections and arrays can be accessed either through an index (typically with YAML) or by using a single comma-separated value (properties). In the latter case, a setter is mandatory. We recommend to always add a setter for such types. If you initialize a collection, make sure it is not immutable (as in the preceding example).

After

Version1

  • Collections can be accessed either through an index (typically with YAML) or by using a single comma-separated value (properties). If you initialize a collection, make sure it is not immutable (as in the preceding example).
  • Arrays can be accessed either through an index (typically with YAML) or by using a single comma-separated value (properties). But in the latter case, a setter is mandatory. We recommend to always add a setter for such types.

I split Collections and Arrays into two part, but some parts of the content might overlapp.

Version 2

  • Collections and arrays can be accessed either through an index (typically with YAML) or by using a single comma-separated value (properties). If you initialize a collection, make sure it is not immutable (as in the preceding example). For arrays, a setter is mandatory when using comma-separated values, and we recommend always adding a setter for such types.

Just modify on the original version, but fix the error that Collections needn't setter().

Do I fix the section correctly🤔 ? If so, could u plz assign this issue to me, and I'll make a PR to modify the doc🥰?

@mhalbritter
Copy link
Contributor Author

Hey @wickdynex, thanks for your initiative. We're currently looking for the exact cause why this sentence is in our docs - maybe the binder implementation changed in the past. We want to make sure that we don't miss any corner case and state incorrect facts in our documentation.

I'll chime back in when we're done with this.

Arrays can be accessed either through an index (typically with YAML) or by using a single comma-separated value (properties). But in the latter case, a setter is mandatory. We recommend to always add a setter for such types.

I think for arrays the setter is always necessary. You can double check, if you like.

I like version 2 more.

@wickdynex
Copy link
Contributor

Hey @wickdynex, thanks for your initiative. We're currently looking for the exact cause why this sentence is in our docs - maybe the binder implementation changed in the past. We want to make sure that we don't miss any corner case and state incorrect facts in our documentation.

I'll chime back in when we're done with this.

Arrays can be accessed either through an index (typically with YAML) or by using a single comma-separated value (properties). But in the latter case, a setter is mandatory. We recommend to always add a setter for such types.

I think for arrays the setter is always necessary. You can double check, if you like.

I like version 2 more.

Okey, it seems that your team need to figure out why doc differs from the actual behavior of the code. Hope everything goes smoothly~ Until you and your team fix the binder bugs even change the feature, can we have the chance to modify the wrong section? Im I right?🤔

@mhalbritter
Copy link
Contributor Author

No we can't modify the section until we have figured out why the doc states that setters for collections are necessary. It may be that the doc is wrong or it may be that I'm wrong and I have missed an edge case in my testing which makes the setters necessary for collections.

@wickdynex
Copy link
Contributor

No we can't modify the section until we have figured out why the doc states that setters for collections are necessary. It may be that the doc is wrong or it may be that I'm wrong and I have missed an edge case in my testing which makes the setters necessary for collections.

Yeah, before making changes, we should conduct thorough tests to identify the bugs and determine which ones are truly problematic👍.

@philwebb philwebb modified the milestones: 3.2.x, 3.3.x Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation A documentation update
Projects
None yet
Development

No branches or pull requests

4 participants