CE-881 - Feedback from code review & continued QA testing

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/actions/reporting/excel/poi/ExcelPoiBasedStreamingExportStreamer.java

@@ -82,12 +82,26 @@ import org.apache.poi.xssf.usermodel.XSSFWorkbook;
 /*******************************************************************************
  ** Excel export format implementation using POI library, but with modifications
  ** to actually stream output rather than use any temp files.
+ **
+ ** For a rough outline:
+ ** - create a basically empty Excel workbook using POI - empty meaning, without
+ ** data rows.
+ ** - have POI write that workbook out into a byte[] - which will be a zip full
+ ** of xml (e.g., xlsx).
+ ** - then open a new ZipOutputStream wrapper around the OutputStream we took in
+ ** as the report destination (e.g., streamed into storage or http output)
+ ** - Copy over all entries from the xlsx into our new zip-output-stream, other than
+ ** ones that are the actual sheets that we want to put data into.
+ ** - For the sheet entries, use the StreamedPoiSheetWriter class to write the
+ ** report's data directly out as Excel XML (not using POI).
+ ** - Pivot tables require a bit of an additional hack - to write a "pivot cache
+ ** definition", which, while we won't put all the data in it (because we'll tell
+ ** it refreshOnLoad="true"), we will at least need to know how many cols & rows
+ ** are in the data-sheet (which we wouldn't know until we streamed that sheet!)
  *******************************************************************************/
 public class ExcelPoiBasedStreamingExportStreamer implements ExportStreamerInterface
 {
    private static final QLogger LOG = QLogger.getLogger(ExcelPoiBasedStreamingExportStreamer.class);
-   public static final String EXCEL_DATE_FORMAT = "yyyy-MM-dd";
-   public static final String EXCEL_DATE_TIME_FORMAT = "yyyy-MM-dd H:mm:ss";
 
    private List<QReportView>    views;
    private ExportInput          exportInput;
@@ -95,17 +109,20 @@ public class ExcelPoiBasedStreamingExportStreamer implements ExportStreamerInter
    private OutputStream         outputStream;
    private ZipOutputStream      zipOutputStream;
 
-   private PoiExcelStylerInterface poiExcelStylerInterface = getStylerInterface();
+   public static final String EXCEL_DATE_FORMAT      = "yyyy-MM-dd";
-   private Map<String, String>     excelCellFormats;
+   public static final String EXCEL_DATE_TIME_FORMAT = "yyyy-MM-dd H:mm:ss";
+
+   private PoiExcelStylerInterface    poiExcelStylerInterface = getStylerInterface();
+   private Map<String, String>        excelCellFormats;
+   private Map<String, XSSFCellStyle> styles                  = new HashMap<>();
 
    private int rowNo      = 0;
    private int sheetIndex = 1;
 
-   private Map<String, String>        pivotViewToCacheDefinitionReferenceMap = new HashMap<>();
+   private Map<String, String> pivotViewToCacheDefinitionReferenceMap = new HashMap<>();
-   private Map<String, XSSFCellStyle> styles                                 = new HashMap<>();
 
-   private Writer                 activeSheetWriter = null;
+   private Writer              activeSheetWriter = null;
-   private StreamedPoiSheetWriter sheetWriter       = null;
+   private StreamedSheetWriter sheetWriter       = null;
 
    private QReportView                       currentView      = null;
    private Map<String, List<QFieldMetaData>> fieldsPerView    = new HashMap<>();
@@ -271,7 +288,6 @@ public class ExcelPoiBasedStreamingExportStreamer implements ExportStreamerInter
       for(QReportField column : dataView.getColumns())
       {
          XSSFCell cell = headerRow.createCell(columnNo++);
-         // todo ... not like this
          cell.setCellValue(QInstanceEnricher.nameToLabel(column.getName()));
       }
 
@@ -435,7 +451,7 @@ public class ExcelPoiBasedStreamingExportStreamer implements ExportStreamerInter
          //////////////////////////////////////////
          zipOutputStream.putNextEntry(new ZipEntry("xl/worksheets/sheet" + this.sheetIndex++ + ".xml"));
          activeSheetWriter = new OutputStreamWriter(zipOutputStream);
-         sheetWriter = new StreamedPoiSheetWriter(activeSheetWriter);
+         sheetWriter = new StreamedSheetWriter(activeSheetWriter);
 
          if(ReportType.PIVOT.equals(view.getType()))
          {
@@ -582,7 +598,13 @@ public class ExcelPoiBasedStreamingExportStreamer implements ExportStreamerInter
                   String format = excelCellFormats.get(field.getName());
                   if(format != null)
                   {
-                     // todo - formats...
+                     ////////////////////////////////////////////////////////////////////////////////////////////
+                     // todo - so - for this streamed/zip approach, we need to know all styles before we start //
+                     // any sheets.  but, right now Report action only calls us with per-sheet styles when     //
+                     // it's starting individual sheets.  so, we can't quite support this at this time.        //
+                     // "just" need to change GenerateReportAction to look up all cell formats for all sheets  //
+                     // before preRun is called... and change all existing streamer classes to handle that too //
+                     ////////////////////////////////////////////////////////////////////////////////////////////
                      // worksheet.style(rowNo, col).format(format).set();
                   }
                }
@@ -609,7 +631,6 @@ public class ExcelPoiBasedStreamingExportStreamer implements ExportStreamerInter
             }
             else if(value instanceof Instant i)
             {
-               // todo - what would be a better zone to use here?
                sheetWriter.createCell(col, DateUtil.getExcelDate(i.atZone(ZoneId.systemDefault()).toLocalDateTime()), dateTimeStyleIndex);
             }
             else
@@ -656,10 +677,11 @@ public class ExcelPoiBasedStreamingExportStreamer implements ExportStreamerInter
          //////////////////////////////////////////////
          closeLastSheetIfOpen();
 
-         //////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+         /////////////////////////////////////////////////////////////////////////////////////
-         // note - leave the zipOutputStream open.  It is a wrapper around the OutputStream we were given by the caller, //
+         // so, we DO need a close here, on the zipOutputStream, to finish its "zippiness". //
-         // so it is their responsibility to close that stream (which implicitly closes the zip, it appears)             //
+         // even though, doing so also closes the outputStream from the caller that this    //
-         //////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+         // zipOutputStream is wrapping (and the caller will likely call close on too)...   //
+         /////////////////////////////////////////////////////////////////////////////////////
          zipOutputStream.close();
       }
       catch(Exception e)
@@ -748,7 +770,7 @@ public class ExcelPoiBasedStreamingExportStreamer implements ExportStreamerInter
          zipOutputStream.putNextEntry(new ZipEntry(pivotViewToCacheDefinitionReferenceMap.get(pivotTableView.getName())));
 
          /////////////////////////////////////////////////////////
-         // prepare the xml for each field (e.g., w/ its labelO //
+         // prepare the xml for each field (e.g., w/ its label) //
          /////////////////////////////////////////////////////////
          List<String> cachedFieldElements = new ArrayList<>();
          for(QFieldMetaData column : this.fieldsPerView.get(dataView.getName()))
@@ -766,7 +788,7 @@ public class ExcelPoiBasedStreamingExportStreamer implements ExportStreamerInter
          activeSheetWriter = new OutputStreamWriter(zipOutputStream);
          activeSheetWriter.write(String.format("""
                <?xml version="1.0" encoding="UTF-8"?>
-               <pivotCacheDefinition xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main" xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships" createdVersion="3" minRefreshableVersion="3" refreshedVersion="3" refreshedBy="Apache POI" refreshedDate="1.7113>  95767702E12" refreshOnLoad="true" r:id="rId1">
+               <pivotCacheDefinition xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main" xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships" createdVersion="3" minRefreshableVersion="3" refreshedVersion="3" refreshedBy="Apache POI" refreshedDate="1.711395767702E12" refreshOnLoad="true" r:id="rId1">
                  <cacheSource type="worksheet">
                    <worksheetSource sheet="%s" ref="A1:%s%d"/>
                  </cacheSource>
@@ -775,7 +797,7 @@ public class ExcelPoiBasedStreamingExportStreamer implements ExportStreamerInter
                  </cacheFields>
                </pivotCacheDefinition>
                """,
-            StreamedPoiSheetWriter.cleanseValue(labelViewsByName.get(dataView.getName())),
+            StreamedSheetWriter.cleanseValue(labelViewsByName.get(dataView.getName())),
             CellReference.convertNumToColString(dataView.getColumns().size() - 1),
             rowsPerView.get(dataView.getName()),
             dataView.getColumns().size(),

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/actions/reporting/excel/poi/StreamedSheetWriter.java

@@ -24,6 +24,8 @@ package com.kingsrook.qqq.backend.core.actions.reporting.excel.poi;
 
 import java.io.IOException;
 import java.io.Writer;
+import java.util.HashMap;
+import java.util.Map;
 import org.apache.poi.ss.util.CellReference;
 
 
@@ -31,7 +33,7 @@ import org.apache.poi.ss.util.CellReference;
  ** Write excel formatted XML to a Writer.
  ** Originally from https://coderanch.com/t/548897/java/Generate-large-excel-POI
  *******************************************************************************/
-public class StreamedPoiSheetWriter
+public class StreamedSheetWriter
 {
    private final Writer writer;
    private       int    rowNo;
@@ -41,7 +43,7 @@ public class StreamedPoiSheetWriter
    /*******************************************************************************
     **
     *******************************************************************************/
-   public StreamedPoiSheetWriter(Writer writer)
+   public StreamedSheetWriter(Writer writer)
    {
       this.writer = writer;
    }
@@ -125,14 +127,44 @@ public class StreamedPoiSheetWriter
    {
       if(value != null)
       {
-         if(value.indexOf('&') > -1 || value.indexOf('<') > -1 || value.indexOf('>') > -1 || value.indexOf('\'') > -1 || value.indexOf('"') > -1)
+         StringBuilder rs = new StringBuilder();
+         for(int i = 0; i < value.length(); i++)
          {
-            value = value.replace("&", "&amp;");
+            char c = value.charAt(i);
-            value = value.replace("<", "&lt;");
+            if(c == '&')
-            value = value.replace(">", "&gt;");
+            {
-            value = value.replace("'", "&apos;");
+               rs.append("&amp;");
-            value = value.replace("\"", "&quot;");
+            }
+            else if(c == '<')
+            {
+               rs.append("&lt;");
+            }
+            else if(c == '>')
+            {
+               rs.append("&gt;");
+            }
+            else if(c == '\'')
+            {
+               rs.append("&apos;");
+            }
+            else if(c == '"')
+            {
+               rs.append("&quot;");
+            }
+            else if (c < 32 && c != '\t' && c != '\n')
+            {
+               rs.append(' ');
+            }
+            else
+            {
+               rs.append(c);
+            }
          }
+
+         Map<String, Integer> m = new HashMap();
+         m.computeIfAbsent("s", (s) -> 3);
+
+         value = rs.toString();
       }
 
       return (value);
@@ -207,8 +239,4 @@ public class StreamedPoiSheetWriter
       writer.write("</c>");
    }
 
-   // todo? public void createCell(int columnIndex, Calendar value, int styleIndex) throws IOException
-   // todo? {
-   // todo?    createCell(columnIndex, DateUtil.getExcelDate(value, false), styleIndex);
-   // todo? }
 }

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/actions/tables/StorageAction.java

@@ -79,7 +79,7 @@ public class StorageAction
 
       if(storageInput.getTableName() == null)
       {
-         throw (new QException("Table name was not specified in query input"));
+         throw (new QException("Table name was not specified in storage input"));
       }
 
       QTableMetaData table = storageInput.getTable();

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/reporting/ReportDestination.java

@@ -26,7 +26,8 @@ import java.io.OutputStream;
 
 
 /*******************************************************************************
- **
+ ** Member of report & export Inputs, that wraps details about the destination of
+ ** where & how the report (or export) is being written.
  *******************************************************************************/
 public class ReportDestination
 {

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/reporting/ReportFormatPossibleValueEnum.java

@@ -31,8 +31,8 @@ import com.kingsrook.qqq.backend.core.model.metadata.possiblevalues.PossibleValu
 public enum ReportFormatPossibleValueEnum implements PossibleValueEnum<String>
 {
    XLSX,
-   JSON,
+   CSV,
-   CSV;
+   JSON;
 
    public static final String NAME = "reportFormat";
 

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/reporting/pivottable/PivotTableDefinition.java

@@ -22,6 +22,7 @@
 package com.kingsrook.qqq.backend.core.model.actions.reporting.pivottable;
 
 
+import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -29,7 +30,7 @@ import java.util.List;
 /*******************************************************************************
  ** Full definition of a pivot table - its rows, columns, and values.
  *******************************************************************************/
-public class PivotTableDefinition implements Cloneable
+public class PivotTableDefinition implements Cloneable, Serializable
 {
    private List<PivotTableGroupBy> rows;
    private List<PivotTableGroupBy> columns;

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/reporting/pivottable/PivotTableGroupBy.java

@@ -22,11 +22,14 @@
 package com.kingsrook.qqq.backend.core.model.actions.reporting.pivottable;
 
 
+import java.io.Serializable;
+
+
 /*******************************************************************************
  ** Either a row or column grouping in a pivot table.  e.g., a field plus
  ** sorting details, plus showTotals boolean.
  *******************************************************************************/
-public class PivotTableGroupBy implements Cloneable
+public class PivotTableGroupBy implements Cloneable, Serializable
 {
    private String            fieldName;
    private PivotTableOrderBy orderBy;

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/reporting/pivottable/PivotTableOrderBy.java

@@ -22,10 +22,13 @@
 package com.kingsrook.qqq.backend.core.model.actions.reporting.pivottable;
 
 
+import java.io.Serializable;
+
+
 /*******************************************************************************
  ** How a group-by (rows or columns) should be sorted.
  *******************************************************************************/
-public class PivotTableOrderBy
+public class PivotTableOrderBy implements Serializable
 {
    // todo - implement, but only if POI supports (or we build our own support...)
 }

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/reporting/pivottable/PivotTableValue.java

@@ -22,10 +22,13 @@
 package com.kingsrook.qqq.backend.core.model.actions.reporting.pivottable;
 
 
+import java.io.Serializable;
+
+
 /*******************************************************************************
  ** a value (e.g., field name + function) used in a pivot table
  *******************************************************************************/
-public class PivotTableValue implements Cloneable
+public class PivotTableValue implements Cloneable, Serializable
 {
    private String             fieldName;
    private PivotTableFunction function;

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/tables/query/QQueryFilter.java

@@ -30,6 +30,7 @@ import java.util.Map;
 import java.util.Objects;
 import com.kingsrook.qqq.backend.core.instances.QMetaDataVariableInterpreter;
 import com.kingsrook.qqq.backend.core.logging.QLogger;
+import com.kingsrook.qqq.backend.core.model.actions.tables.query.expressions.AbstractFilterExpression;
 import com.kingsrook.qqq.backend.core.utils.CollectionUtils;
 import com.kingsrook.qqq.backend.core.utils.ValueUtils;
 
@@ -409,9 +410,20 @@ public class QQueryFilter implements Serializable, Cloneable
 
             for(Serializable value : criterion.getValues())
             {
-               String       valueAsString    = ValueUtils.getValueAsString(value);
+               if(value instanceof AbstractFilterExpression<?>)
-               Serializable interpretedValue = variableInterpreter.interpretForObject(valueAsString);
+               {
-               newValues.add(interpretedValue);
+                  /////////////////////////////////////////////////////////////////////////
+                  // todo - do we want to try to interpret values within the expression? //
+                  // e.g., greater than now minus ${input.noOfDays}                      //
+                  /////////////////////////////////////////////////////////////////////////
+                  newValues.add(value);
+               }
+               else
+               {
+                  String       valueAsString    = ValueUtils.getValueAsString(value);
+                  Serializable interpretedValue = variableInterpreter.interpretForObject(valueAsString);
+                  newValues.add(interpretedValue);
+               }
             }
             criterion.setValues(newValues);
          }

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/actions/tables/storage/StorageInput.java

@@ -26,7 +26,7 @@ import com.kingsrook.qqq.backend.core.model.actions.AbstractTableActionInput;
 
 
 /*******************************************************************************
- **
+ ** Input for Storage actions.
  *******************************************************************************/
 public class StorageInput extends AbstractTableActionInput
 {

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/savedreports/SavedReport.java

@@ -52,7 +52,7 @@ public class SavedReport extends QRecordEntity
    private String label;
 
    @QField(possibleValueSourceName = TablesPossibleValueSourceMetaDataProvider.NAME, maxLength = 250, valueTooLongBehavior = ValueTooLongBehavior.ERROR)
-   private String tableName; // todo - qqqTableId... ?
+   private String tableName;
 
    @QField(maxLength = 250, valueTooLongBehavior = ValueTooLongBehavior.ERROR, dynamicDefaultValueBehavior = DynamicDefaultValueBehavior.USER_ID)
    private String userId;

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/processes/implementations/savedreports/RenderSavedReportExecuteStep.java

@@ -55,10 +55,11 @@ import com.kingsrook.qqq.backend.core.model.savedreports.RenderedReportStatus;
 import com.kingsrook.qqq.backend.core.model.savedreports.SavedReport;
 import com.kingsrook.qqq.backend.core.utils.ExceptionUtils;
 import com.kingsrook.qqq.backend.core.utils.StringUtils;
+import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair;
 
 
 /*******************************************************************************
- **
+ ** Process step to actually execute rendering a saved report.
  *******************************************************************************/
 public class RenderSavedReportExecuteStep implements BackendStep
 {
@@ -86,6 +87,7 @@ public class RenderSavedReportExecuteStep implements BackendStep
          String       storageReference     = LocalDate.now() + "/" + LocalTime.now().toString().replaceAll(":", "").replaceFirst("\\..*", "") + "/" + UUID.randomUUID() + "/" + downloadFileBaseName + "." + reportFormat.getExtension();
          OutputStream outputStream         = new StorageAction().createOutputStream(new StorageInput(storageTableName).withReference(storageReference));
 
+         LOG.info("Starting to render a report", logPair("savedReportId", savedReport.getId()), logPair("tableName", savedReport.getTableName()), logPair("storageReference", storageReference));
          runBackendStepInput.getAsyncJobCallback().updateStatus("Generating Report");
 
          //////////////////////////////////////////////////////////////////
@@ -133,6 +135,7 @@ public class RenderSavedReportExecuteStep implements BackendStep
          runBackendStepOutput.addValue("downloadFileName", downloadFileBaseName + "." + reportFormat.getExtension());
          runBackendStepOutput.addValue("storageTableName", storageTableName);
          runBackendStepOutput.addValue("storageReference", storageReference);
+         LOG.info("Completed rendering a report", logPair("savedReportId", savedReport.getId()), logPair("tableName", savedReport.getTableName()), logPair("storageReference", storageReference), logPair("rowCount", reportOutput.getTotalRecordCount()));
       }
       catch(Exception e)
       {
@@ -167,7 +170,10 @@ public class RenderSavedReportExecuteStep implements BackendStep
          downloadFileBaseName = report.getLabel();
       }
 
-      downloadFileBaseName = downloadFileBaseName.replaceAll("/", "-");
+      //////////////////////////////////////////////////
+      // these chars have caused issues, so, disallow //
+      //////////////////////////////////////////////////
+      downloadFileBaseName = downloadFileBaseName.replaceAll("/", "-").replaceAll(",", "_");
 
       return (downloadFileBaseName + " - " + datePart);
    }

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/processes/implementations/savedreports/RenderSavedReportMetaDataProducer.java

@@ -41,7 +41,7 @@ import com.kingsrook.qqq.backend.core.model.savedreports.SavedReport;
 
 
 /*******************************************************************************
- **
+ ** define process for rendering saved reports!
  *******************************************************************************/
 public class RenderSavedReportMetaDataProducer implements MetaDataProducerInterface<QProcessMetaData>
 {
@@ -79,7 +79,6 @@ public class RenderSavedReportMetaDataProducer implements MetaDataProducerInterf
             .withInputData(new QFunctionInputMetaData().withRecordListMetaData(new QRecordListMetaData()
                .withTableName(SavedReport.TABLE_NAME)))
             .withCode(new QCodeReference(RenderSavedReportExecuteStep.class)))
-         // todo - no no, stream the damn thing... how to do that??
          .addStep(new QFrontendStepMetaData()
             .withName("output")
             .withComponent(new QFrontendComponentMetaData().withType(QComponentType.DOWNLOAD_FORM)));

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/processes/implementations/savedreports/RenderSavedReportPreStep.java

@@ -36,7 +36,9 @@ import com.kingsrook.qqq.backend.core.utils.StringUtils;
 
 
 /*******************************************************************************
- **
+ ** initial backend-step before rendering a saved report.  does some basic
+ ** validations, and then (in future) will set up input fields (how??) for the
+ ** input screen.
  *******************************************************************************/
 public class RenderSavedReportPreStep implements BackendStep
 {

qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/processes/implementations/savedreports/SavedReportToReportMetaDataAdapter.java

@@ -59,7 +59,10 @@ import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair;
 
 
 /*******************************************************************************
+ ** GenerateReportAction takes in ReportMetaData.
  **
+ ** This class knows how to adapt from a SavedReport to a ReportMetaData, so that
+ ** we can render a saved report.
  *******************************************************************************/
 public class SavedReportToReportMetaDataAdapter
 {
@@ -99,7 +102,7 @@ public class SavedReportToReportMetaDataAdapter
          view.setName("main");
          view.setType(ReportType.TABLE);
          view.setDataSourceName(dataSource.getName());
-         view.setLabel(savedReport.getLabel()); // todo eh?
+         view.setLabel(savedReport.getLabel());
          view.setIncludeHeaderRow(true);
 
          ///////////////////////////////////////////////////////////////////////////////////////////////

qqq-backend-module-filesystem/src/main/java/com/kingsrook/qqq/backend/module/filesystem/s3/actions/S3StorageAction.java

@@ -39,7 +39,7 @@ import com.kingsrook.qqq.backend.module.filesystem.s3.utils.S3UploadOutputStream
 
 
 /*******************************************************************************
- ** (mass, streamed) storage action for filesystem module
+ ** (mass, streamed) storage action for s3 module
  *******************************************************************************/
 public class S3StorageAction extends AbstractS3Action implements QStorageInterface
 {

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

@@ -43,7 +43,8 @@ import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair;
 /*******************************************************************************
  ** OutputStream implementation that knows how to stream data into a new S3 file.
  **
- ** This will be done using a multipart-upload if the contents are > 5MB.
+ ** This will be done using a multipart-upload if the contents are > 5MB - else
+ ** just a 1-time-call to PutObject
  *******************************************************************************/
 public class S3UploadOutputStream extends OutputStream
 {