QQQ-21 feedback from code review

src/main/java/com/kingsrook/qqq/backend/module/rdbms/actions/AbstractRDBMSAction.java

@@ -25,13 +25,13 @@ package com.kingsrook.qqq.backend.module.rdbms.actions;
 import java.io.Serializable;
 import java.sql.Connection;
 import java.sql.SQLException;
-import java.time.OffsetDateTime;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.function.Function;
 import java.util.stream.Collectors;
 import com.kingsrook.qqq.backend.core.model.actions.AbstractQTableRequest;
 import com.kingsrook.qqq.backend.core.model.actions.query.QFilterCriteria;
+import com.kingsrook.qqq.backend.core.model.data.QRecord;
 import com.kingsrook.qqq.backend.core.model.metadata.QFieldMetaData;
 import com.kingsrook.qqq.backend.core.model.metadata.QFieldType;
 import com.kingsrook.qqq.backend.core.model.metadata.QTableMetaData;
@@ -108,18 +108,22 @@ public abstract class AbstractRDBMSAction
          }
       }
 
-      //////////////////////////////////////////////////////
+      return (value);
-      // todo - let this come from something in the field //
-      //////////////////////////////////////////////////////
-      if(value == null)
-      {
-         if((isInsert && field.getName().equals("createDate")) || field.getName().equals("modifyDate"))
-         {
-            value = OffsetDateTime.now();
-         }
    }
 
-      return (value);
+
+
+   /*******************************************************************************
+    ** If the table has a field with the given name, then set the given value in the
+    ** given record.
+    *******************************************************************************/
+   protected void setValueIfTableHasField(QRecord record, QTableMetaData table, String fieldName, Serializable value)
+   {
+      QFieldMetaData field = table.getField(fieldName);
+      if(field != null)
+      {
+         record.setValue(fieldName, value);
+      }
    }
 
 
@@ -279,6 +283,8 @@ public abstract class AbstractRDBMSAction
       return (String.join(" AND ", clauses));
    }
 
+
+
    /*******************************************************************************
     **
     *******************************************************************************/

src/main/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSInsertAction.java

@@ -24,6 +24,7 @@ package com.kingsrook.qqq.backend.module.rdbms.actions;
 
 import java.io.Serializable;
 import java.sql.Connection;
+import java.time.Instant;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.Collectors;
@@ -36,6 +37,8 @@ import com.kingsrook.qqq.backend.core.model.metadata.QTableMetaData;
 import com.kingsrook.qqq.backend.core.modules.interfaces.InsertInterface;
 import com.kingsrook.qqq.backend.core.utils.CollectionUtils;
 import com.kingsrook.qqq.backend.module.rdbms.jdbc.QueryManager;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 
 
 /*******************************************************************************
@@ -43,22 +46,38 @@ import com.kingsrook.qqq.backend.module.rdbms.jdbc.QueryManager;
  *******************************************************************************/
 public class RDBMSInsertAction extends AbstractRDBMSAction implements InsertInterface
 {
+   private static final Logger LOG = LogManager.getLogger(RDBMSInsertAction.class);
+
+
 
    /*******************************************************************************
     **
     *******************************************************************************/
    public InsertResult execute(InsertRequest insertRequest) throws QException
    {
+      InsertResult rs = new InsertResult();
+
       if(CollectionUtils.nullSafeIsEmpty(insertRequest.getRecords()))
       {
-         throw (new QException("Request to insert 0 records."));
+         LOG.info("Insert request called with 0 records.  Returning with no-op");
+         rs.setRecords(new ArrayList<>());
+         return (rs);
+      }
+
+      QTableMetaData table = insertRequest.getTable();
+      Instant        now   = Instant.now();
+
+      for(QRecord record : insertRequest.getRecords())
+      {
+         ///////////////////////////////////////////
+         // todo .. better (not hard-coded names) //
+         ///////////////////////////////////////////
+         setValueIfTableHasField(record, table, "createDate", now);
+         setValueIfTableHasField(record, table, "modifyDate", now);
       }
 
       try
       {
-         InsertResult   rs    = new InsertResult();
-         QTableMetaData table = insertRequest.getTable();
-
          List<QFieldMetaData> insertableFields = table.getFields().values().stream()
             .filter(field -> !field.getName().equals("id")) // todo - intent here is to avoid non-insertable fields.
             .toList();
@@ -74,8 +93,12 @@ public class RDBMSInsertAction extends AbstractRDBMSAction implements InsertInte
          StringBuilder sql       = new StringBuilder("INSERT INTO ").append(tableName).append("(").append(columns).append(") VALUES");
          List<Object>  params    = new ArrayList<>();
 
+         try(Connection connection = getConnection(insertRequest))
+         {
+            for(List<QRecord> page : CollectionUtils.getPages(insertRequest.getRecords(), QueryManager.PAGE_SIZE))
+            {
                int recordIndex = 0;
-         for(QRecord record : insertRequest.getRecords())
+               for(QRecord record : page)
                {
                   if(recordIndex++ > 0)
                   {
@@ -86,21 +109,13 @@ public class RDBMSInsertAction extends AbstractRDBMSAction implements InsertInte
                   {
                      Serializable value = record.getValue(field.getName());
                      value = scrubValue(field, value, true);
-
                      params.add(value);
                   }
                }
 
                // todo sql customization - can edit sql and/or param list
-
-         // QueryResult rs = new QueryResult();
-         // List<QRecord> records = new ArrayList<>();
-         // rs.setRecords(records);
-
                // todo - non-serial-id style tables
                // todo - other generated values, e.g., createDate...  maybe need to re-select?
-         try(Connection connection = getConnection(insertRequest))
-         {
                List<Integer> idList        = QueryManager.executeInsertForGeneratedIds(connection, sql.toString(), params);
                List<QRecord> outputRecords = new ArrayList<>();
                rs.setRecords(outputRecords);
@@ -113,6 +128,7 @@ public class RDBMSInsertAction extends AbstractRDBMSAction implements InsertInte
                   outputRecords.add(outputRecord);
                }
             }
+         }
 
          return rs;
       }

src/main/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSUpdateAction.java

@@ -25,6 +25,7 @@ package com.kingsrook.qqq.backend.module.rdbms.actions;
 import java.io.Serializable;
 import java.sql.Connection;
 import java.sql.SQLException;
+import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
@@ -41,6 +42,8 @@ import com.kingsrook.qqq.backend.core.utils.CollectionUtils;
 import com.kingsrook.qqq.backend.core.utils.ListingHash;
 import com.kingsrook.qqq.backend.core.utils.StringUtils;
 import com.kingsrook.qqq.backend.module.rdbms.jdbc.QueryManager;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 
 
 /*******************************************************************************
@@ -53,19 +56,26 @@ import com.kingsrook.qqq.backend.module.rdbms.jdbc.QueryManager;
  *******************************************************************************/
 public class RDBMSUpdateAction extends AbstractRDBMSAction implements UpdateInterface
 {
+   private static final Logger LOG = LogManager.getLogger(RDBMSUpdateAction.class);
+
+
 
    /*******************************************************************************
     **
     *******************************************************************************/
    public UpdateResult execute(UpdateRequest updateRequest) throws QException
    {
+      UpdateResult rs = new UpdateResult();
+
       if(CollectionUtils.nullSafeIsEmpty(updateRequest.getRecords()))
       {
-         throw (new QException("Request to update 0 records."));
+         LOG.info("Update request called with 0 records.  Returning with no-op");
+         rs.setRecords(new ArrayList<>());
+         return (rs);
       }
 
-      UpdateResult   rs    = new UpdateResult();
       QTableMetaData table = updateRequest.getTable();
+      Instant        now   = Instant.now();
 
       List<QRecord> outputRecords = new ArrayList<>();
       rs.setRecords(outputRecords);
@@ -78,6 +88,11 @@ public class RDBMSUpdateAction extends AbstractRDBMSAction implements UpdateInte
       ListingHash<List<String>, QRecord> recordsByFieldBeingUpdated = new ListingHash<>();
       for(QRecord record : updateRequest.getRecords())
       {
+         ////////////////////////////////////////////
+         // todo .. better (not a hard-coded name) //
+         ////////////////////////////////////////////
+         setValueIfTableHasField(record, table, "modifyDate", now);
+
          List<String> updatableFields = table.getFields().values().stream()
             .map(QFieldMetaData::getName)
             // todo - intent here is to avoid non-updateable fields - but this
@@ -159,8 +174,6 @@ public class RDBMSUpdateAction extends AbstractRDBMSAction implements UpdateInte
       // let query manager do the batch updates - note that it will internally page //
       ////////////////////////////////////////////////////////////////////////////////
       QueryManager.executeBatchUpdate(connection, sql, values);
-
-      // todo - auto-updated values, e.g., modifyDate...  maybe need to re-select?
    }
 
 
@@ -187,14 +200,16 @@ public class RDBMSUpdateAction extends AbstractRDBMSAction implements UpdateInte
     *******************************************************************************/
    private void updateRecordsWithMatchingValuesAndFields(Connection connection, QTableMetaData table, List<QRecord> recordList, List<String> fieldsBeingUpdated) throws SQLException
    {
-      String sql = writeUpdateSQLPrefix(table, fieldsBeingUpdated) + " IN (" + StringUtils.join(",", Collections.nCopies(recordList.size(), "?")) + ")";
+      for(List<QRecord> page : CollectionUtils.getPages(recordList, QueryManager.PAGE_SIZE))
+      {
+         String sql = writeUpdateSQLPrefix(table, fieldsBeingUpdated) + " IN (" + StringUtils.join(",", Collections.nCopies(page.size(), "?")) + ")";
 
          // todo sql customization? - let each table have custom sql and/or param list
 
          ////////////////////////////////////////////////////////////////
          // values in the update clause can come from the first record //
          ////////////////////////////////////////////////////////////////
-      QRecord      record0 = recordList.get(0);
+         QRecord      record0 = page.get(0);
          List<Object> params  = new ArrayList<>();
          for(String fieldName : fieldsBeingUpdated)
          {
@@ -206,16 +221,17 @@ public class RDBMSUpdateAction extends AbstractRDBMSAction implements UpdateInte
          //////////////////////////////////////////////////////////////////////
          // values in the where clause (in list) are the id from each record //
          //////////////////////////////////////////////////////////////////////
-      for(QRecord record : recordList)
+         for(QRecord record : page)
          {
             params.add(record.getValue(table.getPrimaryKeyField()));
          }
 
-      ////////////////////////////////////////////////////////////////////////////////
+         /////////////////////////////////////
-      // let query manager do the batch updates - note that it will internally page //
+         // let query manager do the update //
-      ////////////////////////////////////////////////////////////////////////////////
+         /////////////////////////////////////
          QueryManager.executeUpdate(connection, sql, params);
       }
+   }
 
 
 

src/main/java/com/kingsrook/qqq/backend/module/rdbms/jdbc/QueryManager.java

@@ -42,6 +42,8 @@ import java.time.temporal.ChronoUnit;
 import java.util.ArrayList;
 import java.util.Calendar;
 import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import com.kingsrook.qqq.backend.core.utils.CollectionUtils;
@@ -53,10 +55,17 @@ import org.apache.commons.lang.NotImplementedException;
  *******************************************************************************/
 public class QueryManager
 {
-   private static final int PAGE_SIZE        = 2000;
+   public static final  int PAGE_SIZE        = 2000;
    private static final int MS_PER_SEC       = 1000;
    private static final int NINETEEN_HUNDRED = 1900;
 
+   private static boolean collectStatistics = false;
+
+   private static final Map<String, Integer> statistics = Collections.synchronizedMap(new HashMap<>());
+
+   public static final String STAT_QUERIES_RAN = "queriesRan";
+   public static final String STAT_BATCHES_RAN = "batchesRan";
+
 
 
    /*******************************************************************************
@@ -85,6 +94,7 @@ public class QueryManager
       {
          statement = prepareStatementAndBindParams(connection, sql, params);
          statement.execute();
+         incrementStatistic(STAT_QUERIES_RAN);
          resultSet = statement.getResultSet();
 
          procesor.processResultSet(resultSet);
@@ -345,6 +355,7 @@ public class QueryManager
    {
       PreparedStatement statement = prepareStatementAndBindParams(connection, sql, params);
       statement.executeUpdate();
+      incrementStatistic(STAT_QUERIES_RAN);
       return (statement);
    }
 
@@ -357,6 +368,7 @@ public class QueryManager
    {
       PreparedStatement statement = prepareStatementAndBindParams(connection, sql, params);
       statement.executeUpdate();
+      incrementStatistic(STAT_QUERIES_RAN);
       return (statement);
    }
 
@@ -402,6 +414,7 @@ public class QueryManager
       try(PreparedStatement statement = prepareStatementAndBindParams(connection, sql, params))
       {
          statement.executeUpdate();
+         incrementStatistic(STAT_QUERIES_RAN);
          return (statement.getUpdateCount());
       }
    }
@@ -462,6 +475,7 @@ public class QueryManager
          bindParams(params.toArray(), statement);
          statement.executeUpdate();
          ResultSet generatedKeys = statement.getGeneratedKeys();
+         incrementStatistic(STAT_QUERIES_RAN);
          while(generatedKeys.next())
          {
             rs.add(getInteger(generatedKeys, 1));
@@ -552,6 +566,7 @@ public class QueryManager
             updatePS.addBatch();
          }
          updatePS.executeBatch();
+         incrementStatistic(STAT_BATCHES_RAN);
       }
    }
 
@@ -731,8 +746,6 @@ public class QueryManager
       }
    }
 
-
-
    /*******************************************************************************
     **
     *******************************************************************************/
@@ -743,8 +756,6 @@ public class QueryManager
    }
    */
 
-
-
    /*******************************************************************************
     **
     *******************************************************************************/
@@ -1561,4 +1572,52 @@ public class QueryManager
 
    // }
 
+
+
+   /*******************************************************************************
+    ** Setter for collectStatistics
+    **
+    *******************************************************************************/
+   public static void setCollectStatistics(boolean collectStatistics)
+   {
+      QueryManager.collectStatistics = collectStatistics;
+   }
+
+
+
+   /*******************************************************************************
+    ** Increment a statistic
+    **
+    *******************************************************************************/
+   public static void incrementStatistic(String statName)
+   {
+      if(collectStatistics)
+      {
+         statistics.putIfAbsent(statName, 0);
+         statistics.put(statName, statistics.get(statName) + 1);
+      }
+   }
+
+
+
+   /*******************************************************************************
+    ** clear the map of statistics
+    **
+    *******************************************************************************/
+   public static void resetStatistics()
+   {
+      statistics.clear();
+   }
+
+
+
+   /*******************************************************************************
+    ** Getter for statistics
+    **
+    *******************************************************************************/
+   public static Map<String, Integer> getStatistics()
+   {
+      return statistics;
+   }
+
 }

src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSInsertActionTest.java

@@ -22,7 +22,9 @@
 package com.kingsrook.qqq.backend.module.rdbms.actions;
 
 
+import java.util.Collections;
 import java.util.List;
+import com.kingsrook.qqq.backend.core.exceptions.QException;
 import com.kingsrook.qqq.backend.core.model.actions.insert.InsertRequest;
 import com.kingsrook.qqq.backend.core.model.actions.insert.InsertResult;
 import com.kingsrook.qqq.backend.core.model.data.QRecord;
@@ -50,6 +52,34 @@ public class RDBMSInsertActionTest extends RDBMSActionTest
 
 
 
+   /*******************************************************************************
+    **
+    *******************************************************************************/
+   @Test
+   public void testInsertNullList() throws QException
+   {
+      InsertRequest insertRequest = initInsertRequest();
+      insertRequest.setRecords(null);
+      InsertResult insertResult = new RDBMSInsertAction().execute(insertRequest);
+      assertEquals(0, insertResult.getRecords().size());
+   }
+
+
+
+   /*******************************************************************************
+    **
+    *******************************************************************************/
+   @Test
+   public void testInsertEmptyList() throws QException
+   {
+      InsertRequest insertRequest = initInsertRequest();
+      insertRequest.setRecords(Collections.emptyList());
+      InsertResult insertResult = new RDBMSInsertAction().execute(insertRequest);
+      assertEquals(0, insertResult.getRecords().size());
+   }
+
+
+
    /*******************************************************************************
     **
     *******************************************************************************/

src/test/java/com/kingsrook/qqq/backend/module/rdbms/actions/RDBMSUpdateActionTest.java

@@ -25,16 +25,18 @@ package com.kingsrook.qqq.backend.module.rdbms.actions;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import com.kingsrook.qqq.backend.core.exceptions.QException;
 import com.kingsrook.qqq.backend.core.model.actions.update.UpdateRequest;
 import com.kingsrook.qqq.backend.core.model.actions.update.UpdateResult;
 import com.kingsrook.qqq.backend.core.model.data.QRecord;
 import com.kingsrook.qqq.backend.module.rdbms.TestUtils;
+import com.kingsrook.qqq.backend.module.rdbms.jdbc.QueryManager;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
-import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.fail;
 
 
@@ -51,6 +53,20 @@ public class RDBMSUpdateActionTest extends RDBMSActionTest
    public void beforeEach() throws Exception
    {
       super.primeTestDatabase();
+
+      QueryManager.setCollectStatistics(true);
+      QueryManager.resetStatistics();
+   }
+
+
+   /*******************************************************************************
+    **
+    *******************************************************************************/
+   @AfterEach
+   public void afterEach() throws Exception
+   {
+      QueryManager.resetStatistics();
+      QueryManager.setCollectStatistics(false);
    }
 
 
@@ -59,14 +75,12 @@ public class RDBMSUpdateActionTest extends RDBMSActionTest
     **
     *******************************************************************************/
    @Test
-   public void testUpdateNullList()
+   public void testUpdateNullList() throws QException
    {
       UpdateRequest updateRequest = initUpdateRequest();
       updateRequest.setRecords(null);
-      assertThrows(QException.class, () ->
+      UpdateResult updateResult = new RDBMSUpdateAction().execute(updateRequest);
-      {
+      assertEquals(0, updateResult.getRecords().size());
-         new RDBMSUpdateAction().execute(updateRequest);
-      });
    }
 
 
@@ -75,14 +89,13 @@ public class RDBMSUpdateActionTest extends RDBMSActionTest
     **
     *******************************************************************************/
    @Test
-   public void testUpdateEmptyList()
+   public void testUpdateEmptyList() throws QException
    {
       UpdateRequest updateRequest = initUpdateRequest();
       updateRequest.setRecords(Collections.emptyList());
-      assertThrows(QException.class, () ->
-      {
       new RDBMSUpdateAction().execute(updateRequest);
-      });
+      UpdateResult updateResult = new RDBMSUpdateAction().execute(updateRequest);
+      assertEquals(0, updateResult.getRecords().size());
    }
 
 
@@ -101,7 +114,11 @@ public class RDBMSUpdateActionTest extends RDBMSActionTest
          .withValue("email", "jamestk@starfleet.net")
          .withValue("birthDate", "2210-05-20");
       updateRequest.setRecords(List.of(record));
+
       UpdateResult updateResult = new RDBMSUpdateAction().execute(updateRequest);
+      Map<String, Integer> statistics = QueryManager.getStatistics();
+      assertEquals(1, statistics.get(QueryManager.STAT_QUERIES_RAN));
+
       assertEquals(1, updateResult.getRecords().size(), "Should return 1 row");
       assertEquals(2, updateResult.getRecords().get(0).getValue("id"), "Should have id=2 in the row");
       // todo - add errors to QRecord? assertTrue(updateResult.getRecords().stream().noneMatch(qrs -> CollectionUtils.nullSafeHasContents(qrs.getErrors())), "There should be no errors");
@@ -150,7 +167,14 @@ public class RDBMSUpdateActionTest extends RDBMSActionTest
          .withValue("birthDate", null);
 
       updateRequest.setRecords(List.of(record1, record2, record3));
+
       UpdateResult updateResult = new RDBMSUpdateAction().execute(updateRequest);
+
+      // this test runs one batch and one regular query
+      Map<String, Integer> statistics = QueryManager.getStatistics();
+      assertEquals(1, statistics.get(QueryManager.STAT_BATCHES_RAN));
+      assertEquals(1, statistics.get(QueryManager.STAT_QUERIES_RAN));
+
       assertEquals(3, updateResult.getRecords().size(), "Should return 3 rows");
       assertEquals(1, updateResult.getRecords().get(0).getValue("id"), "Should have expected ids in the row");
       assertEquals(3, updateResult.getRecords().get(1).getValue("id"), "Should have expected ids in the row");
@@ -214,7 +238,11 @@ public class RDBMSUpdateActionTest extends RDBMSActionTest
          .withValue("birthDate", null);
 
       updateRequest.setRecords(List.of(record1, record2));
+
       UpdateResult updateResult = new RDBMSUpdateAction().execute(updateRequest);
+      Map<String, Integer> statistics = QueryManager.getStatistics();
+      assertEquals(1, statistics.get(QueryManager.STAT_BATCHES_RAN));
+
       assertEquals(2, updateResult.getRecords().size(), "Should return 2 rows");
       assertEquals(1, updateResult.getRecords().get(0).getValue("id"), "Should have expected ids in the row");
       assertEquals(3, updateResult.getRecords().get(1).getValue("id"), "Should have expected ids in the row");
@@ -257,11 +285,17 @@ public class RDBMSUpdateActionTest extends RDBMSActionTest
       List<QRecord> records       = new ArrayList<>();
       for(int i = 1; i <= 5; i++)
       {
-         records.add(new QRecord().withTableName("person").withValue("id", i).withValue("birthDate", "1999-09-09"));
+         records.add(new QRecord().withTableName("person")
+            .withValue("id", i)
+            .withValue("birthDate", "1999-09-09"));
       }
 
       updateRequest.setRecords(records);
+
       UpdateResult updateResult = new RDBMSUpdateAction().execute(updateRequest);
+      Map<String, Integer> statistics = QueryManager.getStatistics();
+      assertEquals(1, statistics.get(QueryManager.STAT_QUERIES_RAN));
+
       assertEquals(5, updateResult.getRecords().size(), "Should return 5 rows");
       // todo - add errors to QRecord? assertTrue(updateResult.getRecords().stream().noneMatch(qrs -> CollectionUtils.nullSafeHasContents(qrs.getErrors())), "There should be no errors");
       runTestSql("SELECT * FROM person WHERE id <= 5", (rs -> {

src/test/java/com/kingsrook/qqq/backend/module/rdbms/jdbc/QueryManagerTest.java

@@ -1,34 +0,0 @@
-/*
- * QQQ - Low-code Application Framework for Engineers.
- * Copyright (C) 2021-2022.  Kingsrook, LLC
- * 651 N Broad St Ste 205 # 6917 | Middletown DE 19709 | United States
- * contact@kingsrook.com
- * https://github.com/Kingsrook/
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU Affero General Public License as
- * published by the Free Software Foundation, either version 3 of the
- * License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU Affero General Public License for more details.
- *
- * You should have received a copy of the GNU Affero General Public License
- * along with this program.  If not, see <https://www.gnu.org/licenses/>.
- */
-
-package com.kingsrook.qqq.backend.module.rdbms.jdbc;
-
-
-import static org.junit.jupiter.api.Assertions.*;
-
-
-/*******************************************************************************
- **
- *******************************************************************************/
-class QueryManagerTest
-{
-
-}