Skip to content

Allow specifying unit type of configuration property when injected via constructor #21746

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

Conversation

encircled
Copy link
Contributor

Allow specifying unit type of configuration property when injected via constructor #20892

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 7, 2020
@encircled encircled force-pushed the config-props-units-via-constructor branch from 65a6adb to a428fd6 Compare June 7, 2020 20:26
…a constructor spring-projects#20892

Allow specifying unit type of configuration property when injected via constructor spring-projects#20892

...

...
@encircled encircled force-pushed the config-props-units-via-constructor branch from a428fd6 to 6d41a69 Compare June 7, 2020 21:19
@mbhave mbhave added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 11, 2020
@mbhave mbhave added this to the 2.4.x milestone Jun 11, 2020
@snicoll
Copy link
Member

snicoll commented Jun 12, 2020

Before merging this we should double check that the metadata is generated with the correct unit.

@wilkinsona
Copy link
Member

The unit seems to be missing from the metadata. For example:

{
  "name": "test.size",
  "type": "org.springframework.util.unit.DataSize",
  "sourceType": "com.example.demo.ConstructorParameterWithConversionProperties",
  "defaultValue": "3"
}

The above is from a properties class based on the one that's added to the tests in this PR. The constructor argument is declared as @DefaultValue("3") @DataSizeUnit(DataUnit.MEGABYTES) DataSize size so I believe the default value should be 3MB.

@wilkinsona wilkinsona added the for: merge-with-amendments Needs some changes when we merge label Jul 1, 2020
@snicoll
Copy link
Member

snicoll commented Jul 6, 2020

I would have expected that indeed. We need to update this support to lookup the unit annotation on others places.

@snicoll
Copy link
Member

snicoll commented Jul 7, 2020

Looking at the metadata and how values are extracted, this applies using the code that's written for the field initialization something like, Duration someProperty = Duration.ofHours(40);. We can extract 40h from that. If we compare with the constructor based approach, we should provide the value as-is IMO as @DefaultValue is the text-based default value of the property. I think it should be @DefaultValue("3MB") @DataSizeUnit(DataUnit.MEGABYTES) rather than @DefaultValue("3") @DataSizeUnit(DataUnit.MEGABYTES).

Allowing the unit on the constructor parameter makes such construct possible so it'd be legitimate for users to expect the other variant to work. It means that the annotation processor has to look for those 3 additional annotations too and handle it only if the default value is an integer. I wonder if we want that extra complexity.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Jul 7, 2020
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Jul 8, 2020
@snicoll
Copy link
Member

snicoll commented Jul 8, 2020

We've discussed this and concluded that having 3 in the metadata is acceptable considering that using 3 manually would lead to the expected result (3MB). To provide better metadata, the unit should ideally be specified again in @DefaultValue so that the generated default value in the metadata is a bit more explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants