Issue/datajdbc 557- Insert into table with only id column #1047

mipo256 · 2021-09-14T17:40:03Z

I tried hard and came up with the solution. Please, @schauder @mp911de - can you review this changes. I am very glad to hear your opinions. Thank you in advance :)

Closes #777

pivotal-cla · 2021-09-14T17:40:05Z

@mikhail2048 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-cla · 2021-09-14T17:41:59Z

@mikhail2048 Thank you for signing the Contributor License Agreement!

schauder

Thanks for your work.
This is going in the right direction.
I left a couple of comments for changes we need before we can merge this PR.

Let me know if you need help with these changes.
You'll have to touch the SQL rendering infrastructure, which can be intimidating.
I'm happy to point you in the right direction if necessary.

...resources/org.springframework.data.jdbc.core/JdbcAggregateTemplateIntegrationTests-mssql.sql

...esources/org.springframework.data.jdbc.core/JdbcAggregateTemplateIntegrationTests-oracle.sql

...t/resources/org.springframework.data.jdbc.core/JdbcAggregateTemplateIntegrationTests-db2.sql

...-data-relational/src/main/java/org/springframework/data/relational/core/dialect/Dialect.java

.../src/main/java/org/springframework/data/relational/core/dialect/InsertWithDefaultValues.java

schauder · 2021-10-20T13:02:23Z

...ta-relational/src/test/java/org/springframework/data/relational/degraph/DependencyTests.java

@@ -37,6 +38,7 @@ public void cycleFree() {
 				classpath() //
 						.noJars() //
 						.including("org.springframework.data.relational.**") //
+						.excluding("org.springframework.data.relational.core.dialect.**") //


Please do not change this test.
The change also should be superfluous once the .../core/sql no longer references Dialect.

Yeah, I agree with you, but, when I have applied changes in this PR and supply them with proper tests, this particular test method cause the build to fail. I will try it out again and if I will encounter the same behavior again, I will reach you out.

I have still append excluding("org.springframework.data.relational.core.sql.render.**") statement in DependencyTests#cycleFree. The reason is the SqlRenderer is in the org.springframework.data.relational.core.sql.render package, as well as InsertStatementVisitor which I utilize in my case. I mean, we seems to inevitably have to exclude the aforementioned package, have not we? So, I decided to make it so. If I am wrong form your point of view, then, please, feel free to correct me.

schauder · 2021-10-20T13:05:49Z

...ata-relational/src/main/java/org/springframework/data/relational/core/sql/DefaultInsert.java

@@ -77,4 +81,9 @@ public String toString() {

 		return builder.toString();
 	}
+
+	@Override
+	public Dialect getInsertDialect() {


Dialect should not be used within .../core/sql the point of classes in this package is to represent SQL statements in a dialect independent way.

Instead the SqlRenderer should use the Dialect to generate the dialect dependent SQL.

Ok, that's great, but I think SqlRenderer delegates the creation of SQL INSERT statment to InsertStatementVisitor. I think it will be great to use Dialect in SqlGenerator, because within InsertStatementVisitor we dealing only with an Insert, which (if I understood you correctly, sorry if not) should be dialect-independent - it would be quite complicated to extract Dialect depending logic from their.
I am sorry, I am kind of newbie here :) If I does not grasp something - feel free to correct me.

mipo256 · 2021-10-24T19:43:41Z

@schauder Please, check out my changes. Please, let me know what you think about this solution)

schauder

The important problems are:

SqlRenderer.render should not take a Dialect as an argument. Instead the necessary information should be part of the SqlRenderer in the first place.
The dependency circle is a no go. But when the rest works I can take care of this.

schauder · 2021-11-26T13:58:27Z

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java

@@ -675,7 +679,7 @@ private String render(Select select) {
 	}

 	private String render(Insert insert) {
-		return this.sqlRenderer.render(insert);
+		return this.sqlRenderer.render(insert, dialect);


This seem weird. The sqlRenderer already contains information from the Dialect. We should use that route to get the necessary information from the Dialect instead of passing it a second time on the call to render.

I agree, I have removed dialect as parameter and fell back to previous method version. See here

schauder · 2021-11-26T14:00:53Z

...test/java/org/springframework/data/jdbc/core/convert/DefaultDataAccessStrategyUnitTests.java

 	@BeforeEach
 	public void before() {

 		DelegatingDataAccessStrategy relationResolver = new DelegatingDataAccessStrategy();
-		Dialect dialect = HsqlDbDialect.INSTANCE;
+		dialect = HsqlDbDialect.INSTANCE;


If we move the dialect into a field we should also the initialisation with it.
But it would be easier to hard code the exact expected insert statement and leave the Dialect where it is.

Resolved with this

schauder · 2021-11-26T14:02:52Z

...ata-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/SqlGeneratorUnitTests.java


 		String insert = sqlGenerator.getInsert(emptySet());

-		assertThat(insert).endsWith("()");
+		assertThat(insert).endsWith(PostgresDialect.INSTANCE.getSqlInsertWithDefaultValues().getDefaultInsertPart());


Again, just hard code the expected String. That is much easier to understand.

Also: we should have a second test testing for some other variant of the behaviour using a diiferent dialect.

Resolved here

...-data-relational/src/main/java/org/springframework/data/relational/core/dialect/Dialect.java

schauder · 2021-11-26T14:10:14Z

...lational/src/main/java/org/springframework/data/relational/core/dialect/AbstractDialect.java

+	 * @return default implementation of InsertWithDefaultValues
+	 */
+	@Override
+	public InsertWithDefaultValues getSqlInsertWithDefaultValues() {


This Default implementation should go in the interface, so it works also for those that don't use the AbstractDialect

Resolved with this

schauder · 2021-11-26T14:16:27Z

...elational/src/main/java/org/springframework/data/relational/core/sql/render/SqlRenderer.java

@@ -69,16 +71,6 @@ public static String toString(Select select) {
 		return create().render(select);
 	}

-	/**


We should still have a way to render an Insert statement without dialect, falling back to sensible defaults.

…age and intriduced InsertDefaultValues enum

…Dialect info in RenderContext

mipo256 · 2021-11-27T15:57:55Z

@schauder I have removed dialect from sqlRenderer as a parameter - instead I injected the necessary information into DialectRenderContext, where, I suppose, it should logically be. Also I have resolved the dependency cycle issue. Also I have append ms-sql server unit test for sql generation. Please, check out my changes.

This broke in the past for some databases that do not support empty value lists. Closes #777 Original pull request #1047

schauder · 2021-12-06T09:57:59Z

This was already merged a few days ago. Thanks for your work.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 14, 2021

schauder requested changes Oct 20, 2021

View reviewed changes

mipo256 requested a review from schauder October 25, 2021 09:21

mipo256 mentioned this pull request Nov 24, 2021

Specify Oracle user during DML operations #1099

Closed

schauder requested changes Nov 26, 2021

View reviewed changes

mipo256 and others added 9 commits November 27, 2021 00:04

Add support for insert with default values

1e82233

Adjust test for insert with default values

e034798

Supply DATAJDBC-557 feature with Integration test

329454c

issue/DATAJDBC-557 supplied tests scripts with 'DROP TABLE' statements

23e38b1

issue/DATAJDBC-557 extraced Dialect dependency from .../core/sql pack…

320e4c3

…age and intriduced InsertDefaultValues enum

remove deialect as parameter in SqlRenderer method, instaed injected …

71204ad

…Dialect info in RenderContext

Supplied DATAJDBC-557 with MsSQL insert with default values test

1841135

Merge branch 'main' into issue/DATAJDBC-557

f613388

Make ../core/sql/** indenependent from dialect

079327a

mipo256 requested a review from schauder November 27, 2021 15:58

schauder pushed a commit that referenced this pull request Nov 29, 2021

Support inserts for id only entities.

7551cbd

This broke in the past for some databases that do not support empty value lists. Closes #777 Original pull request #1047

schauder closed this Dec 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/datajdbc 557- Insert into table with only id column #1047

Issue/datajdbc 557- Insert into table with only id column #1047

mipo256 commented Sep 14, 2021 •

edited by schauder

Loading

pivotal-cla commented Sep 14, 2021

pivotal-cla commented Sep 14, 2021

schauder left a comment

schauder Oct 20, 2021

mipo256 Oct 21, 2021

mipo256 Oct 23, 2021

schauder Oct 20, 2021

mipo256 Oct 23, 2021

mipo256 commented Oct 24, 2021

schauder left a comment

schauder Nov 26, 2021

mipo256 Nov 27, 2021

schauder Nov 26, 2021

mipo256 Nov 27, 2021

schauder Nov 26, 2021

mipo256 Nov 27, 2021

schauder Nov 26, 2021

mipo256 Nov 27, 2021

schauder Nov 26, 2021

mipo256 commented Nov 27, 2021 •

edited

Loading

schauder commented Dec 6, 2021

Issue/datajdbc 557- Insert into table with only id column #1047

Issue/datajdbc 557- Insert into table with only id column #1047

Conversation

mipo256 commented Sep 14, 2021 • edited by schauder Loading

pivotal-cla commented Sep 14, 2021

pivotal-cla commented Sep 14, 2021

schauder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mipo256 commented Oct 24, 2021

schauder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mipo256 commented Nov 27, 2021 • edited Loading

schauder commented Dec 6, 2021

mipo256 commented Sep 14, 2021 •

edited by schauder

Loading

mipo256 commented Nov 27, 2021 •

edited

Loading