fix: file ending when downloading compressed file #685

also fix that LAPIS now correctly respects multiple, comma-separated values in the accept-encoding header.

lapis2-docs/src/content/docs/references/additional-request-properties.mdx

@@ -92,7 +92,16 @@ GET /sample/aggregated?downloadAsFile=true
 ## Compression
 
 LAPIS supports gzip and Zstd compression.
-You can request compressed data via the `Accept-Encoding` header.
+You can request compressed data by setting the `compression` property in the request.
+Refer to the Swagger UI for allowed values.
+
+```http
+GET /sample/aggregated?compression=gzip
+```
+
+:::note
+
+Alternatively, you can set the `Accept-Encoding` header.
 
 ```http
 GET /sample/aggregated
@@ -106,12 +115,11 @@ Accept-Encoding: zstd
 
 LAPIS will set the `Content-Encoding` header in the response to indicate the compression used.
 
-:::note
-Alternatively, you can use the `compression` property in the request.
-Refer to the Swagger UI for allowed values.
-
-```http
-GET /sample/aggregated?compression=gzip
-```
+:::caution
+If you want to download compressed data via the `downloadAsFile` property,
+then you need to specify the compression type via the `compression` property.
 
+LAPIS will ignore the `Accept-Encoding` header, if `downloadAsFile == true`,
+since browsers always accept GZIP encoding.
+Then, you would not be able to download uncompressed data in a browser.
 :::

lapis2/src/main/kotlin/org/genspectrum/lapis/controller/CompressionFilter.kt

@@ -36,18 +36,21 @@ enum class Compression(
     val value: String,
     val contentType: MediaType,
     val compressionOutputStreamFactory: (OutputStream) -> OutputStream,
+    val fileEnding: String,
 ) {
     GZIP(
         value = "gzip",
         contentType = MediaType.parseMediaType("application/gzip"),
         compressionOutputStreamFactory = ::LazyGzipOutputStream,
+        fileEnding = ".gz",
     ),
     ZSTD(
         value = "zstd",
         contentType = MediaType.parseMediaType("application/zstd"),
         compressionOutputStreamFactory = {
             ZstdOutputStream(it).apply { commitUnderlyingResponseToPreventContentLengthFromBeingSet() }
         },
+        fileEnding = ".zstd",
     ),
     ;
 
@@ -58,6 +61,8 @@ enum class Compression(
             }
 
             val headersList = acceptEncodingHeaders.toList()
+                .flatMap { it.split(',') }
+                .map { it.trim() }
 
             return when {
                 headersList.contains(GZIP.value) -> GZIP
@@ -140,7 +145,7 @@ class CompressionFilter(val objectMapper: ObjectMapper, val requestCompression:
 
         val maybeCompressingResponse = createMaybeCompressingResponse(
             response,
-            reReadableRequest.getHeaders(ACCEPT_ENCODING),
+            reReadableRequest,
             compressionPropertyInRequest,
         )
 
@@ -162,9 +167,11 @@ class CompressionFilter(val objectMapper: ObjectMapper, val requestCompression:
 
     private fun createMaybeCompressingResponse(
         response: HttpServletResponse,
-        acceptEncodingHeaders: Enumeration<String>?,
+        reReadableRequest: CachedBodyHttpServletRequest,
         compressionPropertyInRequest: Compression?,
     ): HttpServletResponse {
+        val acceptEncodingHeaders = reReadableRequest.getHeaders(ACCEPT_ENCODING)
+
         if (compressionPropertyInRequest != null) {
             log.info { "Compressing using $compressionPropertyInRequest from request property" }
 
@@ -175,6 +182,11 @@ class CompressionFilter(val objectMapper: ObjectMapper, val requestCompression:
             )
         }
 
+        val downloadAsFile = reReadableRequest.getBooleanField(DOWNLOAD_AS_FILE_PROPERTY) ?: false
+        if (downloadAsFile) {
+            return response
+        }
+
         val compression = Compression.fromHeaders(acceptEncodingHeaders) ?: return response
 
         log.info { "Compressing using $compression from $ACCEPT_ENCODING header" }

lapis2/src/main/kotlin/org/genspectrum/lapis/controller/DownloadAsFileFilter.kt

@@ -9,14 +9,16 @@ import org.genspectrum.lapis.controller.LapisMediaType.TEXT_TSV
 import org.genspectrum.lapis.util.CachedBodyHttpServletRequest
 import org.springframework.core.annotation.Order
 import org.springframework.http.HttpHeaders.ACCEPT
-import org.springframework.http.HttpHeaders.ACCEPT_ENCODING
 import org.springframework.http.HttpHeaders.CONTENT_DISPOSITION
 import org.springframework.stereotype.Component
 import org.springframework.web.filter.OncePerRequestFilter
 
 @Component
 @Order(DOWNLOAD_AS_FILE_FILTER_ORDER)
-class DownloadAsFileFilter(private val objectMapper: ObjectMapper) : OncePerRequestFilter() {
+class DownloadAsFileFilter(
+    private val objectMapper: ObjectMapper,
+    private val requestCompression: RequestCompression,
+) : OncePerRequestFilter() {
     override fun doFilterInternal(
         request: HttpServletRequest,
         response: HttpServletResponse,
@@ -37,10 +39,9 @@ class DownloadAsFileFilter(private val objectMapper: ObjectMapper) : OncePerRequ
             SampleRoute.entries.find { request.getProxyAwarePath().startsWith("/sample${it.pathSegment}") }
         val dataName = matchingRoute?.pathSegment?.trim('/') ?: "data"
 
-        val compressionEnding = when (Compression.fromHeaders(request.getHeaders(ACCEPT_ENCODING))) {
-            Compression.GZIP -> ".gzip"
-            Compression.ZSTD -> ".zstd"
-            null -> ""
+        val compressionEnding = when (val compressionSource = requestCompression.compressionSource) {
+            is CompressionSource.RequestProperty -> compressionSource.compression.fileEnding
+            else -> ""
         }
 
         val fileEnding = when (request.getHeader(ACCEPT)) {

lapis2/src/test/kotlin/org/genspectrum/lapis/controller/LapisControllerCompressionTest.kt

@@ -7,6 +7,10 @@ import io.mockk.every
 import org.genspectrum.lapis.controller.LapisMediaType.TEXT_CSV
 import org.genspectrum.lapis.controller.LapisMediaType.TEXT_TSV
 import org.genspectrum.lapis.controller.LapisMediaType.TEXT_X_FASTA
+import org.genspectrum.lapis.controller.MockDataCollection.DataFormat.CSV
+import org.genspectrum.lapis.controller.MockDataCollection.DataFormat.NESTED_JSON
+import org.genspectrum.lapis.controller.MockDataCollection.DataFormat.PLAIN_JSON
+import org.genspectrum.lapis.controller.MockDataCollection.DataFormat.TSV
 import org.genspectrum.lapis.controller.SampleRoute.AGGREGATED
 import org.genspectrum.lapis.controller.SampleRoute.ALIGNED_AMINO_ACID_SEQUENCES
 import org.genspectrum.lapis.controller.SampleRoute.ALIGNED_NUCLEOTIDE_SEQUENCES
@@ -17,6 +21,7 @@ import org.hamcrest.MatcherAssert.assertThat
 import org.hamcrest.Matchers.containsString
 import org.hamcrest.Matchers.`is`
 import org.junit.jupiter.api.BeforeEach
+import org.junit.jupiter.api.Test
 import org.junit.jupiter.params.ParameterizedTest
 import org.junit.jupiter.params.provider.Arguments
 import org.junit.jupiter.params.provider.MethodSource
@@ -39,6 +44,9 @@ import java.util.zip.GZIPInputStream
 
 private const val INVALID_COMPRESSION_FORMAT = "invalidCompressionFormat"
 
+const val COMPRESSION_FORMAT_GZIP = "gzip"
+const val COMPRESSION_FORMAT_ZSTD = "zstd"
+
 @SpringBootTest
 @AutoConfigureMockMvc
 class LapisControllerCompressionTest(
@@ -132,6 +140,19 @@ class LapisControllerCompressionTest(
         assertThat(errorDetail, `is`(errorMessage))
     }
 
+    @Test
+    fun `GIVEN multiple values in accept encoding header THEN it should return compressed data`() {
+        val mockData = MockDataForEndpoints.getMockData(AGGREGATED.pathSegment).expecting(PLAIN_JSON)
+        mockData.mockWithData(siloQueryModelMock)
+
+        val acceptEncodingAsBrowsersSendIt = "$COMPRESSION_FORMAT_GZIP, br, deflate"
+
+        mockMvc.perform(getSample(AGGREGATED.pathSegment).header(ACCEPT_ENCODING, acceptEncodingAsBrowsersSendIt))
+            .andExpect(status().isOk)
+            .andExpect(content().contentType(APPLICATION_JSON))
+            .andExpect(header().string(CONTENT_ENCODING, COMPRESSION_FORMAT_GZIP))
+    }
+
     private fun decompressContent(
         response: MvcResult,
         compressionFormat: String,
@@ -165,34 +186,39 @@ class LapisControllerCompressionTest(
                 .flatMap {
                     getRequests(
                         endpoint = it,
-                        dataFormat = MockDataCollection.DataFormat.CSV,
-                        compressionFormat = "gzip",
+                        dataFormat = CSV,
+                        compressionFormat = COMPRESSION_FORMAT_GZIP,
                     ) +
                         getRequests(
                             endpoint = it,
-                            dataFormat = MockDataCollection.DataFormat.CSV,
-                            compressionFormat = "zstd",
+                            dataFormat = CSV,
+                            compressionFormat = COMPRESSION_FORMAT_ZSTD,
                         )
                 } +
                 getRequests(
                     AGGREGATED,
-                    dataFormat = MockDataCollection.DataFormat.NESTED_JSON,
-                    compressionFormat = "gzip",
+                    dataFormat = NESTED_JSON,
+                    compressionFormat = COMPRESSION_FORMAT_GZIP,
                 ) +
                 getRequests(
                     AGGREGATED,
-                    dataFormat = MockDataCollection.DataFormat.TSV,
-                    compressionFormat = "zstd",
+                    dataFormat = TSV,
+                    compressionFormat = COMPRESSION_FORMAT_ZSTD,
                 ) +
                 listOf(
                     "${UNALIGNED_NUCLEOTIDE_SEQUENCES.pathSegment}/main",
                     "${ALIGNED_NUCLEOTIDE_SEQUENCES.pathSegment}/main",
                     "${ALIGNED_AMINO_ACID_SEQUENCES.pathSegment}/gene1",
                 )
-                    .flatMap { getFastaRequests(it, "gzip") + getFastaRequests(it, "zstd") }
+                    .flatMap {
+                        getFastaRequests(it, COMPRESSION_FORMAT_GZIP) + getFastaRequests(
+                            it,
+                            COMPRESSION_FORMAT_ZSTD,
+                        )
+                    }
 
         @JvmStatic
-        val compressionFormats = listOf("gzip", "zstd")
+        val compressionFormats = listOf(COMPRESSION_FORMAT_GZIP, COMPRESSION_FORMAT_ZSTD)
     }
 }
 
@@ -306,17 +332,17 @@ private fun getFastaRequests(
     ),
 )
 
-private fun getContentTypeForCompressionFormat(compressionFormat: String) =
+fun getContentTypeForCompressionFormat(compressionFormat: String) =
     when (compressionFormat) {
-        "gzip" -> "application/gzip"
-        "zstd" -> "application/zstd"
+        COMPRESSION_FORMAT_GZIP -> "application/gzip"
+        COMPRESSION_FORMAT_ZSTD -> "application/zstd"
         else -> throw Exception("Test issue: unknown compression format $compressionFormat")
     }
 
 private fun getContentTypeForDataFormat(dataFormat: MockDataCollection.DataFormat) =
     when (dataFormat) {
-        MockDataCollection.DataFormat.PLAIN_JSON -> APPLICATION_JSON_VALUE
-        MockDataCollection.DataFormat.NESTED_JSON -> APPLICATION_JSON_VALUE
-        MockDataCollection.DataFormat.CSV -> "$TEXT_CSV;charset=UTF-8"
-        MockDataCollection.DataFormat.TSV -> "$TEXT_TSV;charset=UTF-8"
+        PLAIN_JSON -> APPLICATION_JSON_VALUE
+        NESTED_JSON -> APPLICATION_JSON_VALUE
+        CSV -> "$TEXT_CSV;charset=UTF-8"
+        TSV -> "$TEXT_TSV;charset=UTF-8"
     }

lapis2/src/test/kotlin/org/genspectrum/lapis/controller/LapisControllerDownloadAsFileTest.kt

@@ -2,6 +2,7 @@ package org.genspectrum.lapis.controller
 
 import com.ninjasquad.springmockk.MockkBean
 import io.mockk.every
+import org.genspectrum.lapis.controller.MockDataCollection.DataFormat.PLAIN_JSON
 import org.genspectrum.lapis.controller.SampleRoute.AGGREGATED
 import org.genspectrum.lapis.controller.SampleRoute.ALIGNED_AMINO_ACID_SEQUENCES
 import org.genspectrum.lapis.controller.SampleRoute.ALIGNED_NUCLEOTIDE_SEQUENCES
@@ -16,11 +17,14 @@ import org.genspectrum.lapis.request.LapisInfo
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.params.ParameterizedTest
 import org.junit.jupiter.params.provider.MethodSource
+import org.junit.jupiter.params.provider.ValueSource
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc
 import org.springframework.boot.test.context.SpringBootTest
 import org.springframework.http.HttpHeaders.ACCEPT
 import org.springframework.http.HttpHeaders.ACCEPT_ENCODING
+import org.springframework.http.HttpHeaders.CONTENT_DISPOSITION
+import org.springframework.http.HttpHeaders.CONTENT_TYPE
 import org.springframework.http.MediaType.APPLICATION_JSON
 import org.springframework.test.web.servlet.MockMvc
 import org.springframework.test.web.servlet.ResultActions
@@ -92,8 +96,27 @@ class LapisControllerDownloadAsFileTest(
 
         mockMvc.perform(scenario.request)
             .andExpect(status().isOk)
-            .andExpect(header().string("Content-Disposition", attachmentWithFilename(scenario.expectedFilename)))
-            .andExpect(header().string("Content-Encoding", scenario.compressionFormat))
+            .andExpect(header().string(CONTENT_DISPOSITION, attachmentWithFilename(scenario.expectedFilename)))
+            .andExpect(header().string(CONTENT_TYPE, scenario.expectedContentType))
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = [COMPRESSION_FORMAT_GZIP, COMPRESSION_FORMAT_ZSTD, "$COMPRESSION_FORMAT_GZIP, br, deflate"])
+    fun `WHEN I request data as file and accept encoding THEN it should return uncompressed file`(
+        acceptEncodingHeader: String,
+    ) {
+        val mockData = MockDataForEndpoints.getMockData(AGGREGATED.pathSegment).expecting(PLAIN_JSON)
+        mockData.mockWithData(siloQueryModelMock)
+
+        mockMvc.perform(
+            getSample("${AGGREGATED.pathSegment}?$DOWNLOAD_AS_FILE_PROPERTY=true")
+                .header(ACCEPT_ENCODING, acceptEncodingHeader),
+        )
+            .andExpect(status().isOk)
+            .andExpectAttachmentWithContent(
+                expectedFilename = "aggregated.json",
+                assertFileContentMatches = mockData.assertDataMatches,
+            )
     }
 
     private fun ResultActions.andExpectAttachmentWithContent(
@@ -115,7 +138,7 @@ class LapisControllerDownloadAsFileTest(
 
         private val dataFormatsSequence = generateSequence {
             listOf(
-                MockDataCollection.DataFormat.PLAIN_JSON,
+                PLAIN_JSON,
                 MockDataCollection.DataFormat.CSV,
                 MockDataCollection.DataFormat.TSV,
             )
@@ -177,7 +200,7 @@ data class DownloadAsFileScenario(
         ) = listOf(
             DownloadAsFileScenario(
                 mockData = MockDataForEndpoints.getMockData(endpoint)
-                    .expecting(MockDataCollection.DataFormat.PLAIN_JSON),
+                    .expecting(PLAIN_JSON),
                 expectedFilename = "$expectedFilename.json",
                 endpoint = endpoint,
                 requestedDataFormat = "json",
@@ -203,7 +226,7 @@ data class DownloadCompressedFileScenario(
     val mockData: MockData,
     val request: MockHttpServletRequestBuilder,
     val expectedFilename: String,
-    val compressionFormat: String,
+    val expectedContentType: String,
 ) {
     override fun toString() = description
 
@@ -214,12 +237,12 @@ data class DownloadCompressedFileScenario(
         ) = scenariosFor(
             dataFormat = dataFormat,
             route = route,
-            compressionFormat = "gzip",
+            compressionFormat = COMPRESSION_FORMAT_GZIP,
         ) +
             scenariosFor(
                 dataFormat = dataFormat,
                 route = route,
-                compressionFormat = "zstd",
+                compressionFormat = COMPRESSION_FORMAT_ZSTD,
             )
 
         private fun scenariosFor(
@@ -244,27 +267,35 @@ data class DownloadCompressedFileScenario(
                 false -> dataFormat.acceptHeader
             }
 
-            val expectedFilename = "${route.getExpectedFilename()}.$dataFileFormat.$compressionFormat"
+            val fileEnding = when (compressionFormat) {
+                COMPRESSION_FORMAT_ZSTD -> "zstd"
+                COMPRESSION_FORMAT_GZIP -> "gz"
+                else -> throw Exception("Test issue: unknown compression format $compressionFormat")
+            }
+            val expectedFilename = "${route.getExpectedFilename()}.$dataFileFormat.$fileEnding"
+            val expectedContentType = getContentTypeForCompressionFormat(compressionFormat)
 
             return listOf(
                 DownloadCompressedFileScenario(
                     description = "GET $endpoint as $compressionFormat ${dataFormat.fileFormat}",
                     mockData = mockData,
-                    request = getSample("$endpoint?$DOWNLOAD_AS_FILE_PROPERTY=true")
-                        .header(ACCEPT_ENCODING, compressionFormat)
+                    request = getSample(
+                        "$endpoint?$DOWNLOAD_AS_FILE_PROPERTY=true&$COMPRESSION_PROPERTY=$compressionFormat",
+                    )
                         .header(ACCEPT, acceptHeader),
                     expectedFilename = expectedFilename,
-                    compressionFormat = compressionFormat,
+                    expectedContentType = expectedContentType,
                 ),
                 DownloadCompressedFileScenario(
                     description = "POST $endpoint as $compressionFormat ${dataFormat.fileFormat}",
                     mockData = mockData,
-                    request = postSample(endpoint).content("""{ "$DOWNLOAD_AS_FILE_PROPERTY": true }""")
+                    request = postSample(endpoint).content(
+                        """{ "$DOWNLOAD_AS_FILE_PROPERTY": true, "$COMPRESSION_PROPERTY": "$compressionFormat" }""",
+                    )
                         .contentType(APPLICATION_JSON)
-                        .header(ACCEPT_ENCODING, compressionFormat)
                         .header(ACCEPT, acceptHeader),
                     expectedFilename = expectedFilename,
-                    compressionFormat = compressionFormat,
+                    expectedContentType = expectedContentType,
                 ),
             )
         }