Skip to content

Added Sequence generation support #1955

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

Closed
wants to merge 1 commit into from

Conversation

mipo256
Copy link
Contributor

@mipo256 mipo256 commented Dec 9, 2024

Hey @schauder, @mp911de!

Hope you're doing great, I've redone the PR #1922 to make it work via a custom callback that is registered as a bean in AbstractJdbcConfiguration. Let me know what you think of that, thanks!

CC: Targeting issue #1923

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 9, 2024
@mp911de
Copy link
Member

mp911de commented Dec 9, 2024

Hey, great to see a new contribution.

We discussed identifier generation a while ago and wanted explore pathways forward. IdentifierGenerator is a rather specific approach and hard to type (identifiers could be numbers, UUIDs, some random characters). We could explore a ValueGenerator, maybe even ValueGenerator<T> abstraction that is implemented by a sequence generator along with a @GeneratedValue(strategy = SEQUENCE | NATIVE) (NATIVE is what Statement.getGeneratedKeys returns) annotation that could be used on a per-property level.

The implemented SequenceValueGenerator<T extends Number> extends ValueGenerator<T> should encapsulate JdbcTemplate along with the SQL text to obtain the value.

@mipo256
Copy link
Contributor Author

mipo256 commented Dec 9, 2024

@mp911de So, basically, what we want to achieve is the following:

@Table
public class MyEntity {
    
    @Id
    @GeneratedValue(strategy = SEQUENCE)
    private Long id;
    
    @GeneratedValue(strategy = SEQUENCE)
    private Long version;
    
    @GeneratedValue(strategy = NATIVE)
    private Long someValue;
} 

Where we support sequence generation for various fields, along with generation via identitiy columns in database via NATIVE placeholder, am I correct?

@mp911de
Copy link
Member

mp911de commented Dec 10, 2024

While not necessarily generating version (implied; as it would contradict our @Version handling) that is the underlying idea. Paging @schauder for visibility.

In the long run, I could also imagine some @ValueGenerator(UUIDValueGenerator.class) usage but that is a future idea.

/**
* The id should be dervied from the database sequence
*/
SEQUENCE;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why we need this.

@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 13, 2025
@mipo256 mipo256 requested a review from schauder January 17, 2025 07:32
@mipo256 mipo256 force-pushed the GH-1923 branch 4 times, most recently from c24faba to 1a5dfeb Compare January 17, 2025 07:37
@mipo256
Copy link
Contributor Author

mipo256 commented Jan 17, 2025

Hey @schauder. I've fixed most of the comments you left, apart from a single comment. Please, read my explanation. Apart from this - everything is fixed.

schauder pushed a commit that referenced this pull request Feb 5, 2025
Ids can be annotated with @sequence to specify a sequence to pull id values from.

Closes #1923
Original pull request #1955

Signed-off-by: mipo256 <[email protected]>

Some accidential changes removed.
Signed-off-by: schauder <[email protected]>
schauder added a commit that referenced this pull request Feb 5, 2025
Reference issues in tests comments.
Removed `DisabledOnDatabase`
IdGeneration default methods related to sequence generation are now internally consistent.
Formatting and naming.
IdGeneration offers simple support by default.
Fix exception in oracle integration test setup
Use SqlIdentifier for sequence names
Remove SEQUENCE id source
Added documentation

See #1923
Original pull request #1955
@schauder
Copy link
Contributor

schauder commented Feb 5, 2025

Thanks, this is merged

@mp911de
Copy link
Member

mp911de commented Apr 9, 2025

@mipo256 with the infrastructure in place it would make sense to have a R2DBC-variant for @Sequence support. Would you want to contribute that one for the R2DBC part?

@mipo256
Copy link
Contributor Author

mipo256 commented Apr 9, 2025

Of course, @mp911de . Can you create an issue for that?

@mp911de
Copy link
Member

mp911de commented Apr 9, 2025

Just send in a PR right away. For things that don't require extra discussion upfront, we happily accept just a PR right away, without the need to create another ticket.

@mipo256
Copy link
Contributor Author

mipo256 commented Apr 9, 2025

Okay @mp911de , got it, I'll be right back :)

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

Successfully merging this pull request may close these issues.

4 participants