mirror of
https://github.com/ebean-orm/ebean.git
synced 2024-04-21 10:51:47 +00:00
#2813 - Sometimes subquery use wrong alias in SQL
The reason for this is that as part of DefaultOrmQuery.copy() it uses DefaultExpressionList.copy() and that assumed that expressions were safe to share which is NOT the case for IN and EXISTS sub-query expressions so InQueryExpression and ExistsQueryExpression The effective fix for this is that DefaultExpressionList.copy() changes to call SpiExpression.copy() and for InQueryExpression and ExistsQueryExpression to implement that copy() by creating a copy of the sub-query. A "side-fix" is that in DefaultOrmQuery.createExtraJoinsToSupportManyWhereClause() it was creating an instance of ManyWhereJoins, then mutating it ... and if we change that to only doing the assignment at the end (object assignment is atomic) then racy access reading ManyWhereJoins would always get a fully completed non-mutating instance of ManyWhereJoins. Noting this because it kind of points to where I think the race condition is (in createExtraJoinsToSupportManyWhereClause()) but noting that with the change to DefaultExpressionList.copy() this "side-fix" isn't required per say.
This commit is contained in:
+37
-3
@@ -1,16 +1,20 @@
|
||||
package io.ebeaninternal.server.expression;
|
||||
|
||||
import io.ebean.ExpressionList;
|
||||
import io.ebean.Query;
|
||||
import io.ebeaninternal.api.SpiQuery;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import java.util.ArrayList;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.mockito.Mockito.mock;
|
||||
import static org.mockito.Mockito.verify;
|
||||
|
||||
public class DefaultExpressionListTest extends BaseExpressionTest {
|
||||
|
||||
|
||||
DefaultExpressionList<?> exp() {
|
||||
|
||||
return new DefaultExpressionList<>(null, new DefaultExpressionFactory(true, true), null);
|
||||
return new DefaultExpressionList<>(null, new DefaultExpressionFactory(true, true), null, new ArrayList<>());
|
||||
}
|
||||
|
||||
private <T> DefaultExpressionList<T> spi(ExpressionList<T> list) {
|
||||
@@ -95,4 +99,34 @@ public class DefaultExpressionListTest extends BaseExpressionTest {
|
||||
.isSameByBind(spi(exp().eq("a", 10).eq("b", 20)))).isFalse();
|
||||
}
|
||||
|
||||
@SuppressWarnings("unchecked")
|
||||
@Test
|
||||
void copy() {
|
||||
DefaultExpressionList<?> orig = exp();
|
||||
orig.eq("a", 10).in("b", 11);
|
||||
DefaultExpressionList<?> copy = (DefaultExpressionList<?>)orig.copy(mock(Query.class));
|
||||
|
||||
assertThat(copy).isNotSameAs(orig);
|
||||
assertThat(copy.list).hasSize(2);
|
||||
assertThat(copy.list.get(0)).isSameAs(orig.list.get(0));
|
||||
assertThat(copy.list.get(1)).isSameAs(orig.list.get(1));
|
||||
}
|
||||
|
||||
@SuppressWarnings("unchecked")
|
||||
@Test
|
||||
void copy_withSubQuery() {
|
||||
DefaultExpressionList<?> orig = exp();
|
||||
SpiQuery<?> inSubQuery = mock(SpiQuery.class);
|
||||
SpiQuery<?> existsSubQuery = mock(SpiQuery.class);
|
||||
orig.eq("a", 10).in("name", inSubQuery).exists(existsSubQuery);
|
||||
|
||||
DefaultExpressionList<?> copy = (DefaultExpressionList<?>)orig.copy(mock(Query.class));
|
||||
|
||||
assertThat(copy.list).hasSize(3);
|
||||
assertThat(copy.list.get(0)).isSameAs(orig.list.get(0));
|
||||
assertThat(copy.list.get(1)).isNotSameAs(orig.list.get(1));
|
||||
assertThat(copy.list.get(2)).isNotSameAs(orig.list.get(2));
|
||||
verify(inSubQuery).copy();
|
||||
verify(existsSubQuery).copy();
|
||||
}
|
||||
}
|
||||
|
||||
+20
@@ -1,10 +1,14 @@
|
||||
package io.ebeaninternal.server.expression;
|
||||
|
||||
import io.ebeaninternal.api.SpiExpression;
|
||||
import io.ebeaninternal.api.SpiQuery;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import java.util.Arrays;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.mockito.Mockito.mock;
|
||||
import static org.mockito.Mockito.verify;
|
||||
|
||||
public class ExistsQueryExpressionTest extends BaseExpressionTest {
|
||||
|
||||
@@ -13,6 +17,22 @@ public class ExistsQueryExpressionTest extends BaseExpressionTest {
|
||||
return new ExistsQueryExpression(not, sql, Arrays.asList(bindValues));
|
||||
}
|
||||
|
||||
@Test
|
||||
void copy_subQuery_expectNewInstance() {
|
||||
SpiQuery<?> subQuery = mock(SpiQuery.class);
|
||||
var orig = new ExistsQueryExpression(subQuery, false);
|
||||
SpiExpression copy = orig.copy();
|
||||
assertThat(copy).isNotSameAs(orig);
|
||||
verify(subQuery).copy();
|
||||
}
|
||||
|
||||
@Test
|
||||
void copy_sqlLiteral_expectSameInstance() {
|
||||
var orig = exp(true, "sql", 10);
|
||||
SpiExpression copy = orig.copy();
|
||||
assertThat(copy).isSameAs(orig);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void isSameByPlan_when_same() {
|
||||
|
||||
|
||||
+20
-1
@@ -1,18 +1,37 @@
|
||||
package io.ebeaninternal.server.expression;
|
||||
|
||||
import io.ebeaninternal.api.SpiExpression;
|
||||
import io.ebeaninternal.api.SpiQuery;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import java.util.Arrays;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.mockito.Mockito.mock;
|
||||
import static org.mockito.Mockito.verify;
|
||||
|
||||
public class InQueryExpressionTest extends BaseExpressionTest {
|
||||
|
||||
|
||||
private InQueryExpression exp(String propertyName, boolean not, String sql, Object... bindValues) {
|
||||
return new InQueryExpression(propertyName, not, sql, Arrays.asList(bindValues));
|
||||
}
|
||||
|
||||
@Test
|
||||
void copy_subQuery_expectNewInstance() {
|
||||
SpiQuery<?> subQuery = mock(SpiQuery.class);
|
||||
var orig = new InQueryExpression("name", subQuery, false);
|
||||
SpiExpression copy = orig.copy();
|
||||
assertThat(copy).isNotSameAs(orig);
|
||||
verify(subQuery).copy();
|
||||
}
|
||||
|
||||
@Test
|
||||
void copy_sqlLiteral_expectSameInstance() {
|
||||
var orig = exp("name", true, "sql", 10);
|
||||
SpiExpression copy = orig.copy();
|
||||
assertThat(copy).isSameAs(orig);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void isSameByPlan_when_same() {
|
||||
|
||||
|
||||
+3
-3
@@ -5,16 +5,16 @@ import io.ebean.Expression;
|
||||
import io.ebean.Junction;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import java.util.ArrayList;
|
||||
|
||||
public class JunctionExpressionTest extends BaseExpressionTest {
|
||||
|
||||
Expression eq(String propName, int value) {
|
||||
return Expr.eq(propName, value);
|
||||
}
|
||||
|
||||
|
||||
DefaultExpressionList<?> exp(Expression... expressions) {
|
||||
|
||||
DefaultExpressionList<Object> list = new DefaultExpressionList<>(null, new DefaultExpressionFactory(true, false), null);
|
||||
DefaultExpressionList<Object> list = new DefaultExpressionList<>(null, new DefaultExpressionFactory(true, false), null, new ArrayList<>());
|
||||
for (Expression ex : expressions) {
|
||||
list.add(ex);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user