-
Notifications
You must be signed in to change notification settings - Fork 350
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
base: main
Are you sure you want to change the base?
Conversation
Hey, great to see a new contribution. We discussed identifier generation a while ago and wanted explore pathways forward. The implemented |
@mp911de So, basically, what we want to achieve is the following:
Where we support sequence generation for various fields, along with generation via identitiy columns in database via |
While not necessarily generating In the long run, I could also imagine some |
@@ -299,6 +299,37 @@ | |||
</plugins> | |||
</build> | |||
</profile> | |||
<profile> |
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.
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( |
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.
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; | |||
|
|||
/** |
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.
Why are you removing this?
@@ -135,6 +138,29 @@ public void savesAnEntity() { | |||
"id_Prop = " + entity.getIdProp())).isEqualTo(1); | |||
} | |||
|
|||
@Test | |||
@DisabledOnDatabase(database = DatabaseType.MYSQL) |
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.
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; |
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.
I don't see why we need this.
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