From 172b25f33e06bb8a71bf0537749a6fe208b5b784 Mon Sep 17 00:00:00 2001 From: Darin Kelkhoff Date: Mon, 8 Jul 2024 09:49:43 -0500 Subject: [PATCH] CE-1460 Fix in makeFromClause, to flip join before getting names out of it. Fixes a case where the JoinContext can send a backward join this far. --- .../rdbms/actions/AbstractRDBMSAction.java | 55 +++++-- .../actions/RDBMSQueryActionJoinsTest.java | 153 +++++++++++++++++- 2 files changed, 188 insertions(+), 20 deletions(-) diff --git a/qqq-backend-module-rdbms/src/main/java/com/kingsrook/qqq/backend/module/rdbms/actions/AbstractRDBMSAction.java b/qqq-backend-module-rdbms/src/main/java/com/kingsrook/qqq/backend/module/rdbms/actions/AbstractRDBMSAction.java index 082759bc..f9e12361 100644 --- a/qqq-backend-module-rdbms/src/main/java/com/kingsrook/qqq/backend/module/rdbms/actions/AbstractRDBMSAction.java +++ b/qqq-backend-module-rdbms/src/main/java/com/kingsrook/qqq/backend/module/rdbms/actions/AbstractRDBMSAction.java @@ -245,31 +245,64 @@ public abstract class AbstractRDBMSAction *******************************************************************************/ protected String makeFromClause(QInstance instance, String tableName, JoinsContext joinsContext, List params) { + ////////////////////////////////////////////////////////////////////// + // start with the main table - un-aliased (well, aliased as itself) // + ////////////////////////////////////////////////////////////////////// StringBuilder rs = new StringBuilder(escapeIdentifier(getTableName(instance.getTable(tableName))) + " AS " + escapeIdentifier(tableName)); + //////////////////////////////////////////////////////////////////////////////////////////////////////// + // sort the query joins from the main table "outward"... // + // this might not be perfect, e.g., for cases where what we actually might need is a tree of joins... // + //////////////////////////////////////////////////////////////////////////////////////////////////////// List queryJoins = sortQueryJoinsForFromClause(tableName, joinsContext.getQueryJoins()); + + //////////////////////////////////////////////////////// + // iterate over joins, adding to the from clause (rs) // + //////////////////////////////////////////////////////// for(QueryJoin queryJoin : queryJoins) { - QTableMetaData joinTable = instance.getTable(queryJoin.getJoinTable()); - String tableNameOrAlias = queryJoin.getJoinTableOrItsAlias(); + QTableMetaData joinTable = instance.getTable(queryJoin.getJoinTable()); + String joinTableNameOrAlias = queryJoin.getJoinTableOrItsAlias(); + //////////////////////////////////////////////////////// + // add the ` JOIN table AS alias` bit to the rs // + //////////////////////////////////////////////////////// rs.append(" ").append(queryJoin.getType()).append(" JOIN ") .append(escapeIdentifier(getTableName(joinTable))) - .append(" AS ").append(escapeIdentifier(tableNameOrAlias)); + .append(" AS ").append(escapeIdentifier(joinTableNameOrAlias)); - //////////////////////////////////////////////////////////// - // find the join in the instance, to set the 'on' clause // - //////////////////////////////////////////////////////////// + //////////////////////////////////////////////////////////////////////////////// + // find the join in the instance, for building the ON clause // + // append each sub-clause (condition) into a list, for later joining with AND // + //////////////////////////////////////////////////////////////////////////////// List joinClauseList = new ArrayList<>(); String baseTableName = Objects.requireNonNullElse(joinsContext.resolveTableNameOrAliasToTableName(queryJoin.getBaseTableOrAlias()), tableName); QJoinMetaData joinMetaData = Objects.requireNonNull(queryJoin.getJoinMetaData(), () -> "Could not find a join between tables [" + baseTableName + "][" + queryJoin.getJoinTable() + "]"); + ////////////////////////////////////////////////// + // loop over join-ons (e.g., multi-column join) // + ////////////////////////////////////////////////// for(JoinOn joinOn : joinMetaData.getJoinOns()) { + //////////////////////////////////////////////////////////////////////////////////////////////////////// + // figure out if the join needs flipped. We want its left table to equal the queryJoin's base table. // + //////////////////////////////////////////////////////////////////////////////////////////////////////// QTableMetaData leftTable = instance.getTable(joinMetaData.getLeftTable()); QTableMetaData rightTable = instance.getTable(joinMetaData.getRightTable()); + if(!joinMetaData.getLeftTable().equals(baseTableName)) + { + joinOn = joinOn.flip(); + QTableMetaData tmpTable = leftTable; + leftTable = rightTable; + rightTable = tmpTable; + } + + //////////////////////////////////////////////////////////// + // get the table-names-or-aliases to use in the ON clause // + //////////////////////////////////////////////////////////// String baseTableOrAlias = queryJoin.getBaseTableOrAlias(); + String joinTableOrAlias = queryJoin.getJoinTableOrItsAlias(); if(baseTableOrAlias == null) { baseTableOrAlias = leftTable.getName(); @@ -279,15 +312,6 @@ public abstract class AbstractRDBMSAction } } - String joinTableOrAlias = queryJoin.getJoinTableOrItsAlias(); - if(!joinMetaData.getLeftTable().equals(baseTableName)) - { - joinOn = joinOn.flip(); - QTableMetaData tmpTable = leftTable; - leftTable = rightTable; - rightTable = tmpTable; - } - joinClauseList.add(escapeIdentifier(baseTableOrAlias) + "." + escapeIdentifier(getColumnName(leftTable.getField(joinOn.getLeftField()))) + " = " + escapeIdentifier(joinTableOrAlias) @@ -938,6 +962,7 @@ public abstract class AbstractRDBMSAction { try { + params = Objects.requireNonNullElse(params, Collections.emptyList()); params = params.size() <= 100 ? params : params.subList(0, 99); ///////////////////////////////////////////////////////////////////////////// diff --git a/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSQueryActionJoinsTest.java b/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSQueryActionJoinsTest.java index 12993de3..22480177 100644 --- a/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSQueryActionJoinsTest.java +++ b/qqq-backend-module-rdbms/src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSQueryActionJoinsTest.java @@ -26,11 +26,14 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; import com.kingsrook.qqq.backend.core.actions.tables.CountAction; +import com.kingsrook.qqq.backend.core.actions.tables.DeleteAction; import com.kingsrook.qqq.backend.core.actions.tables.QueryAction; import com.kingsrook.qqq.backend.core.context.QContext; import com.kingsrook.qqq.backend.core.exceptions.QException; import com.kingsrook.qqq.backend.core.model.actions.tables.count.CountInput; +import com.kingsrook.qqq.backend.core.model.actions.tables.delete.DeleteInput; import com.kingsrook.qqq.backend.core.model.actions.tables.query.QCriteriaOperator; import com.kingsrook.qqq.backend.core.model.actions.tables.query.QFilterCriteria; import com.kingsrook.qqq.backend.core.model.actions.tables.query.QFilterOrderBy; @@ -47,6 +50,7 @@ import com.kingsrook.qqq.backend.core.model.metadata.tables.QTableMetaData; import com.kingsrook.qqq.backend.core.model.session.QSession; import com.kingsrook.qqq.backend.core.utils.StringUtils; import com.kingsrook.qqq.backend.core.utils.collections.ListBuilder; +import com.kingsrook.qqq.backend.core.utils.collections.SetBuilder; import com.kingsrook.qqq.backend.module.rdbms.TestUtils; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -54,6 +58,7 @@ import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; /******************************************************************************* @@ -69,10 +74,6 @@ public class RDBMSQueryActionJoinsTest extends RDBMSActionTest public void beforeEach() throws Exception { super.primeTestDatabase(); - - AbstractRDBMSAction.setLogSQL(true); - AbstractRDBMSAction.setLogSQLReformat(true); - AbstractRDBMSAction.setLogSQLOutput("system.out"); } @@ -909,7 +910,7 @@ public class RDBMSQueryActionJoinsTest extends RDBMSActionTest ** *******************************************************************************/ @Test - void testMultipleReversedDirectionJoinsBetweenSameTables() throws QException + void testMultipleReversedDirectionJoinsBetweenSameTablesAllAccessKey() throws QException { QContext.setQSession(new QSession().withSecurityKeyValue(TestUtils.SECURITY_KEY_STORE_ALL_ACCESS, true)); @@ -952,6 +953,76 @@ public class RDBMSQueryActionJoinsTest extends RDBMSActionTest + /******************************************************************************* + ** + *******************************************************************************/ + @Test + void testMultipleReversedDirectionJoinsBetweenSameTables() throws QException + { + ///////////////////////////////////////////////////////////////////////////////////////////////// + // primer sql file loads 2 instructions for order 1, 3 for order 2, then 1 for all the others. // + // let's make things a bit more interesting by deleting the 2 for order 1 // + ///////////////////////////////////////////////////////////////////////////////////////////////// + QContext.setQSession(new QSession().withSecurityKeyValue(TestUtils.SECURITY_KEY_STORE_ALL_ACCESS, true)); + DeleteInput deleteInput = new DeleteInput(); + deleteInput.setTableName(TestUtils.TABLE_NAME_ORDER_INSTRUCTIONS); + deleteInput.setQueryFilter(new QQueryFilter(new QFilterCriteria("orderId", QCriteriaOperator.EQUALS, 1))); + new DeleteAction().execute(deleteInput); + + QContext.setQSession(new QSession().withSecurityKeyValue(TestUtils.TABLE_NAME_STORE, 1)); + + // Integer noOfOrders = new CountAction().execute(new CountInput(TestUtils.TABLE_NAME_ORDER)).getCount(); + // Integer noOfOrderInstructions = new CountAction().execute(new CountInput(TestUtils.TABLE_NAME_ORDER_INSTRUCTIONS)).getCount(); + + { + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + // make sure we can join on order.current_order_instruction_id = order_instruction.id -- and that we get back 1 row per order // + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + QueryInput queryInput = new QueryInput(); + queryInput.setTableName(TestUtils.TABLE_NAME_ORDER); + queryInput.withQueryJoin(new QueryJoin(TestUtils.TABLE_NAME_ORDER_INSTRUCTIONS).withSelect(true).withJoinMetaData(QContext.getQInstance().getJoin("orderJoinCurrentOrderInstructions"))); + QueryOutput queryOutput = new QueryAction().execute(queryInput); + assertEquals(2, queryOutput.getRecords().size()); + } + + { + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + // make sure we can join on order.current_order_instruction_id = order_instruction.id -- and that we get back 1 row per order // + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + QueryInput queryInput = new QueryInput(); + queryInput.setTableName(TestUtils.TABLE_NAME_ORDER); + queryInput.withQueryJoin(new QueryJoin(TestUtils.TABLE_NAME_ORDER_INSTRUCTIONS).withSelect(true).withType(QueryJoin.Type.LEFT).withJoinMetaData(QContext.getQInstance().getJoin("orderJoinCurrentOrderInstructions"))); + QueryOutput queryOutput = new QueryAction().execute(queryInput); + assertEquals(5, queryOutput.getRecords().size()); + } + + /* + { + //////////////////////////////////////////////////////////////////////////////////////////////// + // assert that the query succeeds (based on exposed join) if the joinMetaData isn't specified // + //////////////////////////////////////////////////////////////////////////////////////////////// + QueryInput queryInput = new QueryInput(); + queryInput.setTableName(TestUtils.TABLE_NAME_ORDER); + queryInput.withQueryJoin(new QueryJoin(TestUtils.TABLE_NAME_ORDER_INSTRUCTIONS)); + QueryOutput queryOutput = new QueryAction().execute(queryInput); + // assertEquals(noOfOrders, queryOutput.getRecords().size()); + } + + { + /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + // make sure we can join on order.id = order_instruction.order_id (e.g., not the exposed one used above) -- and that we get back 1 row per order instruction // + /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + QueryInput queryInput = new QueryInput(); + queryInput.setTableName(TestUtils.TABLE_NAME_ORDER); + queryInput.withQueryJoin(new QueryJoin(TestUtils.TABLE_NAME_ORDER_INSTRUCTIONS).withJoinMetaData(QContext.getQInstance().getJoin("orderInstructionsJoinOrder"))); + QueryOutput queryOutput = new QueryAction().execute(queryInput); + // assertEquals(noOfOrderInstructions, queryOutput.getRecords().size()); + } + */ + } + + + /******************************************************************************* ** *******************************************************************************/ @@ -992,6 +1063,32 @@ public class RDBMSQueryActionJoinsTest extends RDBMSActionTest + /******************************************************************************* + ** We had, at one time, a bug where, for tables with 2 joins between each other, + ** an ON clause could get written using the wrong table name in one part. + ** + ** With that bug, this QueryAction.execute would throw an SQL Exception. + ** + ** So this test, just makes sure that no such exception gets thrown. + *******************************************************************************/ + @Test + void testFlippedJoinForOnClause() throws QException + { + QContext.setQSession(new QSession().withSecurityKeyValue(TestUtils.TABLE_NAME_STORE, 1)); + + QueryInput queryInput = new QueryInput(); + queryInput.setTableName(TestUtils.TABLE_NAME_ORDER_INSTRUCTIONS); + queryInput.withQueryJoin(new QueryJoin(TestUtils.TABLE_NAME_ORDER)); + QueryOutput queryOutput = new QueryAction().execute(queryInput); + assertFalse(queryOutput.getRecords().isEmpty()); + + //////////////////////////////////// + // if no exception, then we pass. // + //////////////////////////////////// + } + + + /******************************************************************************* ** Addressing a regression where a table was brought into a query for its ** security field, but it was a write-scope lock, so, it shouldn't have been. @@ -1029,4 +1126,50 @@ public class RDBMSQueryActionJoinsTest extends RDBMSActionTest assertEquals(5, new QueryAction().execute(new QueryInput(TestUtils.TABLE_NAME_PERSON)).getRecords().size()); } + + + /******************************************************************************* + ** scenario: + ** order LEFT JOIN shipment. both have storeId as security field. + ** not all orders have a shipment (but due to left-join, expect to get all + ** orders back, even if missing a shipment). + ** + ** If the security clause for shipment is in the WHERE clause, it will "negate" + ** the effect of the LEFT JOIN (unless it includes, e.g., "OR id IS NULL"). + ** + ** At one point, we tried AND'ing such a security clause to the JOIN ... ON, + ** but that is frequently causing issues... + *******************************************************************************/ + @Test + void testLeftJoinSecurityClause() throws QException + { + ////////////////////////////////////////////////////////////////////////////////// + // scenario: // + // order LEFT JOIN shipment. both have storeId as security field. // + // not all orders have a shipment (but due to left-join, expect to get all // + // orders back, even if missing a shipment). // + // // + // If the security clause for shipment is in the WHERE clause, it will "negate" // + // the effect of the LEFT JOIN (unless it includes, e.g., "OR id IS NULL"). // + // // + // At one point, we tried AND'ing such a security clause to the JOIN ... ON, // + // but that is frequently causing issues... // + // // + // order table has 3 rows for store 1: // + // - id=1 has 1 shipment, but assigned to the wrong store! // + // - id=2 has no shipments. // + // - id=3 has 2 shipments. // + ////////////////////////////////////////////////////////////////////////////////// + QContext.setQSession(new QSession().withSecurityKeyValue(TestUtils.TABLE_NAME_STORE, 1)); + + QueryInput queryInput = new QueryInput(); + queryInput.setTableName(TestUtils.TABLE_NAME_ORDER); + queryInput.withQueryJoin(new QueryJoin(TestUtils.TABLE_NAME_SHIPMENT).withSelect(true).withType(QueryJoin.Type.LEFT)); + QueryOutput queryOutput = new QueryAction().execute(queryInput); + List records = queryOutput.getRecords(); + assertEquals(4, records.size(), "expected no of records"); + assertEquals(SetBuilder.of(1), records.stream().map(r -> r.getValue("storeId")).collect(Collectors.toSet())); + assertEquals(SetBuilder.of(null, 1), records.stream().map(r -> r.getValue(TestUtils.TABLE_NAME_SHIPMENT + ".storeId")).collect(Collectors.toSet())); + } + }