CE-773 Feedback from code review

qqq-backend-module-filesystem/src/main/java/com/kingsrook/qqq/backend/module/filesystem/base/actions/AbstractBaseFilesystemAction.java

@@ -25,6 +25,7 @@ package com.kingsrook.qqq.backend.module.filesystem.base.actions;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
 import com.kingsrook.qqq.backend.core.actions.customizers.QCodeLoader;
@@ -52,6 +53,7 @@ import com.kingsrook.qqq.backend.module.filesystem.base.model.metadata.Cardinali
 import com.kingsrook.qqq.backend.module.filesystem.exceptions.FilesystemException;
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang.NotImplementedException;
+import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair;
 
 
 /*******************************************************************************
@@ -205,15 +207,17 @@ public abstract class AbstractBaseFilesystemAction<FILE>
          AbstractFilesystemTableBackendDetails tableDetails = getTableBackendDetails(AbstractFilesystemTableBackendDetails.class, table);
          List<FILE>                            files        = listFiles(table, queryInput.getBackend(), queryInput.getFilter());
 
+         int recordCount = 0;
+
+         FILE_LOOP:
          for(FILE file : files)
          {
-            LOG.info("Processing file: " + getFullPathForFile(file));
-
             InputStream inputStream = readFile(file);
             switch(tableDetails.getCardinality())
             {
                case MANY:
                {
+                  LOG.info("Extracting records from file", logPair("table", table.getName()), logPair("path", getFullPathForFile(file)));
                   switch(tableDetails.getRecordFormat())
                   {
                      case CSV:
@@ -260,14 +264,33 @@ public abstract class AbstractBaseFilesystemAction<FILE>
                }
                case ONE:
                {
+                  ////////////////////////////////////////////////////////////////////////////////
+                  // for one-record tables, put the entire file's contents into a single record //
+                  ////////////////////////////////////////////////////////////////////////////////
                   String filePathWithoutBase = stripBackendAndTableBasePathsFromFileName(getFullPathForFile(file), queryInput.getBackend(), table);
-
                   byte[] bytes               = inputStream.readAllBytes();
+
                   QRecord record = new QRecord()
                      .withValue(tableDetails.getFileNameFieldName(), filePathWithoutBase)
                      .withValue(tableDetails.getContentsFieldName(), bytes);
                   queryOutput.addRecord(record);
 
+                  ///////////////////////////////////////////////////////////////////////////////////////////////////////////
+                  // keep our own count - in case the query output is using a pipe (e.g., so we can't just call a .size()) //
+                  ///////////////////////////////////////////////////////////////////////////////////////////////////////////
+                  recordCount++;
+
+                  ////////////////////////////////////////////////////////////////////////////
+                  // break out of the file loop if we have hit the limit (if one was given) //
+                  ////////////////////////////////////////////////////////////////////////////
+                  if(queryInput.getFilter() != null && queryInput.getFilter().getLimit() != null)
+                  {
+                     if(recordCount >= queryInput.getFilter().getLimit())
+                     {
+                        break FILE_LOOP;
+                     }
+                  }
+
                   break;
                }
                default:
@@ -374,6 +397,8 @@ public abstract class AbstractBaseFilesystemAction<FILE>
          QTableMetaData   table   = insertInput.getTable();
          QBackendMetaData backend = insertInput.getBackend();
 
+         output.setRecords(new ArrayList<>());
+
          AbstractFilesystemTableBackendDetails tableDetails = getTableBackendDetails(AbstractFilesystemTableBackendDetails.class, table);
          if(tableDetails.getCardinality().equals(Cardinality.ONE))
          {

qqq-backend-module-filesystem/src/main/java/com/kingsrook/qqq/backend/module/filesystem/local/actions/AbstractFilesystemAction.java

@@ -85,6 +85,14 @@ public class AbstractFilesystemAction extends AbstractBaseFilesystemAction<File>
       {
          if(filter != null && filter.hasAnyCriteria())
          {
+            if(CollectionUtils.nullSafeHasContents(filter.getSubFilters()))
+            {
+               ///////////////////////////////////////////////////////////////////////
+               // todo - well, we could - just build up a tree of and's and or's... //
+               ///////////////////////////////////////////////////////////////////////
+               throw (new QException("Filters with sub-filters are not supported for querying filesystems at this time."));
+            }
+
             List<FileFilter> fileFilterList = new ArrayList<>();
             for(QFilterCriteria criteria : filter.getCriteria())
             {

qqq-backend-module-filesystem/src/main/java/com/kingsrook/qqq/backend/module/filesystem/s3/utils/S3Utils.java

@@ -106,6 +106,10 @@ public class S3Utils
          useQQueryFilter = true;
       }
 
+      ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+      // if there's a filter for single file, make that file name the "prefix" that we send to s3, so we just get back that 1 file. //
+      // as this will be a common case.                                                                                             //
+      ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
       if(filter != null && useQQueryFilter)
       {
          if(filter.getCriteria() != null && filter.getCriteria().size() == 1)
@@ -200,6 +204,18 @@ public class S3Utils
             }
 
             rs.add(objectSummary);
+
+            /////////////////////////////////////////////////////////////////
+            // if we have a limit, and we've hit it, break out of the loop //
+            /////////////////////////////////////////////////////////////////
+            if(filter != null && useQQueryFilter && filter.getLimit() != null)
+            {
+               if(rs.size() >= filter.getLimit())
+               {
+                  break;
+               }
+            }
+
          }
       }
       while(listObjectsV2Result.isTruncated());
@@ -219,6 +235,14 @@ public class S3Utils
          return (true);
       }
 
+      if(CollectionUtils.nullSafeHasContents(filter.getSubFilters()))
+      {
+         ///////////////////////////////
+         // todo - well, we could ... //
+         ///////////////////////////////
+         throw (new QException("Filters with sub-filters are not supported for querying filesystems at this time."));
+      }
+
       Path path = Path.of(URI.create("file:///" + key));
 
       ////////////////////////////////////////////////////////////////////////////////////////////////////

qqq-backend-module-filesystem/src/test/java/com/kingsrook/qqq/backend/module/filesystem/local/actions/FilesystemQueryActionTest.java

@@ -93,13 +93,15 @@ public class FilesystemQueryActionTest extends FilesystemActionTest
    @Test
    public void testQueryForCardinalityOne() throws QException
    {
+      FilesystemQueryAction filesystemQueryAction = new FilesystemQueryAction();
+
       QueryInput queryInput = new QueryInput(TestUtils.TABLE_NAME_BLOB_LOCAL_FS);
       queryInput.setFilter(new QQueryFilter());
-      QueryOutput queryOutput = new FilesystemQueryAction().execute(queryInput);
+      QueryOutput queryOutput = filesystemQueryAction.execute(queryInput);
       assertEquals(3, queryOutput.getRecords().size(), "Unfiltered query should find all rows");
 
       queryInput.setFilter(new QQueryFilter(new QFilterCriteria("fileName", QCriteriaOperator.EQUALS, "BLOB-1.txt")));
-      queryOutput = new FilesystemQueryAction().execute(queryInput);
+      queryOutput = filesystemQueryAction.execute(queryInput);
       assertEquals(1, queryOutput.getRecords().size(), "Filtered query should find 1 row");
       assertEquals("BLOB-1.txt", queryOutput.getRecords().get(0).getValueString("fileName"));
 
@@ -112,8 +114,15 @@ public class FilesystemQueryActionTest extends FilesystemActionTest
       reInitInstanceInContext(instance);
 
       queryInput.setFilter(new QQueryFilter());
-      queryOutput = new FilesystemQueryAction().execute(queryInput);
+      queryOutput = filesystemQueryAction.execute(queryInput);
       assertEquals(2, queryOutput.getRecords().size(), "Query should use glob and find 2 rows");
+
+      //////////////////////////////
+      // add a limit to the query //
+      //////////////////////////////
+      queryInput.setFilter(new QQueryFilter().withLimit(1));
+      queryOutput = filesystemQueryAction.execute(queryInput);
+      assertEquals(1, queryOutput.getRecords().size(), "Query with limit should be respected");
    }
 
 

qqq-backend-module-filesystem/src/test/java/com/kingsrook/qqq/backend/module/filesystem/s3/actions/S3QueryActionTest.java

@@ -105,6 +105,13 @@ public class S3QueryActionTest extends BaseS3Test
       queryInput.setFilter(new QQueryFilter());
       queryOutput = s3QueryAction.execute(queryInput);
       assertEquals(2, queryOutput.getRecords().size(), "Query should use glob and find 2 rows");
+
+      //////////////////////////////
+      // add a limit to the query //
+      //////////////////////////////
+      queryInput.setFilter(new QQueryFilter().withLimit(1));
+      queryOutput = s3QueryAction.execute(queryInput);
+      assertEquals(1, queryOutput.getRecords().size(), "Query with limit should be respected");
    }
 
 }