apply code review comments

Author: mprins

src/main/java/org/tailormap/api/controller/LayerExtractController.java

@@ -69,7 +69,7 @@ public LayerExtractController(
 
   /**
    * Download the result of an extract request. The extract generation should be initiated first by a POST to
-   * {@code /{viewerKind}/{viewerName}/layer/{appLayerId}/extract}.
+   * {@code /{viewerKind}/{viewerName}/layer/{appLayerId}/extract/{clientId}}.
    */
   @GetMapping(path = "/download/{downloadId}")
   @Counted(value = "tailormap_api_extract_download", description = "Count of layer extract downloads")
@@ -119,7 +119,9 @@ public ResponseEntity<?> formats(
       @ModelAttribute GeoService service,
       @ModelAttribute Application application,
       @ModelAttribute AppTreeLayerNode appTreeLayerNode) {
-    return ResponseEntity.ok(allowedExtractOutputFormats);
+    return ResponseEntity.ok(allowedExtractOutputFormats.stream()
+        .map(ExtractOutputFormat::getValue)
+        .toList());
   }
 
   @Transactional

src/main/java/org/tailormap/api/service/CreateLayerExtractService.java

@@ -159,8 +159,9 @@ public void validateClientId(@NonNull String clientId) throws IllegalArgumentExc
   /**
    * Create a validated filename for an extract. The naming follows the pattern
    * {@code "%s_%s_%s.%s".formatted(sourceFT.getName(), clientId, UUIDv7.randomV7(), outputFormat.getExtension()) }
-   * where the first part is the source feature type name, the second part is the SSE client id, the third part is a
-   * random UUIDv7 and the fourth part is the file extension based on the requested output format.
+   * where the first part is the source feature type name (this is cleaned from some characters), the second part is
+   * the SSE client id, the third part is a random UUIDv7 and the fourth part is the file extension based on the
+   * requested output format.
    *
    * @param clientId the SSE client id
    * @param sourceFT the source featuretype for the extract
@@ -180,6 +181,9 @@ public String createExtractFilename(
     if (cleanFTName.contains(":")) {
       // clip off the WFS namespace part
       cleanFTName = cleanFTName.substring(cleanFTName.lastIndexOf(":") + 1);
+      // remove: . _ which are used as separators in the filename and could cause issues when parsing the filename
+      // later on
+      cleanFTName = cleanFTName.replaceAll("[._]", "");
     }
     return "%s_%s_%s.%s".formatted(cleanFTName, clientId, UUIDv7.randomV7(), outputFormat.getExtension());
   }
@@ -197,13 +201,16 @@ public void createLayerExtract(
       @NonNull String outputFileName) {
     SimpleFeatureSource inputFeatureSource = null;
 
-    this.emitProgress(clientId, outputFileName, 0, false, null);
+    this.emitProgress(clientId, outputFileName, 0, false, "Starting extract");
 
     try (Transaction outputTransaction = new DefaultTransaction("tailormap-extract-output")) {
       inputFeatureSource = featureSourceFactoryHelper.openGeoToolsFeatureSource(inputTmFeatureType);
 
       Query q = new Query(inputFeatureSource.getName().toString());
-      q.setPropertyNames(attributes.toArray(new String[0]));
+      if (!attributes.isEmpty()) {
+        q.setPropertyNames(attributes.toArray(new String[0]));
+      }
+
       if (!StringUtils.isBlank(filterCQL)) {
         Filter filter = ECQL.toFilter(filterCQL);
         q.setFilter(filter);
@@ -213,8 +220,8 @@ public void createLayerExtract(
       }
 
       final int featCount = inputFeatureSource.getCount(q);
-      AtomicInteger featsAdded = new AtomicInteger();
       logger.debug("Filtered source counts {}", featCount);
+      final AtomicInteger featsAdded = new AtomicInteger();
 
       FileDataStore outputDataStore = getExtractDataStore(extractOutputFormat, outputFileName, clientId);
       SimpleFeatureType fType =
@@ -245,7 +252,7 @@ public void createLayerExtract(
         logger.error("Output datastore is not a SimpleFeatureStore, cannot write features");
       }
       outputDataStore.dispose();
-      this.emitProgress(clientId, outputFileName, 100, true, null);
+      this.emitProgress(clientId, outputFileName, 100, true, "Extract completed successfully");
     } catch (IOException | CQLException | SchemaException e) {
       emitError(clientId, e.getMessage());
       logger.error("Creating extract failed", e);

src/main/resources/application.properties

@@ -32,7 +32,7 @@ tailormap-api.feature.info.maxitems=30
 # deprecated
 tailormap-api.export.allowed-outputformats=csv,text/csv,application/vnd.openxmlformats-officedocument.spreadsheetml.sheet,excel2007,application/vnd.shp,application/x-zipped-shp,SHAPE-ZIP,application/geopackage+sqlite3,application/x-gpkg,geopackage,geopkg,gpkg,application/geo+json,application/geojson,application/json,json,DXF-ZIP
 
-# see org.tailormap.api.controller.LayerExtractController.ExtractOutputFormattFormat for valid values
+# see org.tailormap.api.controller.LayerExtractController.ExtractOutputFormat for valid values
 tailormap-api.extract.allowed-outputformats=csv,geojson,xlsx,shape
 # any files older than this (in minutes) in the extract output directory will be deleted by a scheduled job, to prevent filling up the disk
 # tailormap-api.extract.cleanup-minutes=120

src/main/resources/openapi/status-responses.yaml

@@ -81,14 +81,14 @@ components:
           type: object
           nullable: true
         id:
-          description: 'optional event identifier, can be used for event ordering and deduplication'
+          description: 'optional event identifier (UUIDv7), can be used for event ordering and deduplication'
           type: string
           format: uuid
           nullable: true
       example:
-        eventType: 'extract-started'
+        eventType: 'extract-progress'
         details:
-          status: 'started'
           message: 'Extracting data'
           progress: 17
+          downloadId: '123e4567-e89b-12d3-a456-426614174001.csv'
         id: '123e4567-e89b-12d3-a456-426614174000'

src/main/resources/openapi/viewer-api.yaml

@@ -1820,7 +1820,7 @@ paths:
           in: path
           required: true
           description: 'A client-generated identifier; 
-            this must be the same as the clientId used in eg. the `/extract` request to correlate the events with the extract request.
+            this must be the same as the clientId used in eg. the `/{viewerKind}/{viewerName}/layer/{appLayerId}/extract/{clientId}` request to correlate the events with the extract request.
              The format should use the "Nano ID" format, for example `V1StGXR8_Z5jdHi6B-myT`.'
           schema:
             type: string
@@ -1871,7 +1871,7 @@ paths:
                 type: array
                 items:
                   type: string
-                example: '["csv","shape.zip"]'
+                example: '["csv","shape"]'
 
   /{viewerKind}/{viewerName}/layer/{appLayerId}/extract/{clientId}:
     description: 'Export the attributes as shown in the attribute list for a layer.'
@@ -1944,8 +1944,8 @@ paths:
       responses:
         '202':
           description: 'Export started/queued. The client should listen to the `/events/` stream and wait for an 
-          `extract-completed` event with `status: completed` to know when the export is finished and the file is ready 
-          to be downloaded. If the connection is closed before that, the export may be cancelled.'
+          `extract-completed` event with `message: Extract task completed` to know when the export is finished and the 
+          file is ready to be downloaded. If the connection is closed before that, the export may be cancelled.'
           content:
             application/json:
               schema:
@@ -1957,12 +1957,7 @@ paths:
                 required:
                   - message
               example:
-                eventType: 'extract-started'
-                id: '123e4567-e89b-12d3-a456-426614174000'
-                details:
-                  status: started
-                  message: 'Extracting data'
-                  progress: 50
+                  message: 'Extract request accepted'
                   downloadId: '123e4567-e89b-12d3-a456-426614174001.csv'
         '400':
           description: 'Bad Request. May be returned for some combination of parameters that can not be processed or are incomplete.'

src/test/java/org/tailormap/api/controller/LayerExtractControllerIntegrationTest.java

@@ -228,7 +228,7 @@ void should_export_wfs_to_csv_with_authentication() throws Exception {
     final String extractedDownloadId = getDownloadId(lastCompletedEventJson);
     assertThat(extractedDownloadId, containsString(".csv"));
 
-    final String downloadUrl = apiBasePath + layerBegroeidTerreindeelPostgis + downloadPath + extractedDownloadId;
+    final String downloadUrl = apiBasePath + layerProxiedWithAuthInPublicApp + downloadPath + extractedDownloadId;
     MvcResult download = mockMvc.perform(get(downloadUrl).with(setServletPath(downloadUrl)))
         .andExpect(status().isOk())
         .andExpect(result -> {

src/test/java/org/tailormap/api/controller/LayerExtractControllerRestrictedFormatsIntegrationTest.java

@@ -12,6 +12,7 @@
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
 import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.request;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
 import static org.tailormap.api.TestRequestProcessor.setServletPath;
 import static org.tailormap.api.controller.TestUrls.layerBegroeidTerreindeelPostgis;
@@ -52,7 +53,15 @@ void list_supported_formats() throws Exception {
 
   @Test
   void invalid_output_format_should_return_bad_request_on_extract() throws Exception {
-    final String validClientId = "format-test-" + System.nanoTime();
+    final String validClientId = "invalid_output_format-" + System.nanoTime();
+    final String sseUrl = apiBasePath + "/events/" + validClientId;
+    mockMvc.perform(get(sseUrl)
+            .accept(MediaType.TEXT_EVENT_STREAM)
+            .with(setServletPath(sseUrl))
+            .acceptCharset(StandardCharsets.UTF_8))
+        .andExpect(request().asyncStarted())
+        .andReturn();
+
     final String extractUrl = apiBasePath + layerBegroeidTerreindeelPostgis + extractPath + validClientId;
     mockMvc.perform(post(extractUrl)
             .accept(MediaType.APPLICATION_JSON)

src/test/resources/application.properties

@@ -4,7 +4,7 @@ management.endpoints.web.base-path=/api/actuator
 tailormap-api.new-admin-username=tm-admin
 # deprecated
 tailormap-api.export.allowed-outputformats=application/geopackage+sqlite3,application/json
-# see org.tailormap.api.controller.LayerExtractController.ExtractOutputFormattFormat for valid values
+# see org.tailormap.api.controller.LayerExtractController.ExtractOutputFormat for valid values
 tailormap-api.extract.allowed-outputformats=csv,geojson,xlsx,shape
 # any files older than this (in minutes) in the extract output directory will be deleted by a scheduled job, to prevent filling up the disk
 # tailormap-api.extract.cleanup-minutes=120