From a46e086dc4afa22f29582cfa7161b899d96125d8 Mon Sep 17 00:00:00 2001 From: Roland Praml Date: Sun, 26 Feb 2017 00:37:36 +0100 Subject: [PATCH] fix several bugs in findSingleAttibute (#980) * add test case for distinct on id property * suggested fix for "select distinct id" * add test case for distinct with fetch * FIX: .setDistinct(true) can be used in conjunction with fetch() now * add test case for distinct with beans that have a disriminator column * FIX: discriminator column is only read if also Id is read. * fine tuned the test case * added test cases for findSingleAttributeList without distinct * improved fix to support also findSingleAttribute without distinct * ADD: Bonus test case - assertion not yet verified --- .../server/query/SqlTreeBuilder.java | 6 +- .../server/query/SqlTreeNodeBean.java | 23 +-- .../server/query/SqlTreeNodeManyRoot.java | 2 +- .../server/querydefn/DefaultOrmQuery.java | 5 + .../server/querydefn/OrmQueryDetail.java | 2 +- .../query/other/TestQuerySingleAttribute.java | 146 ++++++++++++++++++ 6 files changed, 171 insertions(+), 13 deletions(-) diff --git a/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java b/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java index 57dbb9b18..2390c9de1 100644 --- a/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java +++ b/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java @@ -322,7 +322,9 @@ public class SqlTreeBuilder { return new SqlTreeNodeManyRoot(prefix, (BeanPropertyAssocMany) prop, props, myList, temporalMode, disableLazyLoad); } else { - return new SqlTreeNodeBean(prefix, prop, props, myList, temporalMode, disableLazyLoad); + // do not read Id on child beans (e.g. when used with fetch()) + boolean withId = (query == null || !query.isSingleAttribute()); + return new SqlTreeNodeBean(prefix, prop, props, myList, withId, temporalMode, disableLazyLoad); } } @@ -438,7 +440,7 @@ public class SqlTreeBuilder { p = desc.findBeanProperty("id"); selectProps.add(p); - } else if (p.isId()) { + } else if (p.isId() && (query == null || !query.isSingleAttribute())) { // do not bother to include id for normal queries as the // id is always added (except for subQueries) diff --git a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java index 68901ff42..c601680b6 100644 --- a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java +++ b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java @@ -58,7 +58,7 @@ class SqlTreeNodeBean implements SqlTreeNode { * False if report bean and has no id property. */ protected final boolean readId; - + private final boolean disableLazyLoad; protected final InheritInfo inheritInfo; @@ -89,9 +89,9 @@ class SqlTreeNodeBean implements SqlTreeNode { * Construct for leaf node. */ SqlTreeNodeBean(String prefix, BeanPropertyAssoc beanProp, SqlTreeProperties props, - List myChildren, SpiQuery.TemporalMode temporalMode, boolean disableLazyLoad) { + List myChildren, boolean withId, SpiQuery.TemporalMode temporalMode, boolean disableLazyLoad) { - this(prefix, beanProp, beanProp.getTargetDescriptor(), props, myChildren, true, null, temporalMode, disableLazyLoad); + this(prefix, beanProp, beanProp.getTargetDescriptor(), props, myChildren, withId, null, temporalMode, disableLazyLoad); } /** @@ -135,6 +135,11 @@ class SqlTreeNodeBean implements SqlTreeNode { @Override public BeanProperty getSingleProperty() { + if (properties == null || properties.length == 0) { + // if we have no property ask first children (in a distinct select with join) + // if we have also no children, NPE happens anyway. + return children[0].getSingleProperty(); + } return properties[0]; } @@ -162,9 +167,9 @@ class SqlTreeNodeBean implements SqlTreeNode { @Override public void buildRawSqlSelectChain(List selectChain) { if (readId) { - if (desc.hasInheritance()) { + if (inheritInfo != null) { // discriminator column always proceeds id column - selectChain.add(getPath(prefix, desc.getInheritInfo().getDiscriminatorColumn())); + selectChain.add(getPath(prefix, inheritInfo.getDiscriminatorColumn())); } idBinder.buildRawSqlSelectChain(prefix, selectChain); } @@ -434,11 +439,11 @@ class SqlTreeNodeBean implements SqlTreeNode { lazyLoadParent.addSelectExported(ctx, prefix); } - if (!subQuery && inheritInfo != null) { - ctx.appendColumn(inheritInfo.getDiscriminatorColumn()); - } - if (readId) { + if (!subQuery && inheritInfo != null) { + ctx.appendColumn(inheritInfo.getDiscriminatorColumn()); + } + appendSelectId(ctx, idBinder.getBeanProperty()); } appendSelect(ctx, subQuery, properties); diff --git a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java index db1173c7c..835d201b3 100644 --- a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java +++ b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java @@ -15,7 +15,7 @@ final class SqlTreeNodeManyRoot extends SqlTreeNodeBean { SqlTreeNodeManyRoot(String prefix, BeanPropertyAssocMany prop, SqlTreeProperties props, List myList, SpiQuery.TemporalMode temporalMode, boolean disableLazyLoad) { - super(prefix, prop, props, myList, temporalMode, disableLazyLoad); + super(prefix, prop, props, myList, true, temporalMode, disableLazyLoad); this.manyProp = prop; } diff --git a/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java b/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java index 4dd9bf351..fe9e99fed 100644 --- a/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java +++ b/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java @@ -48,6 +48,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -1220,6 +1221,10 @@ public class DefaultOrmQuery implements SpiQuery { @Override @SuppressWarnings("unchecked") public List findSingleAttributeList() { + if (!detail.hasSelectClause()) { + // (no explicit select set - clear all properties) + detail.setBase(new OrmQueryProperties(null, new LinkedHashSet<>())); + } return (List) server.findSingleAttributeList(this, null); } diff --git a/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryDetail.java b/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryDetail.java index 36f423f85..b77e81689 100644 --- a/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryDetail.java +++ b/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryDetail.java @@ -440,7 +440,7 @@ public class OrmQueryDetail implements Serializable { } } - private boolean hasSelectClause() { + public boolean hasSelectClause() { return baseProps.hasSelectClause(); } diff --git a/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java b/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java index 069b208be..b4e1af080 100644 --- a/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java +++ b/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java @@ -5,6 +5,10 @@ import io.ebean.Ebean; import io.ebean.Query; import org.tests.model.basic.Customer; import org.tests.model.basic.ResetBasicData; +import org.avaje.test.model.rawsql.inherit.ChildA; +import org.avaje.test.model.rawsql.inherit.Data; +import org.avaje.test.model.rawsql.inherit.EUncle; +import org.junit.Ignore; import org.junit.Test; import java.sql.Date; @@ -125,4 +129,146 @@ public class TestQuerySingleAttribute extends BaseTestCase { query2.findList(); assertThat(sqlOf(query2, 1)).contains("select t0.id, t0.name from o_customer t0"); } + + @Test + public void distinctOnIdProperty(){ + Query query = Ebean.find(Customer.class) + .setDistinct(true) + .select("id") + .setMaxRows(100); + + List ids = query.findSingleAttributeList(); + if (isSqlServer()) { + assertThat(sqlOf(query)).contains("select distinct top 100 t0.id from o_customer t0"); + } else { + assertThat(sqlOf(query)).contains("select distinct t0.id from o_customer t0 limit 100"); + } + assertThat(ids).isNotEmpty(); + } + + @Test + public void distinctWithFetch() { + + ResetBasicData.reset(); + + Query query = Ebean.find(Customer.class) + .setDistinct(true) + .fetch("billingAddress","city") + .setMaxRows(100); + + List cities = query.findSingleAttributeList(); + + assertThat(sqlOf(query)).contains("select distinct t1.city from o_customer t0 left join o_address t1 on t1.id = t0.billing_address_id"); + assertThat(cities).contains("Auckland").containsNull(); + } + + @Test + public void distinctSelectOnInheritedBean() { + + ResetBasicData.reset(); + + Query query = Ebean.find(ChildA.class) + .setDistinct(true) + .select("more") + .setMaxRows(100); + + query.findSingleAttributeList(); + assertThat(sqlOf(query)).contains("select distinct t0.more from rawinherit_parent t0 where t0.type = 'A' limit 100"); + + } + + @Test + public void distinctFetchManyToOneInheritedBean() { + + ResetBasicData.reset(); + + Query query = Ebean.find(EUncle.class) + .setDistinct(true) + .fetch("parent","more") + .setMaxRows(100); + + query.findSingleAttributeList(); + + assertThat(sqlOf(query)).contains("select distinct t1.more from rawinherit_uncle t0 join rawinherit_parent t1 on t1.id = t0.parent_id and t1.type in ('A','B')"); + + } + + // hmm - same problem when not using distinct + @Test + public void findSingleOnIdProperty(){ + Query query = Ebean.find(Customer.class) + .select("id") + .setMaxRows(100); + + List ids = query.findSingleAttributeList(); + if (isSqlServer()) { + assertThat(sqlOf(query)).contains("select top 100 t0.id from o_customer t0"); + } else { + assertThat(sqlOf(query)).contains("select t0.id from o_customer t0 limit 100"); + } + assertThat(ids).isNotEmpty(); + } + + @Test + public void findSingleWithFetch() { + + ResetBasicData.reset(); + + Query query = Ebean.find(Customer.class) + .fetch("billingAddress","city") + .setMaxRows(100); + + List cities = query.findSingleAttributeList(); + + assertThat(cities).contains("Auckland").containsNull(); + assertThat(sqlOf(query)).contains("select t1.city from o_customer t0 left join o_address t1 on t1.id = t0.billing_address_id"); + } + + @Test + public void findSingleSelectOnInheritedBean() { + + ResetBasicData.reset(); + + Query query = Ebean.find(ChildA.class) + .select("more") + .setMaxRows(100); + + query.findSingleAttributeList(); + assertThat(sqlOf(query)).contains("select t0.more from rawinherit_parent t0 where t0.type = 'A' limit 100"); + + } + + @Test + public void findSingleFetchManyToOneInheritedBean() { + + ResetBasicData.reset(); + + Query query = Ebean.find(EUncle.class) + .fetch("parent","more") + .setMaxRows(100); + + query.findSingleAttributeList(); + + assertThat(sqlOf(query)).contains("select t1.more from rawinherit_uncle t0 join rawinherit_parent t1 on t1.id = t0.parent_id and t1.type in ('A','B')"); + + } + + @Test + @Ignore //don't know if ebean can handle this on many to many, as this means that the cartesian product is generated + + public void distinctFetchManyToManyInheritedBean() { + + ResetBasicData.reset(); + + Query query = Ebean.find(Data.class) + .setDistinct(true) + .fetch("parents","more") + .setMaxRows(100); + + query.findSingleAttributeList(); + + assertThat(sqlOf(query)).contains("select distinct t0.more from rawinherit_data t0 " + + "join rawinherit_parent_rawinherit_data t1 on t0.id = t1.rawinherit_data_id " + + "join parent t2 on t1.rawinherit_parent_id = t2.id"); + } }