-
Notifications
You must be signed in to change notification settings - Fork 3.6k
HHH-19584 add @CurrentTimestamp(allowMutation) #10462
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
base: main
Are you sure you want to change the base?
HHH-19584 add @CurrentTimestamp(allowMutation) #10462
Conversation
@Override | ||
public boolean allowMutation() { | ||
return allowMutation; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yrodiere This is the only significant thing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks, but should we merge this immediately, or do you want someone to have a look at the "manually set the value for inserts" case too?
Asking because that might change the exposed API, if we decide its' better (simpler) to expose a single allowOverride()
attribute controlling whether people can manually set the value for both inserts and updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks, but should we merge this immediately, or do you want someone to have a look at the "manually set the value for inserts" case too?
Well, nobody actually asked for that yet. (Except you 😉)
Asking because that might change the exposed API, if we decide its' better (simpler) to expose a single
allowOverride()
attribute controlling whether people can manually set the value for both inserts and updates.
It's not linked directly to inserts vs updates, it depends on whether the client can override generation. So I think it's two different things.
All that said, we can't merge this without a test.
755f3ee
to
94349ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a new allowMutation
flag to the @CurrentTimestamp
annotation and its generator implementation, controlling whether application code can override generated timestamps outside of generation events.
- Introduces
allowMutation
attribute inCurrentTimestamp
API (default: false). - Propagates
allowMutation
throughCurrentTimestampAnnotation
andCurrentTimestampGeneration
. - Adds accessor in the generator for downstream use.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
hibernate-core/src/main/java/org/hibernate/generator/internal/CurrentTimestampGeneration.java | Added allowMutation field, constructor assignments, and allowMutation() method; refactored delegate lookup formatting. |
hibernate-core/src/main/java/org/hibernate/boot/models/annotations/internal/CurrentTimestampAnnotation.java | Added allowMutation field, constructors, getter, and setter. |
hibernate-core/src/main/java/org/hibernate/annotations/CurrentTimestamp.java | Extended annotation definition with allowMutation() and Javadoc. |
Comments suppressed due to low confidence (3)
hibernate-core/src/main/java/org/hibernate/generator/internal/CurrentTimestampGeneration.java:265
- [nitpick] Consider adding a Javadoc comment for the new allowMutation() method to explain its purpose and usage in the generator.
@Override
hibernate-core/src/main/java/org/hibernate/generator/internal/CurrentTimestampGeneration.java:82
- The new allowMutation feature should be covered by unit tests to verify its behavior across different event types and ensure regression safety.
private final boolean allowMutation;
hibernate-core/src/main/java/org/hibernate/generator/internal/CurrentTimestampGeneration.java:233
- [nitpick] The else block is redundant after a return; you can flatten this code by removing the else and decreasing nested indentation for better readability.
else {
[Please describe here what your change is about]
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.
https://hibernate.atlassian.net/browse/HHH-19584