Skip to content

Suggestion: Use Seq[T] instead of Option[Seq[T]] in UploadDtos #348

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.

Sign up for GitHub

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

Open
ggVGc wants to merge 6 commits into
base: jonathanthiry/l3-variables
Choose a base branch
from
Open

Suggestion: Use Seq[T] instead of Option[Seq[T]] in UploadDtos #348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b95dda0
Simplify varnames from Option[Seq] to just Seq
ggVGc Feb 24, 2026
a55f960
Preserve original behaviour of empty string when empty list
ggVGc Feb 24, 2026
05e8152
Change SpatioTemporalDto.variables from Option[Seq] to Seq
ggVGc Feb 25, 2026
2e52fb4
Change SpatioTemporalMeta.variables from Option[Seq] to Seq
ggVGc Feb 25, 2026
6e4271f
Change StationTimeSeriesMeta.columns from Option[Seq] to Seq
ggVGc Feb 25, 2026
8de9db9
Improve diff
ggVGc Feb 25, 2026
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
13 changes: 7 additions & 6 deletions core/src/main/scala/se/lu/nateko/cp/meta/core/data/DataObject.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ case class StationTimeSeriesMeta(
productionInfo: Option[DataProduction],
nRows: Option[Int],
coverage: Option[GeoFeature],
columns: Option[Seq[VarMeta]]
columns: Seq[VarMeta]
)

case class ValueType(self: UriResource, quantityKind: Option[UriResource], unit: Option[String])
Expand All @@ -108,7 +108,7 @@ case class SpatioTemporalMeta(
station: Option[Station],
samplingHeight: Option[Float],
productionInfo: DataProduction,
variables: Option[Seq[VarMeta]]
variables: Seq[VarMeta]
){
def acquisition: Option[DataAcquisition] = station.map{
DataAcquisition(_, None, Some(temporal.interval), None, None, samplingHeight)
Expand Down Expand Up @@ -164,11 +164,12 @@ case class DataObject(
l2 => l2.coverage.orElse(l2.acquisition.coverage)
)

val cols = specificInfo.fold(
l3 => l3.variables,
l2 => l2.columns
)

val varsAndPosits = for
cols <- specificInfo.fold(
l3 => l3.variables,
l2 => l2.columns
).toSeq
col <- cols
deps <- col.instrumentDeployments.toSeq
dep <- deps
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/se/lu/nateko/cp/meta/UploadDtos.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ case class SpatioTemporalDto(
forStation: Option[URI],
samplingHeight: Option[Float],
customLandingPage: Option[URI],
variables: Option[Seq[String]]
variables: Seq[String]
)

case class DataProductionDto(
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/se/lu/nateko/cp/meta/services/UploadDtoReader.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ object UploadDtoReader{
samplingHeight = l3.samplingHeight,
production = dataProductionToDto(l3.productionInfo),
customLandingPage = dobj.accessUrl.filterNot(uri => uri.getPath.endsWith(dobj.hash.id)),
variables = l3.variables.map(_.map(_.model.uri.toString.split('/').last))
variables = l3.variables.map(_.model.uri.toString.split('/').last)
))
case Right(l2) => Right(StationTimeSeriesDto(
station = l2.acquisition.station.org.self.uri,
Expand Down
5 changes: 2 additions & 3 deletions src/main/scala/se/lu/nateko/cp/meta/services/citation/AttributionProvider.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,9 @@ final class AttributionProvider(vocab: CpVocab, val metaVocab: CpmetaVocab) exte
if(dobj.specification.theme.self.uri === vocab.atmoTheme) memb => (memb.role.weight.isDefined && {
val speciesOk = for(
extra <- memb.role.extra;
l2 <- dobj.specificInfo.toOption;
cols <- l2.columns
l2 <- dobj.specificInfo.toOption
Copy link
Copy Markdown
Contributor Author

@ggVGc ggVGc Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note, but this usage of Either.toOption is why I don't really like the Either type in general. It signifies that one value (the Left one) is more important or correct than the other one. So, in that sense it is essentially an "Option with an error message".
But, our usage of it here does not actually carry that meaning.
So, I would prefer a compound type like SpatioTemporalMeta | StationTimeSeriesMeta instead, and that the uses of Either.fold be replaced by just pattern matches.

Copy link
Copy Markdown
Contributor

@jonathanthiry jonathanthiry Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like it could be a nice improvement. I cannot think of a reason why we are using Either other than union types didn't exist when this code was written.

) yield{
val colLabels = cols.map(_.label.toLowerCase)
val colLabels = l2.columns.map(_.label.toLowerCase)
extra.split(',').map(_.trim.toLowerCase).exists(species =>
colLabels.exists(_.contains(species)) ||
dobj.specification.self.label.getOrElse("").toLowerCase.contains(species)
Expand Down
8 changes: 5 additions & 3 deletions src/main/scala/se/lu/nateko/cp/meta/services/citation/CitationMaker.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,11 @@ class CitationMaker(
val height = acq.samplingHeight.fold("")(sh => s" ($sh m)")
val vars =
if dobj.specification.self.uri === vocab.atmGhgProdSpec then
stationTs.columns.fold("")(_.collect{
case v if v.valueType.unit.isDefined => v.label
}.mkString(" (", ", ", ")"))
if (stationTs.columns.nonEmpty)
Copy link
Copy Markdown
Contributor Author

@ggVGc ggVGc Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code wants to produce an empty string if there is no content, and the .mkString(...) call would not do that with an empty list. This check preserves original behaviour.

stationTs.columns.collect{
case v if v.valueType.unit.isDefined => v.label
}.mkString(" (", ", ", ")")
else ""
else ""

s"$spec$vars from $station$height"
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/se/lu/nateko/cp/meta/services/metaexport/SchemaOrg.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ class SchemaOrg(handleProxies: HandleProxiesConfig)(using envri: Envri, envriCon
)
}

val variableMeasured = asOptArray(dobj.specificInfo.fold(_.variables, _.columns))(
val variableMeasured = asOptArray(dobj.specificInfo.fold((_.variables), _.columns))(
Copy link
Copy Markdown
Contributor Author

@ggVGc ggVGc Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON serialization is one case there Option[Seq[T]] could differ from Seq[T], producing a null or [].
However, here we are using asOptArray which converts an empty list to JsNull as seen here

So, even though we are changing from an Option to a Seq here, the end result will be the same during serialization.

Copy link
Copy Markdown
Contributor

@Jonathan-Schenk Jonathan-Schenk Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
val variableMeasured = asOptArray(dobj.specificInfo.fold((_.variables), _.columns))(
val variableMeasured = asOptArray(dobj.specificInfo.fold(_.variables, _.columns))(

variable => JsObject(
"@type" -> JsString("PropertyValue"),
"name" -> JsString(variable.label),
Expand Down
4 changes: 2 additions & 2 deletions src/main/scala/se/lu/nateko/cp/meta/services/upload/DobjMetaReader.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ trait DobjMetaReader(val vocab: CpVocab) extends CpmetaReader:
case Some(interval) =>
addInstrDeplInfo(stationUri, interval, columns)
columnsOptV.sinkOption.map: columnsOpt =>
StationTimeSeriesMeta(acq, prod, nRows, lblCoverage, columnsOpt)
StationTimeSeriesMeta(acq, prod, nRows, lblCoverage, columnsOpt.getOrElse(Nil))
Copy link
Copy Markdown
Contributor Author

@ggVGc ggVGc Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The option value columnsOpt here comes from parseJsonStringArray, where the None case signifies a parsing failure (a swallowed exception).
Hence, I don't see that there would be any semantic difference between Option and empty Seq here either.

resV.flatMap(identity)
end getStationTimeSerMeta

Expand Down Expand Up @@ -325,7 +325,7 @@ trait DobjMetaReader(val vocab: CpVocab) extends CpmetaReader:
station = station,
samplingHeight = samplingHeightOpt.flatten,
productionInfo = prod,
variables = Some(variables.flatten).filterNot(_.isEmpty)
variables = variables.flatten()
)

def getContributors(objIri: IRI, contribPredicate: IRI)(using conn: DobjConn | DocConn): Validated[IndexedSeq[Agent]] =
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/se/lu/nateko/cp/meta/services/upload/StatementsProducer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ class StatementsProducer(vocab: CpVocab, metaVocab: CpmetaVocab) {
makeSt(acq, metaVocab.prov.wasAssociatedWith, meta.forStation.map(_.toRdf)) ++
makeSt(acq, metaVocab.hasSamplingHeight, meta.samplingHeight.map(vocab.lit)) ++
makeSt(objUri, RDFS.SEEALSO, meta.customLandingPage.map(_.toRdf)) ++
meta.variables.toSeq.flatten.flatMap(getL3VarInfoStatements(objUri, hash, _))
meta.variables.flatMap(getL3VarInfoStatements(objUri, hash, _))
}

private def getStationDataStatements(hash: Sha256Sum, meta: StationTimeSeriesDto)(using Envri): Seq[Statement] = {
Expand Down
18 changes: 10 additions & 8 deletions src/main/scala/se/lu/nateko/cp/meta/services/upload/validation/UploadValidator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,14 +338,16 @@ class UploadValidator(servers: DataObjectInstanceServers):
if spec.specificDatasetType != DatasetType.SpatioTemporal
then errors += "Wrong type of dataset for this object spec (must be spatiotemporal)"
else
for vars <- spTempMeta.variables do spec.datasetSpec.fold(
errors += s"Data object specification ${spec.self.uri} lacks a dataset specification; cannot accept variable info."
): dsSpec =>
val valTypeLookupV = metaReader.getValTypeLookup(dsSpec.self.uri.toRdf)
errors ++= valTypeLookupV.errors
for valTypeLookup <- valTypeLookupV; varName <- vars do
if valTypeLookup.lookup(varName).isEmpty then errors +=
s"Variable name '$varName' is not compatible with dataset specification ${dsSpec.self.uri}"
if (spTempMeta.variables.nonEmpty) {
Copy link
Copy Markdown
Contributor Author

@ggVGc ggVGc Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure if this empty check is necessary, but including it assures we are keeping existing behaviour.

spec.datasetSpec.fold(
errors += s"Data object specification ${spec.self.uri} lacks a dataset specification; cannot accept variable info."
): dsSpec =>
val valTypeLookupV = metaReader.getValTypeLookup(dsSpec.self.uri.toRdf)
errors ++= valTypeLookupV.errors
for valTypeLookup <- valTypeLookupV; varName <- spTempMeta.variables do
if valTypeLookup.lookup(varName).isEmpty then errors +=
s"Variable name '$varName' is not compatible with dataset specification ${dsSpec.self.uri}"
}

case Right(stationMeta) =>
if spec.specificDatasetType != DatasetType.StationTimeSeries
Expand Down
6 changes: 3 additions & 3 deletions src/main/twirl/views/DataLandingPage.scala.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ <h2 class="fs-3 mt-5">Production</h2>

<br>

@for(variables <- varMetas){
@if(varMetas.nonEmpty){
Copy link
Copy Markdown
Contributor Author

@ggVGc ggVGc Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserving original behaviour, so we do not show any preview if there are no items. There is a slight difference here from previous behaviour, which I argue is actually a bug-fix.
Previously, we could have a Some(Seq.empty), which would mean we show this section but without any items, which I believe is not the intention. With this change, we only ever show the section if there are items.

<h2 class="fs-3 mt-5">Previewable variables</h2>
<div class="col-md-12 overflow-auto">
<table class="table">
Expand All @@ -155,7 +155,7 @@ <h2 class="fs-3 mt-5">Previewable variables</h2>
</tr>
</thead>
<tbody>
@for(varInfo <- variables){
@for(varInfo <- varMetas){
<tr>
<td>@(varInfo.label)</td>
<td>@(varInfo.valueType.self.label.getOrElse(""))</td>
Expand Down Expand Up @@ -402,7 +402,7 @@ <h2 class="fs-3 mt-5">Technical information</h2>
}

@instrumentDeploymentsPresent = @{
varMetas.exists(_.exists(_.instrumentDeployments.isDefined))
varMetas.exists(_.instrumentDeployments.isDefined)
}

@moratoriumText = @{
Expand Down
2 changes: 1 addition & 1 deletion src/test/scala/se/lu/nateko/cp/meta/upload/L3UpdateWorkbench.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ object L3UpdateWorkbench extends CpmetaJsonProtocol{
val l3 = dto.specificInfo.left.getOrElse(???)
val l3updated = l3.copy(
spatial = new URI("http://meta.icos-cp.eu/resources/latlonboxes/globalLatLonBox"),
variables = Some(Seq("emission")),
variables = Seq("emission"),
)
dto.copy(
submitterId = "CP",
Expand Down
10 changes: 5 additions & 5 deletions uploadgui/src/main/scala/se/lu/nateko/cp/meta/upload/Backend.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,18 +96,18 @@ object Backend {
sparqlSelect(datasetVariableQuery(dataset)).map(_.map(toDatasetVar))

def tryIngestion(
file: File, spec: ObjSpec, nRows: Option[Int], varnames: Option[Seq[String]]
file: File, spec: ObjSpec, nRows: Option[Int], varnames: Seq[String]
)(implicit envriConfig: EnvriConfig): Future[Unit] = {

val hasVars: Boolean = varnames.flatMap(_.headOption).isDefined
val hasVars = varnames.nonEmpty

if (spec.dataset.isDefined && (spec.isStationTimeSer || hasVars)) || spec.isZip || spec.isNetCDF then

val nRowsQ = nRows.fold("")(nr => s"&nRows=$nr")
val varsQ = varnames.fold(""){vns =>
val varsJson = encodeURIComponent(Json.toJson(vns).toString)
val varsQ = if (varnames.nonEmpty) {
Copy link
Copy Markdown
Contributor Author

@ggVGc ggVGc Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emptiness check to make sure we produce empty string instead of $varnames= when there are no vars.

val varsJson = encodeURIComponent(Json.toJson(varnames).toString)
s"&varnames=$varsJson"
}
} else { "" }

val url = s"https://${envriConfig.dataHost}/tryingest?specUri=${spec.uri}$nRowsQ$varsQ"
fetchOk("validate data object", url, new RequestInit{
Expand Down
16 changes: 7 additions & 9 deletions uploadgui/src/main/scala/se/lu/nateko/cp/meta/upload/formcomponents/L3VarInfoForm.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,17 @@ class L3VarInfoForm(elemId: String, notifyUpdate: () => Unit) {

var list: IndexedSeq[DatasetVar] = IndexedSeq.empty

def values: Try[Option[Seq[DatasetVar]]] = if(elems.isEmpty) Success(None) else Try{
Some(elems.map(_.varInfo.get).toIndexedSeq)
def values: Try[Seq[DatasetVar]] = if(elems.isEmpty) Success(Nil) else Try{
Copy link
Copy Markdown
Contributor Author

@ggVGc ggVGc Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we clearly see that there is no semantic change from our perspective between Option and Seq, since we are explicitly turning the empty case into a None in the previous version.

elems.map(_.varInfo.get).toIndexedSeq
}

def setValues(vars: Option[Seq[DatasetVar]]): Unit = {
def setValues(vars: Seq[DatasetVar]): Unit = {
elems.foreach(_.remove())
elems.clear()
vars.foreach{vdtos =>
vdtos.foreach{vdto =>
val input = new L3VarInfoInput
input.setValue(vdto)
elems.append(input)
}
vars.foreach{vdto =>
val input = new L3VarInfoInput
input.setValue(vdto)
elems.append(input)
}
}

Expand Down
30 changes: 17 additions & 13 deletions uploadgui/src/main/scala/se/lu/nateko/cp/meta/upload/subforms/SpatioTemporalPanel.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,10 @@ class SpatioTemporalPanel(covs: IndexedSeq[SpatialCoverage])(implicit bus: PubSu
samplingHeight = height,
production = prod,
customLandingPage = customLanding,
variables = varInfo.map(_.map(_.uri.toString.split('/').last))
variables = varInfo.map(_.uri.toString.split('/').last)
)

def varnames: Try[Option[Seq[String]]] = varInfoForm.values.map(_.map(_.map(_.title)))
def varnames: Try[Seq[String]] = varInfoForm.values.map(_.map(_.title))

private val titleInput = new TextInput("l3title", notifyUpdate, "elaborated product title")
private val descriptionInput = new DescriptionInput("l3descr", notifyUpdate)
Expand All @@ -55,14 +55,14 @@ class SpatioTemporalPanel(covs: IndexedSeq[SpatialCoverage])(implicit bus: PubSu
private val varInfoForm = new L3VarInfoForm("l3varinfo-form", notifyUpdate)
private val externalPageInput = new UriOptInput("l3landingpage", notifyUpdate)
private var datasetSpec: Option[URI] = None
private var selsectedVars: Option[Seq[String]] = None
private var selsectedVars: Seq[String] = Nil

def resetForm(): Unit = {
Iterable(
titleInput, descriptionInput, timeStartInput, timeStopInput,
temporalResInput, externalPageInput
).foreach(_.reset())
varInfoForm.setValues(None)
varInfoForm.setValues(Nil)
spatialCovSelect.resetForm()
}

Expand All @@ -76,9 +76,8 @@ class SpatioTemporalPanel(covs: IndexedSeq[SpatialCoverage])(implicit bus: PubSu
case GotStationsList(stations) => stationSelect.setOptions(stations)
case GotVariableList(variables) =>
varInfoForm.list = variables
selsectedVars.map { variables =>
varInfoForm.setValues(Some(variables.flatMap(uri => varInfoForm.list.find(_.uri.toString.split('/').last == uri))))
}
varInfoForm.setValues(selsectedVars.flatMap(uri =>
varInfoForm.list.find(_.uri.toString.split('/').last == uri)))

}

Expand All @@ -98,14 +97,19 @@ class SpatioTemporalPanel(covs: IndexedSeq[SpatialCoverage])(implicit bus: PubSu
samplingHeightInput.value = spatTemp.samplingHeight
externalPageInput.value = spatTemp.customLandingPage
selsectedVars = spatTemp.variables
spatTemp.variables.map { varUris =>
datasetSpec.map { dataset =>
whenDone(getVariables(dataset)) { variables =>
varInfoForm.list = variables
varInfoForm.setValues(Some(varUris.flatMap(uri => varInfoForm.list.find(_.uri.toString.split('/').last == uri))))
if (selsectedVars.nonEmpty) {
Copy link
Copy Markdown
Contributor

@Jonathan-Schenk Jonathan-Schenk Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block can be unindented. The selsectedVars could also be renamed to selectedVars to remove the typo.

datasetSpec match {
case None => ()
case Some(dataset) => {
whenDone(getVariables(dataset)) { variables =>
varInfoForm.list = variables
varInfoForm.setValues(selsectedVars.flatMap(uri =>
varInfoForm.list.find(_.uri.toString.split('/').last == uri)
))
}
}
Comment on lines +100 to +110
Copy link
Copy Markdown
Contributor Author

@ggVGc ggVGc Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another nonEmpty check I am not entirely sure is needed, but added to make sure we preserve prior behaviour.
I also changed the map call, since we are ignoring the result value and hence should use .foreach, but I personally think an explicit pattern match is much clearer.

}
}
}
spatialCovSelect.handleReceivedSpatialCoverage(Some(spatTemp.spatial))
show()
case _ =>
Expand Down