From 8160d1f01cb5660deaa464a03c21430ce1a31e9d Mon Sep 17 00:00:00 2001 From: rob bygrave Date: Fri, 19 Oct 2018 15:56:39 +1300 Subject: [PATCH] #1508 - Invalid SQL used to fetch identity value for @Draftable with database platform that uses Identity but not getGeneratedKeys --- .../ebean/config/dbplatform/DbIdentity.java | 8 +- .../server/core/PersistRequestBean.java | 6 + .../server/deploy/BeanDescriptor.java | 13 ++- .../server/deploy/BeanDescriptorManager.java | 5 +- .../deploy/meta/DeployBeanDescriptor.java | 8 +- .../server/persist/dml/InsertHandler.java | 13 +-- .../server/persist/dml/InsertMeta.java | 20 ++-- .../config/PlatformNoGeneratedKeysTest.java | 103 ++++++++++++++++++ .../model/draftable/BasicDraftableBean.java | 32 ++++++ src/test/resources/extra-ddl.xml | 2 +- 10 files changed, 182 insertions(+), 28 deletions(-) create mode 100644 src/test/java/io/ebean/config/PlatformNoGeneratedKeysTest.java create mode 100644 src/test/java/org/tests/model/draftable/BasicDraftableBean.java diff --git a/src/main/java/io/ebean/config/dbplatform/DbIdentity.java b/src/main/java/io/ebean/config/dbplatform/DbIdentity.java index af1f4574c..c57626b3f 100644 --- a/src/main/java/io/ebean/config/dbplatform/DbIdentity.java +++ b/src/main/java/io/ebean/config/dbplatform/DbIdentity.java @@ -8,7 +8,9 @@ import java.util.regex.Pattern; */ public class DbIdentity { - private static final Pattern TABLE_REPLACE = Pattern.compile("{table}", Pattern.LITERAL); + private static final String TABLE_PLACEHOLDER = "{table}"; + + private static final Pattern TABLE_REPLACE = Pattern.compile(TABLE_PLACEHOLDER, Pattern.LITERAL); /** * Set if this DB supports sequences. Note some DB's support both Sequences @@ -53,7 +55,9 @@ public class DbIdentity { if (selectLastInsertedIdTemplate == null) { return null; } - + if (!selectLastInsertedIdTemplate.contains(TABLE_PLACEHOLDER)) { + return selectLastInsertedIdTemplate; + } return TABLE_REPLACE.matcher(selectLastInsertedIdTemplate).replaceAll(Matcher.quoteReplacement(table)); } diff --git a/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java b/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java index 44e5a32ad..acaa7c57d 100644 --- a/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java +++ b/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java @@ -1398,4 +1398,10 @@ public final class PersistRequestBean extends PersistRequest implements BeanP return orphanBean; } + /** + * Return the SQL used to fetch the last inserted id value. + */ + public String getSelectLastInsertedId() { + return beanDescriptor.getSelectLastInsertedId(publish); + } } diff --git a/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java b/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java index f114970f9..6f951ab87 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java +++ b/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java @@ -182,6 +182,7 @@ public class BeanDescriptor implements BeanType, STreeType { * getGeneratedKeys is not supported. */ private final String selectLastInsertedId; + private final String selectLastInsertedIdDraft; private final boolean autoTunable; @@ -473,6 +474,7 @@ public class BeanDescriptor implements BeanType, STreeType { this.sequenceInitialValue = deploy.getSequenceInitialValue(); this.sequenceAllocationSize = deploy.getSequenceAllocationSize(); this.selectLastInsertedId = deploy.getSelectLastInsertedId(); + this.selectLastInsertedIdDraft = deploy.getSelectLastInsertedIdDraft(); this.concurrencyMode = deploy.getConcurrencyMode(); this.updateChangesOnly = deploy.isUpdateChangesOnly(); this.indexDefinitions = deploy.getIndexDefinitions(); @@ -3086,8 +3088,15 @@ public class BeanDescriptor implements BeanType, STreeType { * supported. *

*/ - public String getSelectLastInsertedId() { - return selectLastInsertedId; + public String getSelectLastInsertedId(boolean publish) { + return publish ? selectLastInsertedId : selectLastInsertedIdDraft; + } + + /** + * Return true if this bean uses a SQL select to fetch the last inserted id value. + */ + public boolean supportsSelectLastInsertedId() { + return selectLastInsertedId != null; } @Override diff --git a/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptorManager.java b/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptorManager.java index 470df5ac8..1083235cc 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptorManager.java +++ b/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptorManager.java @@ -1387,9 +1387,10 @@ public class BeanDescriptorManager implements BeanDescriptorMap { } if (IdType.IDENTITY == desc.getIdType()) { - // used when getGeneratedKeys is not supported (SQL Server 2000) + // used when getGeneratedKeys is not supported (SQL Server 2000, SAP Hana) String selectLastInsertedId = dbIdentity.getSelectLastInsertedId(desc.getBaseTable()); - desc.setSelectLastInsertedId(selectLastInsertedId); + String selectLastInsertedIdDraft = (!desc.isDraftable()) ? selectLastInsertedId : dbIdentity.getSelectLastInsertedId(desc.getDraftTable()); + desc.setSelectLastInsertedId(selectLastInsertedId, selectLastInsertedIdDraft); return; } diff --git a/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanDescriptor.java b/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanDescriptor.java index e6688c222..76a54220a 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanDescriptor.java +++ b/src/main/java/io/ebeaninternal/server/deploy/meta/DeployBeanDescriptor.java @@ -126,6 +126,7 @@ public class DeployBeanDescriptor { * Used with Identity columns but no getGeneratedKeys support. */ private String selectLastInsertedId; + private String selectLastInsertedIdDraft; /** * The concurrency mode for beans of this type. @@ -839,11 +840,16 @@ public class DeployBeanDescriptor { return selectLastInsertedId; } + public String getSelectLastInsertedIdDraft() { + return selectLastInsertedIdDraft; + } + /** * Set the SQL used to return the last inserted Id. */ - public void setSelectLastInsertedId(String selectLastInsertedId) { + public void setSelectLastInsertedId(String selectLastInsertedId, String selectLastInsertedIdDraft) { this.selectLastInsertedId = selectLastInsertedId; + this.selectLastInsertedIdDraft = selectLastInsertedIdDraft; } /** diff --git a/src/main/java/io/ebeaninternal/server/persist/dml/InsertHandler.java b/src/main/java/io/ebeaninternal/server/persist/dml/InsertHandler.java index 8e7246730..70e2917c8 100644 --- a/src/main/java/io/ebeaninternal/server/persist/dml/InsertHandler.java +++ b/src/main/java/io/ebeaninternal/server/persist/dml/InsertHandler.java @@ -39,7 +39,7 @@ public class InsertHandler extends DmlHandler { * A SQL Select used to fetch back the Id where generatedKeys is not * supported. */ - private String selectLastInsertedId; + private boolean useSelectLastInsertedId; /** * Create to handle the insert execution. @@ -79,7 +79,7 @@ public class InsertHandler extends DmlHandler { useGeneratedKeys = true; } else { // use a query to get the last inserted id - selectLastInsertedId = meta.getSelectLastInsertedId(); + useSelectLastInsertedId = meta.supportsSelectLastInsertedId(); } } @@ -117,8 +117,7 @@ public class InsertHandler extends DmlHandler { } /** - * Execute the insert in a normal non batch fashion. Additionally using - * getGeneratedKeys if required. + * Execute non batched insert additionally using getGeneratedKeys if required. */ @Override public int execute() throws SQLException, OptimisticLockException { @@ -127,7 +126,7 @@ public class InsertHandler extends DmlHandler { // get the auto-increment value back and set into the bean getGeneratedKeys(); - } else if (selectLastInsertedId != null) { + } else if (useSelectLastInsertedId) { // fetch back the Id using a query fetchGeneratedKeyUsingSelect(); } @@ -167,12 +166,10 @@ public class InsertHandler extends DmlHandler { */ private void fetchGeneratedKeyUsingSelect() throws SQLException { - Connection conn = transaction.getConnection(); - PreparedStatement stmt = null; ResultSet rset = null; try { - stmt = conn.prepareStatement(selectLastInsertedId); + stmt = transaction.getConnection().prepareStatement(persistRequest.getSelectLastInsertedId()); rset = stmt.executeQuery(); setGeneratedKey(rset); } finally { diff --git a/src/main/java/io/ebeaninternal/server/persist/dml/InsertMeta.java b/src/main/java/io/ebeaninternal/server/persist/dml/InsertMeta.java index ff11081aa..d38ae2952 100644 --- a/src/main/java/io/ebeaninternal/server/persist/dml/InsertMeta.java +++ b/src/main/java/io/ebeaninternal/server/persist/dml/InsertMeta.java @@ -38,7 +38,7 @@ public final class InsertMeta { /** * Used for DB that do not support getGeneratedKeys. */ - private final String selectLastInsertedId; + private final boolean supportsSelectLastInsertedId; private final Bindable shadowFKey; @@ -69,7 +69,7 @@ public final class InsertMeta { this.sqlNullId = null; this.sqlDraftNullId = null; this.supportsGetGeneratedKeys = false; - this.selectLastInsertedId = null; + this.supportsSelectLastInsertedId = false; } else { // insert sql for db identity or sequence insert @@ -77,11 +77,11 @@ public final class InsertMeta { if (id.getIdentityColumn() == null) { this.identityDbColumns = new String[]{}; this.supportsGetGeneratedKeys = false; - this.selectLastInsertedId = null; + this.supportsSelectLastInsertedId = false; } else { this.identityDbColumns = new String[]{id.getIdentityColumn()}; this.supportsGetGeneratedKeys = dbPlatform.getDbIdentity().isSupportsGetGeneratedKeys(); - this.selectLastInsertedId = desc.getSelectLastInsertedId(); + this.supportsSelectLastInsertedId = desc.supportsSelectLastInsertedId(); } this.sqlNullId = genSql(true, tableName, false); this.sqlDraftNullId = desc.isDraftable() ? genSql(true, draftTableName, true) : sqlNullId; @@ -116,15 +116,11 @@ public final class InsertMeta { } /** - * Returns sql that is used to fetch back the last inserted id. This will - * return null if it should not be used. - *

- * This is only for DB's that do not support getGeneratedKeys. For MS - * SQLServer 2000 this could return "SELECT (at)(at)IDENTITY as id". - *

+ * Return true if we should use a SQL query to return the generated key. + * This can not be used with JDBC batch mode. */ - public String getSelectLastInsertedId() { - return selectLastInsertedId; + public boolean supportsSelectLastInsertedId() { + return supportsSelectLastInsertedId; } /** diff --git a/src/test/java/io/ebean/config/PlatformNoGeneratedKeysTest.java b/src/test/java/io/ebean/config/PlatformNoGeneratedKeysTest.java new file mode 100644 index 000000000..857577a9c --- /dev/null +++ b/src/test/java/io/ebean/config/PlatformNoGeneratedKeysTest.java @@ -0,0 +1,103 @@ +package io.ebean.config; + +import io.ebean.EbeanServer; +import io.ebean.EbeanServerFactory; +import io.ebean.Transaction; +import io.ebean.annotation.Platform; +import io.ebean.config.dbplatform.DbIdentity; +import io.ebean.config.dbplatform.IdType; +import io.ebean.config.dbplatform.h2.H2Platform; +import org.junit.Test; +import org.tests.model.basic.EBasicVer; +import org.tests.model.draftable.BasicDraftableBean; + +import static org.assertj.core.api.StrictAssertions.assertThat; + +public class PlatformNoGeneratedKeysTest { + + static EbeanServer server = testH2Server(); + + @Test + public void insertBatch_expect_noIdValuesFetched() { + + EBasicVer b0 = new EBasicVer("a"); + EBasicVer b1 = new EBasicVer("b"); + EBasicVer b2 = new EBasicVer("c"); + + try (Transaction transaction = server.beginTransaction()) { + transaction.setBatchMode(true); + + server.save(b0); + server.save(b1); + server.save(b2); + + transaction.commit(); + } + + assertThat(b0.getId()).isNull(); + assertThat(b1.getId()).isNull(); + assertThat(b2.getId()).isNull(); + + } + + @Test + public void insertNoBatch_expect_selectIdentity() { + + EBasicVer b0 = new EBasicVer("one"); + server.save(b0); + + assertThat(b0.getId()).isNotNull(); + + + BasicDraftableBean d0 = new BasicDraftableBean("done"); + server.save(d0); + + assertThat(d0.getId()).isNotNull(); + + server.publish(BasicDraftableBean.class, d0.getId()); + + BasicDraftableBean one = server.find(BasicDraftableBean.class, d0.getId()); + + assertThat(one.getName()).isEqualTo("done"); + assertThat(one.isDraft()).isFalse(); + } + + private static EbeanServer testH2Server() { + + ServerConfig config = new ServerConfig(); + config.setName("h2_noGeneratedKeys"); + + OtherH2Platform platform = new OtherH2Platform(); + DbIdentity dbIdentity = platform.getDbIdentity(); + dbIdentity.setIdType(IdType.IDENTITY); + dbIdentity.setSupportsIdentity(true); + dbIdentity.setSupportsGetGeneratedKeys(false); + dbIdentity.setSupportsSequence(false); + dbIdentity.setSelectLastInsertedIdTemplate("select identity() --{table}"); + + config.setDatabasePlatform(platform); + config.getDataSourceConfig().setUsername("sa"); + config.getDataSourceConfig().setPassword(""); + config.getDataSourceConfig().setUrl("jdbc:h2:mem:withPCQuery;"); + config.getDataSourceConfig().setDriver("org.h2.Driver"); + + config.setDisableL2Cache(true); + config.setDefaultServer(false); + config.setRegister(false); + config.setDdlGenerate(true); + config.setDdlRun(true); + config.getClasses().add(EBasicVer.class); + config.getClasses().add(BasicDraftableBean.class); + + + return EbeanServerFactory.create(config); + } + + static class OtherH2Platform extends H2Platform { + + OtherH2Platform() { + super(); + this.platform = Platform.GENERIC; + } + } +} diff --git a/src/test/java/org/tests/model/draftable/BasicDraftableBean.java b/src/test/java/org/tests/model/draftable/BasicDraftableBean.java new file mode 100644 index 000000000..98b79cb68 --- /dev/null +++ b/src/test/java/org/tests/model/draftable/BasicDraftableBean.java @@ -0,0 +1,32 @@ +package org.tests.model.draftable; + +import io.ebean.annotation.Draft; +import io.ebean.annotation.Draftable; + +import javax.persistence.Entity; + +@Entity +@Draftable +public class BasicDraftableBean extends BaseDomain { + + private String name; + + @Draft + boolean draft; + + public BasicDraftableBean(String name) { + this.name = name; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public boolean isDraft() { + return draft; + } +} diff --git a/src/test/resources/extra-ddl.xml b/src/test/resources/extra-ddl.xml index 8ca843ad6..bb786221b 100644 --- a/src/test/resources/extra-ddl.xml +++ b/src/test/resources/extra-ddl.xml @@ -4,7 +4,7 @@ drop view order_agg_vw if exists; - + create or replace view order_agg_vw as select d.order_id, sum(d.order_qty * d.unit_price) as order_total,