Suggestion: Use Seq[T] instead of Option[Seq[T]] in UploadDtos by ggVGc · Pull Request #348 · ICOS-Carbon-Portal/meta

ggVGc · 2026-02-25T11:25:47Z

Wrapping a Seq in Option has the same semantic meaning in almost every case I can think of, and in the cases where it does not, I think an alternative and more descriptive solution should probably be used.

The changes in this PR should be functionally equivalent to the previous version, while simplifying the logic using these data types, but I have not tested anything in practice yet, so there may of course be edge cases I have not thought of.
I have left comments at relevant places for why I believe these changes are functionally equivalent and valid, though.

Note: Based on #341 since it touches same files.

There are a few more DTOs with Option[Seq[]] that should probably also be changed as part of this PR.

Testing TODO

Tests should be added in a new branch, based on master, verifying existing behavior. Then the tests can be merged into this one.
Where could this change have visible impacts?:
- When registering a new metadata package, do we still accept the same payloads? Parsing of a payload happens here.
- The structures in [DataObject.scala)[https://github.com/ICOS-Carbon-Portal/meta/blob/master/core/src/main/scala/se/lu/nateko/cp/meta/core/data/DataObject.scala#L1] are serialized to JSON and embedded in the source of metadata previews. That is implemented in SchemaOrg.dataJson(object: DataObject), which can be tested directly.
- The changes affect files in meta/upload/subforms. Most likely, the best way to test it is to manually check that the interface still works as expected.
Are there existing tests, and what do they cover?
- There does not seem to be any test covering the upload route. Adding it next to AlternativePathRouteTest would make sense.

ggVGc

Reasonings behind the main changes.

ggVGc · 2026-02-25T11:31:13Z

 				extra <- memb.role.extra;
-				l2 <- dobj.specificInfo.toOption;
-				cols <- l2.columns
+				l2 <- dobj.specificInfo.toOption


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.

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.

ggVGc · 2026-02-25T11:32:21Z

-							stationTs.columns.fold("")(_.collect{
-								case v if v.valueType.unit.isDefined => v.label
-							}.mkString(" (", ", ", ")"))
+							if (stationTs.columns.nonEmpty)


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.

ggVGc · 2026-02-25T11:37:41Z

 		}

-		val variableMeasured = asOptArray(dobj.specificInfo.fold(_.variables, _.columns))(
+		val variableMeasured = asOptArray(dobj.specificInfo.fold((_.variables), _.columns))(


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.

ggVGc · 2026-02-25T11:39:15Z

-						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) {


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

ggVGc · 2026-02-25T11:44:02Z

 							addInstrDeplInfo(stationUri, interval, columns)
 			columnsOptV.sinkOption.map: columnsOpt =>
-				StationTimeSeriesMeta(acq, prod, nRows, lblCoverage, columnsOpt)
+				StationTimeSeriesMeta(acq, prod, nRows, lblCoverage, columnsOpt.getOrElse(Nil))


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.

ggVGc · 2026-02-25T11:46:18Z

 			<br>

-			@for(variables <- varMetas){
+			@if(varMetas.nonEmpty){


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.

ggVGc · 2026-02-25T11:49:13Z


-	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{


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.

ggVGc · 2026-02-25T11:52:53Z

+					if (selsectedVars.nonEmpty) {
+						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)
+									))
+								}
+							}


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.

ggVGc · 2026-02-25T11:53:58Z

 			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) {


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

jonathanthiry

In general, I agree with the idea. I don't know for sure why it was decided to use Option[Seq[T]] to start with. As you are hinting in your comments, these changes would require proper testing of the impacted code paths to avoid introducing any bugs, as it is not easy to judge this at a glance.

jonathanthiry · 2026-02-25T17:07:29Z

 				extra <- memb.role.extra;
-				l2 <- dobj.specificInfo.toOption;
-				cols <- l2.columns
+				l2 <- dobj.specificInfo.toOption


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.

Jonathan-Schenk

The changes you suggest make sense and improve the clarity of the code. I think it is a good idea to apply them after testing that nothing breaks because of them, although it seems that all current behaviors would be maintained.

Jonathan-Schenk · 2026-03-05T16:12:12Z

 		}

-		val variableMeasured = asOptArray(dobj.specificInfo.fold(_.variables, _.columns))(
+		val variableMeasured = asOptArray(dobj.specificInfo.fold((_.variables), _.columns))(


Suggested change

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

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

Jonathan-Schenk · 2026-03-05T16:14:48Z

-						whenDone(getVariables(dataset)) { variables =>
-							varInfoForm.list = variables
-							varInfoForm.setValues(Some(varUris.flatMap(uri => varInfoForm.list.find(_.uri.toString.split('/').last == uri))))
+					if (selsectedVars.nonEmpty) {


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

ggVGc added 6 commits February 25, 2026 10:55

Simplify varnames from Option[Seq] to just Seq

b95dda0

Preserve original behaviour of empty string when empty list

a55f960

Change SpatioTemporalDto.variables from Option[Seq] to Seq

05e8152

Change SpatioTemporalMeta.variables from Option[Seq] to Seq

2e52fb4

Change StationTimeSeriesMeta.columns from Option[Seq] to Seq

6e4271f

Improve diff

8de9db9

ggVGc marked this pull request as draft February 25, 2026 11:25

ggVGc commented Feb 25, 2026

View reviewed changes

ggVGc requested review from Jonathan-Schenk and jonathanthiry February 25, 2026 11:56

jonathanthiry reviewed Feb 26, 2026

View reviewed changes

Jonathan-Schenk reviewed Mar 5, 2026

View reviewed changes

ggVGc marked this pull request as ready for review March 18, 2026 12:07

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

Conversation

ggVGc commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggVGc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanthiry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jonathan-Schenk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ggVGc commented Feb 25, 2026 •

edited

Loading