From 5b6bbc1f9d4ce9d4d0b7a0ee6e3aa02e4c0b4751 Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Thu, 21 Oct 2021 07:09:39 +0200 Subject: [PATCH 1/3] 1073-duplicate-select - Prepare branch --- pom.xml | 2 +- spring-data-jdbc-distribution/pom.xml | 2 +- spring-data-jdbc/pom.xml | 4 ++-- spring-data-relational/pom.xml | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pom.xml b/pom.xml index eeaa0b9e93..a3cf6d2eaf 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-relational-parent - 2.3.0-SNAPSHOT + 2.3.0-1073-duplicate-select-SNAPSHOT pom Spring Data Relational Parent diff --git a/spring-data-jdbc-distribution/pom.xml b/spring-data-jdbc-distribution/pom.xml index 03d6a5c2a0..46ca8ac46a 100644 --- a/spring-data-jdbc-distribution/pom.xml +++ b/spring-data-jdbc-distribution/pom.xml @@ -14,7 +14,7 @@ org.springframework.data spring-data-relational-parent - 2.3.0-SNAPSHOT + 2.3.0-1073-duplicate-select-SNAPSHOT ../pom.xml diff --git a/spring-data-jdbc/pom.xml b/spring-data-jdbc/pom.xml index af9ad0904e..0261c82051 100644 --- a/spring-data-jdbc/pom.xml +++ b/spring-data-jdbc/pom.xml @@ -6,7 +6,7 @@ 4.0.0 spring-data-jdbc - 2.3.0-SNAPSHOT + 2.3.0-1073-duplicate-select-SNAPSHOT Spring Data JDBC Spring Data module for JDBC repositories. @@ -15,7 +15,7 @@ org.springframework.data spring-data-relational-parent - 2.3.0-SNAPSHOT + 2.3.0-1073-duplicate-select-SNAPSHOT diff --git a/spring-data-relational/pom.xml b/spring-data-relational/pom.xml index 4e42a006ec..8befbd5164 100644 --- a/spring-data-relational/pom.xml +++ b/spring-data-relational/pom.xml @@ -6,7 +6,7 @@ 4.0.0 spring-data-relational - 2.3.0-SNAPSHOT + 2.3.0-1073-duplicate-select-SNAPSHOT Spring Data Relational Spring Data Relational support @@ -14,7 +14,7 @@ org.springframework.data spring-data-relational-parent - 2.3.0-SNAPSHOT + 2.3.0-1073-duplicate-select-SNAPSHOT From 7a313360dab3d2b09f619918339216396a7eaf3d Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Thu, 21 Oct 2021 15:39:36 +0200 Subject: [PATCH 2/3] Avoid duplicate selection of columns. When the key column of a MappedCollection is also present in the contained entity that column got select twice. We now check if the column already gets selected before adding it to the selection. This only works properly if the column names derived for the entity property and the key column match exactly, which might require use of a `@Column` annotation depending on the used NamingStrategy. Closes #1073 --- .../data/jdbc/core/convert/SqlGenerator.java | 24 +++++----- .../core/convert/SqlGeneratorUnitTests.java | 19 +++++++- .../relational/core/sql/AbstractSegment.java | 27 ++++++----- .../data/relational/core/sql/Column.java | 44 +++++++++++++++++ .../data/relational/core/sql/Table.java | 47 ++++++++++++++++++- 5 files changed, 134 insertions(+), 27 deletions(-) diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java index 06bfaf0a52..93f47af8da 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java @@ -381,11 +381,11 @@ private String createAcquireLockById(LockMode lockMode) { Table table = this.getTable(); Select select = StatementBuilder // - .select(getIdColumn()) // - .from(table) // - .where(getIdColumn().isEqualTo(getBindMarker(ID_SQL_PARAMETER))) // - .lock(lockMode) // - .build(); + .select(getIdColumn()) // + .from(table) // + .where(getIdColumn().isEqualTo(getBindMarker(ID_SQL_PARAMETER))) // + .lock(lockMode) // + .build(); return render(select); } @@ -395,10 +395,10 @@ private String createAcquireLockAll(LockMode lockMode) { Table table = this.getTable(); Select select = StatementBuilder // - .select(getIdColumn()) // - .from(table) // - .lock(lockMode) // - .build(); + .select(getIdColumn()) // + .from(table) // + .lock(lockMode) // + .build(); return render(select); } @@ -727,9 +727,9 @@ static final class Join { Join(Table joinTable, Column joinColumn, Column parentId) { - Assert.notNull( joinTable,"JoinTable must not be null."); - Assert.notNull( joinColumn,"JoinColumn must not be null."); - Assert.notNull( parentId,"ParentId must not be null."); + Assert.notNull(joinTable, "JoinTable must not be null."); + Assert.notNull(joinColumn, "JoinColumn must not be null."); + Assert.notNull(parentId, "ParentId must not be null."); this.joinTable = joinTable; this.joinColumn = joinColumn; diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/SqlGeneratorUnitTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/SqlGeneratorUnitTests.java index d6e89bc745..8c0fd53be6 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/SqlGeneratorUnitTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/SqlGeneratorUnitTests.java @@ -23,7 +23,6 @@ import java.util.Map; import java.util.Set; -import org.assertj.core.api.SoftAssertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.data.annotation.Id; @@ -374,6 +373,23 @@ public void findAllByPropertyWithKeyOrdered() { + "WHERE dummy_entity.backref = :backref " + "ORDER BY key-column"); } + @Test // GH-1073 + public void findAllByPropertyAvoidsDuplicateColumns() { + + final SqlGenerator sqlGenerator = createSqlGenerator(ReferencedEntity.class); + final String sql = sqlGenerator.getFindAllByProperty( + Identifier.of(quoted("id"), "parent-id-value", DummyEntity.class), // + quoted("X_L1ID"), // this key column collides with the name derived by the naming strategy for the id of + // ReferencedEntity. + false); + + final String id = "referenced_entity.x_l1id AS x_l1id"; + assertThat(sql.indexOf(id)) // + .describedAs(sql) // + .isEqualTo(sql.lastIndexOf(id)); + + } + @Test // DATAJDBC-219 public void updateWithVersion() { @@ -709,6 +725,7 @@ static class DummyEntity { Set elements; Map mappedElements; AggregateReference other; + Map mappedReference; } static class VersionedEntity extends DummyEntity { diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/AbstractSegment.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/AbstractSegment.java index 7f567ca627..3e095dc7b6 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/AbstractSegment.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/AbstractSegment.java @@ -15,12 +15,15 @@ */ package org.springframework.data.relational.core.sql; +import java.util.Arrays; + import org.springframework.util.Assert; /** * Abstract implementation to support {@link Segment} implementations. * * @author Mark Paluch + * @author Jens Schauder * @since 1.1 */ abstract class AbstractSegment implements Segment { @@ -47,21 +50,21 @@ public void visit(Visitor visitor) { visitor.leave(this); } - /* - * (non-Javadoc) - * @see java.lang.Object#hashCode() - */ @Override - public int hashCode() { - return toString().hashCode(); + public boolean equals(Object o) { + + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + AbstractSegment that = (AbstractSegment) o; + return Arrays.equals(children, that.children); } - /* - * (non-Javadoc) - * @see java.lang.Object#equals(java.lang.Object) - */ @Override - public boolean equals(Object obj) { - return obj instanceof Segment && toString().equals(obj.toString()); + public int hashCode() { + return Arrays.hashCode(children); } } diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/Column.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/Column.java index 71f3ab2d5f..e536d1dca3 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/Column.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/Column.java @@ -15,6 +15,8 @@ */ package org.springframework.data.relational.core.sql; +import java.util.Objects; + import org.springframework.lang.Nullable; import org.springframework.util.Assert; @@ -364,6 +366,27 @@ String getPrefix() { return prefix; } + @Override + public boolean equals(Object o) { + + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + if (!super.equals(o)) { + return false; + } + Column column = (Column) o; + return name.equals(column.name) && table.equals(column.table); + } + + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), name, table); + } + /** * {@link Aliased} {@link Column} implementation. */ @@ -419,5 +442,26 @@ public Column from(Table table) { public String toString() { return getPrefix() + getName() + " AS " + getAlias(); } + + @Override + public boolean equals(Object o) { + + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + if (!super.equals(o)) { + return false; + } + AliasedColumn that = (AliasedColumn) o; + return alias.equals(that.alias); + } + + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), alias); + } } } diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/Table.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/Table.java index 39b2892847..0e1d681c19 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/Table.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/Table.java @@ -15,15 +15,18 @@ */ package org.springframework.data.relational.core.sql; +import java.util.Objects; + import org.springframework.util.Assert; /** - * Represents a table reference within a SQL statement. Typically used to denote {@code FROM} or {@code JOIN} or to + * Represents a table reference within a SQL statement. Typically, used to denote {@code FROM} or {@code JOIN} or to * prefix a {@link Column}. *

* Renders to: {@code } or {@code AS }. * * @author Mark Paluch + * @author Jens Schauder * @since 1.1 */ public class Table extends AbstractSegment implements TableLike { @@ -107,7 +110,6 @@ public Table as(SqlIdentifier alias) { return new AliasedTable(name, alias); } - /* * (non-Javadoc) * @see org.springframework.data.relational.core.sql.Named#getName() @@ -135,6 +137,26 @@ public String toString() { return name.toString(); } + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + if (!super.equals(o)) { + return false; + } + Table table = (Table) o; + return name.equals(table.name); + } + + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), name); + } + /** * {@link Aliased} {@link Table} implementation. */ @@ -184,5 +206,26 @@ public SqlIdentifier getReferenceName() { public String toString() { return getName() + " AS " + getAlias(); } + + @Override + public boolean equals(Object o) { + + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + if (!super.equals(o)) { + return false; + } + AliasedTable that = (AliasedTable) o; + return alias.equals(that.alias); + } + + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), alias); + } } } From 2430e0edd5247abafa3b03ad86fcc37aaca3fff6 Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Thu, 21 Oct 2021 15:43:22 +0200 Subject: [PATCH 3/3] Polishing. Code formatting. See #1073 --- .../data/jdbc/core/convert/SqlGenerator.java | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java index 93f47af8da..356c55819b 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlGenerator.java @@ -15,6 +15,11 @@ */ package org.springframework.data.jdbc.core.convert; +import java.util.*; +import java.util.function.Function; +import java.util.regex.Pattern; +import java.util.stream.Collectors; + import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Sort; import org.springframework.data.jdbc.repository.support.SimpleJdbcRepository; @@ -33,11 +38,6 @@ import org.springframework.lang.Nullable; import org.springframework.util.Assert; -import java.util.*; -import java.util.function.Function; -import java.util.regex.Pattern; -import java.util.stream.Collectors; - /** * Generates SQL statements to be used by {@link SimpleJdbcRepository} * @@ -751,12 +751,14 @@ Column getParentId() { @Override public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } Join join = (Join) o; - return joinTable.equals(join.joinTable) && - joinColumn.equals(join.joinColumn) && - parentId.equals(join.parentId); + return joinTable.equals(join.joinTable) && joinColumn.equals(join.joinColumn) && parentId.equals(join.parentId); } @Override @@ -767,10 +769,10 @@ public int hashCode() { @Override public String toString() { - return "Join{" + - "joinTable=" + joinTable + - ", joinColumn=" + joinColumn + - ", parentId=" + parentId + + return "Join{" + // + "joinTable=" + joinTable + // + ", joinColumn=" + joinColumn + // + ", parentId=" + parentId + // '}'; } }