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 #1955

Open
wants to merge 1 commit into
base: main
Choose a base branch
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.

@@ -299,6 +299,37 @@
</plugins>
</build>
</profile>
<profile>
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 think we need an additional Maven profile.

.map(insertSubject -> sqlParametersFactory.forInsert(insertSubject.getInstance(), domainType,
insertSubject.getIdentifier(), idValueSource))
.toArray(SqlIdentifierParameterSource[]::new);
.map(insertSubject -> sqlParametersFactory.forInsert(
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of formatting is not stable under the formatter used for the project. You'll need to use // at the line end to prevent the formatter to collapse it into a single line.

@@ -47,9 +49,6 @@ public class SqlParametersFactory {
private final RelationalMappingContext context;
private final JdbcConverter converter;

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you removing this?

@@ -135,6 +138,29 @@ public void savesAnEntity() {
"id_Prop = " + entity.getIdProp())).isEqualTo(1);
}

@Test
@DisabledOnDatabase(database = DatabaseType.MYSQL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use TestDatabaseFeatures instead so it becomes clear why certain tests are excluded for certain databases.

/**
* 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants