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

Make the name member of @XxxxGenerator annotations optional #416

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

gavinking
Copy link
Contributor

See #406.

@lukasj lukasj linked an issue Aug 4, 2023 that may be closed by this pull request
Copy link
Contributor

@lukasj lukasj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the deafult name should be defined as: SEQUENCE_ENTITYNAME for the class level use annotation and SEQUENCE_ENTITYNAME_FIELDNAME for the field level use of the annotation

@lukasj
Copy link
Contributor

lukasj commented Aug 4, 2023

add a line to appendixes.adoc/changelog

@gavinking
Copy link
Contributor Author

gavinking commented Aug 7, 2023

the deafult name should be defined as: SEQUENCE_ENTITYNAME for the class level use annotation and SEQUENCE_ENTITYNAME_FIELDNAME for the field level use of the annotation

@lukasj after reviewing this and re-thinking it through properly this time, I realized that what we talked about isn't quite right, and I actually got it right the first time.

We need the generator name inferred for @GeneratedValue to line up with the name inferred for a @SequenceGenerator or @TableGenerator. Since the @GeneratedValue doesn't know what kind of generator it's looking for, nor where it's declared, the plain entity name is the right thing to use here.

If we want to nail down the actual inferred name of the database sequence or table when sequenceName or table is missing, that's something we could do, but it's an orthogonal question, since the spec currently doesn't constrain that at all. (It just says "Defaults to a name chosen by persistence provider".)

So I think we should merge this as-is.

Sorry for the confusion.

@gavinking
Copy link
Contributor Author

add a line to appendixes.adoc/changelog

Done.

@lukasj lukasj merged commit 4daf494 into jakartaee:master Aug 8, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"entity local" @SequenceGenerator/@TableGenerator
2 participants