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

Allow @DefaultValue to be used on record components #29010

Closed
wants to merge 10 commits into from

Conversation

An1s9n
Copy link
Contributor

@An1s9n An1s9n commented Dec 14, 2021

It would be useful to have ability to use @DefaultValue annotation with records and Lombok's auto-generated constructors. I've made small code changes in spring-boot, which provide such an ability.

I've created a sample project which shows desired usage, please see source code and test in this public repo: https://github.com/An1s9n/spring-boot-sample.

In case of any questions, feel free to contact me, preferred (and fastest as well) way is Telegram: https://t.me/An1s9n.

@pivotal-cla
Copy link

@An1s9n Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@An1s9n Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 14, 2021
@mbhave mbhave added the for: team-meeting An issue we'd like to discuss as a team to make progress label Dec 14, 2021
@An1s9n
Copy link
Contributor Author

An1s9n commented Dec 27, 2021

Hi! How often do you organise team meetings? :)

@bclozel
Copy link
Member

bclozel commented Jan 5, 2022

@An1s9n we've just discussed this in a team meeting.

While we think it could be an interesting addition for Lombok users, we don't think that adding field+method targets for this annotation is worth it, since it will let most Java users in a position where they can use it in an invalid arrangement.

The record component target is valuable for all Spring Boot users and we'd like to implement this. Would you like to amend your PR with the following?

  • only target the record component variant
  • add unit tests for this case. Note that since Spring Boot 2.x is built with Java 8, we can't have records in our test source code, so we're executing conditional tests for Java 16+ and writing source classes compiled on the fly (you can see that arrangement in action in 681df90).

Since this requires more work here and this won't address the Lombok use case, feel free to let us know if you don't feel like tackling this issue.

Thanks!

@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue and removed for: team-meeting An issue we'd like to discuss as a team to make progress labels Jan 5, 2022
@An1s9n
Copy link
Contributor Author

An1s9n commented Jan 5, 2022

@bclozel, hi! Yes, I'd like to amend my PR as you described, because I want to contribute to Spring Boot hah :)) I'll try to do it asap.

@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 Jan 5, 2022
@An1s9n
Copy link
Contributor Author

An1s9n commented Jan 7, 2022

@bclozel, I've done what you've asked me for :) Let me know if something else is required - I'll fix it

@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Jan 10, 2022
@philwebb philwebb added this to the 2.7.x milestone Jan 10, 2022
@philwebb philwebb changed the title Improved @DefaultValue Allow @DefaultValue to be used on record components Jan 10, 2022
@snicoll
Copy link
Member

snicoll commented Jan 11, 2022

I am not sure that the annotation processor would handle such use of @DefaultValue out-of-the-box. We should give that a try and create a separate issue if needed.

@An1s9n
Copy link
Contributor Author

An1s9n commented Jan 11, 2022

@snicoll, you mean meta-data generation, am I right?

@An1s9n
Copy link
Contributor Author

An1s9n commented Jan 12, 2022

@snicoll, luckily it works just perfect without any additional changes! :) I've added a unit test for such a case.

@wilkinsona
Copy link
Member

This is targeted at 2.7.x where we compile using Java 8 but ElementType.RECORD_COMPONENT isn't available in Java 8. We need to decide if we're happy to wait till 3.0 to ship this enhancement, or if we want to build and ship a multi-release jar for users on Java 17. #25090 is related to the latter option.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Feb 14, 2022
@philwebb
Copy link
Member

I think we should move this to 3.0 given the amount of work it will take to get our build to produce a multi-release jar.

@wilkinsona wilkinsona removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Feb 18, 2022
@wilkinsona wilkinsona modified the milestones: 2.7.x, 3.0.x Feb 18, 2022
@wilkinsona wilkinsona self-assigned this Mar 4, 2022
@wilkinsona wilkinsona modified the milestones: 3.0.x, 3.0.0-M2 Mar 4, 2022
wilkinsona pushed a commit that referenced this pull request Mar 4, 2022
@wilkinsona wilkinsona closed this in a1ee9c4 Mar 4, 2022
@wilkinsona
Copy link
Member

Thanks very much, @An1s9n.

@wilkinsona
Copy link
Member

Since merging these changes, we've learned that they weren't actually necessary. The existing PARAMETER target on @DefaultValue means that @DefaultValue is already propagated from the record component to its corresponding constructor parameters. It's picked up from there by both the binder and the configuration metadata annotation processor. The change made in this issue just means that @DefaultValue will be propagated to the record component itself and available from there using reflection. That isn't necessary for our purposes but it probably doesn't do any harm. I'll flag this for discussion at a forthcoming team meeting so that we can decide if we want to keep the RECORD_COMPONENT target or revert it.

@wilkinsona wilkinsona added type: task A general task and removed type: enhancement A general enhancement labels Mar 29, 2022
@wilkinsona
Copy link
Member

#30460 has updated the docs to describe our record support with @DefaultValue and backport some tests to verify that it works as the docs describe.

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

Successfully merging this pull request may close these issues.

None yet

8 participants