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

Added Sequence generation support #1922

Closed
wants to merge 1 commit into from

Conversation

mipo256
Copy link
Contributor

@mipo256 mipo256 commented Oct 26, 2024

At the last conference that I participated in, there were a lot of feedback I've received about spring-data-jdbc, most notably, the things that they wish were there. One of them was id generation via sequence. So I've added this solution. Frankly, there might be an optimization for the batch insert to query the N next elements out of sequence at one go, but I think we can take care of that later, since the PR is already quite large.

Worth to mention that the id generation from sequence won't be possible for MySQL Dialect since there is no native support for it in place

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 26, 2024
@mipo256 mipo256 force-pushed the feature/GH-sequence branch 2 times, most recently from adc328e to cfee29e Compare October 26, 2024 18:24
@mp911de mp911de added status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 28, 2024
@schauder
Copy link
Contributor

A significant change like this should start with an issue instead of a PR, so we can discuss the goals we want to achieve with this.

I don't like the current approach. It only looks at sequences with no thought given to other ways to create Ids: (UUID, HiLo, and so on).
It also creates a completely new code path just for this, although the existing code path, of using CallBacks for this is completely sufficient.

And finally we should also at least think about how we might be able to support value generation in general, not just for Ids and not only for JDBC but also R2DBC and other Spring Data modules.

I'll therefore going to reject this PR.
Feel free to create an issue with a suggestion of an implementation, that we then can discuss.

@schauder schauder closed this Oct 28, 2024
@mipo256
Copy link
Contributor Author

mipo256 commented Oct 28, 2024

Okay, I'll file an issue, but

It also creates a completely new code path just for this, although the existing code path, of using CallBacks for this is completely sufficient.

I do not understand what is actually meant by "new code path". This is just a new IdValueSource entry and processing for it, so what exactly is a new path - I do not understand, can you please elaborate a bit more?

The Callbacks are NOT sufficient, writing the custom callbacks logic for generation of Ids in every app is exactly the reason it is tedious.

@mipo256
Copy link
Contributor Author

mipo256 commented Oct 28, 2024

And finally we should also at least think about how we might be able to support value generation in general, not just for Ids and not only for JDBC but also R2DBC and other Spring Data modules.

The support for R2DBC is going to be absolutely the same, I've not included this in PR for the reasons of not making it 1000 lines of code PR.

@schauder
Copy link
Contributor

The Callbacks are NOT sufficient, writing the custom callbacks logic for generation of Ids in every app is exactly the reason it is tedious.

Yes, it is tedious, but it works just fine. Anything we build to make that less tedious should utilise Callbacks, or explain why it can't or shouldn't be done.

I do not understand what is actually meant by "new code path". This is just a new IdValueSource entry and processing for it, so what exactly is a new path - I do not understand, can you please elaborate a bit more?

This is what I mean by a new code path: https://github.com/spring-projects/spring-data-relational/pull/1922/files#diff-6adeda6c2cf507ae7f66b7517d5c23d0d0186adaef23e3770a36ab943e485c9cR116
It puts additional stuff in the main persistence code path, without any abstraction. It works only for sequences and is different from the existing code path for this kind of thing: Callbacks.

@schauder
Copy link
Contributor

The support for R2DBC is going to be absolutely the same, I've not included this in PR for the reasons of not making it 1000 lines of code PR.

Not including it is a good choice. But it should be clear how it fits into the picture.

@mipo256
Copy link
Contributor Author

mipo256 commented Oct 28, 2024

Okay, let's build it via callbacks, but I really hope that we can agree at least on the fact that the solution via callbacks is tedious

I've filled the #1923

Let's do it this way - I'll rewrite the implementation to work over the callbacks, and I'll re-open the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants