Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: write empty string to CSV/TSV for null values #724

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
package org.genspectrum.lapis.controller

import com.fasterxml.jackson.annotation.JsonIgnore
import org.apache.commons.csv.CSVFormat
import org.apache.commons.csv.CSVPrinter
import org.springframework.stereotype.Component
import java.io.StringWriter

interface CsvRecord {
fun asArray(): Array<String>
@JsonIgnore
fun getValuesList(): List<String?>

@JsonIgnore
fun getHeader(): Array<String>
}

Expand All @@ -24,6 +27,7 @@ class CsvWriter {
CSVFormat.DEFAULT.builder()
.setRecordSeparator("\n")
.setDelimiter(delimiter.value)
.setNullString("")
.also {
when {
headers != null -> it.setHeader(*headers)
Expand All @@ -32,10 +36,10 @@ class CsvWriter {
.build(),
).use {
for (datum in data) {
it.printRecord(*datum.asArray())
it.printRecord(datum.getValuesList())
}
}
return stringWriter.toString().trim()
return stringWriter.toString().trimEnd('\n')
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.genspectrum.lapis.response

import com.fasterxml.jackson.annotation.JsonIgnore
import io.swagger.v3.oas.annotations.media.Schema
import org.genspectrum.lapis.controller.CsvRecord

Expand All @@ -23,9 +22,8 @@ data class NucleotideMutationResponse(
"given filter criteria with non-ambiguous reads at that position",
) val proportion: Double,
) : CsvRecord {
override fun asArray() = arrayOf(mutation, count.toString(), proportion.toString())
override fun getValuesList() = listOf(mutation, count.toString(), proportion.toString())

@JsonIgnore
override fun getHeader() = arrayOf("mutation", "count", "proportion")
}

Expand All @@ -45,9 +43,8 @@ data class AminoAcidMutationResponse(
"given filter criteria with non-ambiguous reads at that position",
) val proportion: Double,
) : CsvRecord {
override fun asArray() = arrayOf(mutation, count.toString(), proportion.toString())
override fun getValuesList() = listOf(mutation, count.toString(), proportion.toString())

@JsonIgnore
override fun getHeader() = arrayOf("mutation", "count", "proportion")
}

Expand All @@ -65,9 +62,8 @@ data class NucleotideInsertionResponse(
)
val count: Int,
) : CsvRecord {
override fun asArray() = arrayOf(insertion, count.toString())
override fun getValuesList() = listOf(insertion, count.toString())

@JsonIgnore
override fun getHeader() = arrayOf("insertion", "count")
}

Expand All @@ -84,8 +80,7 @@ data class AminoAcidInsertionResponse(
)
val count: Int,
) : CsvRecord {
override fun asArray() = arrayOf(insertion, count.toString())
override fun getValuesList() = listOf(insertion, count.toString())

@JsonIgnore
override fun getHeader() = arrayOf("insertion", "count")
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import com.fasterxml.jackson.databind.JsonDeserializer
import com.fasterxml.jackson.databind.JsonNode
import com.fasterxml.jackson.databind.JsonSerializer
import com.fasterxml.jackson.databind.SerializerProvider
import com.fasterxml.jackson.databind.node.NullNode
import io.swagger.v3.oas.annotations.media.Schema
import org.genspectrum.lapis.config.DatabaseConfig
import org.genspectrum.lapis.controller.CsvRecord
Expand All @@ -19,17 +20,26 @@ data class AggregationData(
val count: Int,
@Schema(hidden = true) val fields: Map<String, JsonNode>,
) : CsvRecord {
override fun asArray() = fields.values.map { it.asText() }.plus(count.toString()).toTypedArray()
override fun getValuesList() =
fields.values
.map { it.toCsvValue() }
.plus(count.toString())

override fun getHeader() = fields.keys.plus(COUNT_PROPERTY).toTypedArray()
}

data class DetailsData(val map: Map<String, JsonNode>) : Map<String, JsonNode> by map, CsvRecord {
override fun asArray() = values.map { it.asText() }.toTypedArray()
override fun getValuesList() = values.map { it.toCsvValue() }

override fun getHeader() = keys.toTypedArray()
}

private fun JsonNode.toCsvValue() =
when (this) {
is NullNode -> null
else -> asText()
}

@JsonComponent
class DetailsDataDeserializer : JsonDeserializer<DetailsData>() {
override fun deserialize(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
package org.genspectrum.lapis.controller

import com.fasterxml.jackson.databind.node.NullNode
import com.fasterxml.jackson.databind.node.TextNode
import com.ninjasquad.springmockk.MockkBean
import io.mockk.every
import org.genspectrum.lapis.controller.LapisMediaType.TEXT_CSV
import org.genspectrum.lapis.controller.LapisMediaType.TEXT_CSV_WITHOUT_HEADERS
import org.genspectrum.lapis.controller.LapisMediaType.TEXT_TSV
import org.genspectrum.lapis.model.SiloQueryModel
import org.genspectrum.lapis.request.LapisInfo
import org.genspectrum.lapis.response.AggregationData
import org.genspectrum.lapis.response.DetailsData
import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.Test
import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.MethodSource
import org.springframework.beans.factory.annotation.Autowired
Expand Down Expand Up @@ -133,6 +138,49 @@ class LapisControllerCsvTest(
.drop(1)
.joinToString("\n")

@Test
fun `GIVEN aggregated endpoint returns result with null values THEN CSV contains empty strings instead`() {
every { siloQueryModelMock.getAggregated(any()) } returns listOf(
AggregationData(
1,
mapOf("firstKey" to TextNode("someValue"), "keyWithNullValue" to NullNode.instance),
),
)

val expectedCsv = """
firstKey,keyWithNullValue,count
someValue,,1
""".trimIndent()

mockMvc.perform(getSample("/aggregated?country=Switzerland").header(ACCEPT, "text/csv"))
.andExpect(status().isOk)
.andExpect(header().string("Content-Type", "text/csv;charset=UTF-8"))
.andExpect(content().string(expectedCsv))
}

@Test
fun `GIVEN details endpoint returns result with null values THEN CSV contains empty strings instead`() {
every { siloQueryModelMock.getDetails(any()) } returns listOf(
DetailsData(
mapOf(
"firstKey" to TextNode("some first value"),
"keyWithNullValue" to NullNode.instance,
"someOtherKey" to TextNode("someValue"),
),
),
)

val expectedCsv = """
firstKey,keyWithNullValue,someOtherKey
some first value,,someValue
""".trimIndent()

mockMvc.perform(getSample("/details?country=Switzerland").header(ACCEPT, "text/csv"))
.andExpect(status().isOk)
.andExpect(header().string("Content-Type", "text/csv;charset=UTF-8"))
.andExpect(content().string(expectedCsv))
}

private companion object {
@JvmStatic
val endpoints = SampleRoute.entries.filter { !it.servesFasta }.map { it.pathSegment }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,12 +191,12 @@ object MockDataForEndpoints {
expectedCsv = """
country,age,floatValue
Switzerland,42,3.14
Switzerland,43,null
Switzerland,43,
""".trimIndent(),
expectedTsv = """
country age floatValue
Switzerland 42 3.14
Switzerland 43 null
Switzerland 43
""".trimIndent(),
)

Expand Down
4 changes: 2 additions & 2 deletions siloLapisTests/test/aggregated.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ describe('The /aggregated endpoint', () => {
expect(await result.text()).to.be.equal(
String.raw`
age,country,count
null,Switzerland,2
,Switzerland,2
4,Switzerland,2
5,Switzerland,1
6,Switzerland,1
Expand Down Expand Up @@ -183,7 +183,7 @@ null,Switzerland,2
expect(await result.text()).to.be.equal(
String.raw`
age country count
null Switzerland 2
Switzerland 2
4 Switzerland 2
5 Switzerland 1
6 Switzerland 1
Expand Down
Loading