aglinxinyuan commented on code in PR #5697: URL: https://github.com/apache/texera/pull/5697#discussion_r3408758293
########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/tablesChart/TablesConfigSpec.scala: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.visualization.tablesChart + +import com.fasterxml.jackson.annotation.JsonProperty +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec + +import javax.validation.constraints.NotNull + +class TablesConfigSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Default state + // --------------------------------------------------------------------------- + + "TablesConfig" should "default attributeName to the empty string" in { + val c = new TablesConfig + assert(c.attributeName == "") + } + + // --------------------------------------------------------------------------- + // Mutability — the @JsonProperty bag is `var`-based by design + // --------------------------------------------------------------------------- + + it should "allow attributeName to be assigned post-construction" in { + val c = new TablesConfig + c.attributeName = "col-a" + assert(c.attributeName == "col-a") + } + + // --------------------------------------------------------------------------- + // JSON round-trip preserves the field + // --------------------------------------------------------------------------- + + "TablesConfig JSON round-trip" should + "serialize and deserialize attributeName unchanged" in { + val original = new TablesConfig + original.attributeName = "my-attr" + val json = objectMapper.writeValueAsString(original) + val restored = objectMapper.readValue(json, classOf[TablesConfig]) + assert(restored.attributeName == "my-attr") + } + + it should "round-trip the default empty attributeName" in { + val original = new TablesConfig + val restored = objectMapper.readValue( + objectMapper.writeValueAsString(original), + classOf[TablesConfig] + ) + assert(restored.attributeName == "") + } + + // --------------------------------------------------------------------------- + // Independent instances — no static-field leakage + // --------------------------------------------------------------------------- + + it should "construct two independent instances (no static state shared)" in { + val a = new TablesConfig + val b = new TablesConfig + a.attributeName = "first" + assert(b.attributeName == "", "second instance must not see first instance's mutation") + } + + // --------------------------------------------------------------------------- + // Annotations — verified via reflection (the Jackson + validation + // contract is what consumers actually depend on) + // --------------------------------------------------------------------------- + + "TablesConfig#attributeName" should "carry @JsonProperty(required = true)" in { + val field = classOf[TablesConfig].getDeclaredField("attributeName") + val jp = field.getAnnotation(classOf[JsonProperty]) + assert(jp != null, "attributeName must carry @JsonProperty") + assert(jp.required, "attributeName must be marked required") + } + + it should "carry @NotNull (jakarta validation contract)" in { + val field = classOf[TablesConfig].getDeclaredField("attributeName") + val notNull = field.getAnnotation(classOf[NotNull]) + assert(notNull != null, "attributeName must carry @NotNull for jakarta validation") + } +} Review Comment: Fixed in bc33a15ea9 — reworded the case-name + assertion message to "javax.validation" so it matches the codebase, and added a separate case pinning `@AutofillAttributeName` on `TablesConfig.attributeName` via reflection (parallel to `FigureFactoryTableConfigSpec`). ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/tablesChart/TablesConfigSpec.scala: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.visualization.tablesChart + +import com.fasterxml.jackson.annotation.JsonProperty +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec + +import javax.validation.constraints.NotNull + Review Comment: Fixed in bc33a15ea9 — added the `import org.apache.texera.amber.operator.metadata.annotations.AutofillAttributeName` plus a case asserting the annotation is present on `attributeName`. ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/nestedTable/NestedTableConfigSpec.scala: ########## @@ -0,0 +1,132 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.visualization.nestedTable + +import com.fasterxml.jackson.annotation.JsonProperty +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec + +class NestedTableConfigSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Default state + // --------------------------------------------------------------------------- + + "NestedTableConfig" should "default every field to the empty string" in { + val c = new NestedTableConfig + assert(c.attributeGroup == "") + assert(c.originalName == "") + assert(c.newName == "") + } + + // --------------------------------------------------------------------------- + // Mutability + // --------------------------------------------------------------------------- + + it should "allow every field to be assigned post-construction" in { + val c = new NestedTableConfig + c.attributeGroup = "g" + c.originalName = "orig" + c.newName = "renamed" + assert(c.attributeGroup == "g") + assert(c.originalName == "orig") + assert(c.newName == "renamed") + } + + // --------------------------------------------------------------------------- + // JSON round-trip + // --------------------------------------------------------------------------- + + "NestedTableConfig JSON round-trip" should "preserve all three fields" in { + val original = new NestedTableConfig + original.attributeGroup = "group-1" + original.originalName = "src-name" + original.newName = "dst-name" + val json = objectMapper.writeValueAsString(original) + val restored = objectMapper.readValue(json, classOf[NestedTableConfig]) + assert(restored.attributeGroup == "group-1") + assert(restored.originalName == "src-name") + assert(restored.newName == "dst-name") + } + + it should + "serialize newName under the JSON key `name` (per @JsonProperty(value = \"name\"))" in { + // The wire-key for `newName` is `name`, not `newName` — a regression + // that drifted to `newName` would silently break every workflow JSON + // that carries this config. Pin the wire-key explicitly. + val c = new NestedTableConfig + c.newName = "renamed" + val json = objectMapper.writeValueAsString(c) + assert( + json.contains("\"name\":\"renamed\""), + s"expected 'name':'renamed' wire-key in JSON, got: $json" + ) + assert( + !json.contains("\"newName\""), + s"the field name 'newName' must NOT appear as a JSON key, got: $json" + ) + } Review Comment: Fixed in bc33a15ea9 — replaced the `json.contains("...")` substring checks with `objectMapper.readTree(...)` + `JsonNode.has("name") / get("name").asText()`. Robust to whitespace and key-ordering changes in the serializer. ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/dumbbellPlot/DumbbellDotConfigSpec.scala: ########## @@ -0,0 +1,128 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.visualization.dumbbellPlot + +import com.fasterxml.jackson.annotation.JsonProperty +import com.kjetland.jackson.jsonSchema.annotations.JsonSchemaInject +import org.apache.texera.amber.operator.metadata.annotations.AutofillAttributeName +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec + +import javax.validation.constraints.NotNull + +class DumbbellDotConfigSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Default state + // --------------------------------------------------------------------------- + + "DumbbellDotConfig" should "default dotValue to the empty string" in { + val c = new DumbbellDotConfig + assert(c.dotValue == "") + } + + // --------------------------------------------------------------------------- + // Mutability + // --------------------------------------------------------------------------- + + it should "allow dotValue to be assigned post-construction" in { + val c = new DumbbellDotConfig + c.dotValue = "numeric-col" + assert(c.dotValue == "numeric-col") + } + + // --------------------------------------------------------------------------- + // JSON round-trip — verify both the Scala field name AND the wire key + // --------------------------------------------------------------------------- + + "DumbbellDotConfig JSON round-trip" should + "preserve dotValue via the wire-key `dot` (per @JsonProperty(value = \"dot\"))" in { + val original = new DumbbellDotConfig + original.dotValue = "amount" + val json = objectMapper.writeValueAsString(original) + assert( + json.contains("\"dot\":\"amount\""), + s"expected 'dot':'amount' wire-key in JSON, got: $json" + ) + val restored = objectMapper.readValue(json, classOf[DumbbellDotConfig]) + assert(restored.dotValue == "amount") + } Review Comment: Fixed in bc33a15ea9 — same approach as `NestedTableConfigSpec`: parse the JSON with `objectMapper.readTree` and assert on `tree.get("dot").asText()`, plus `!tree.has("dotValue")` to confirm the Scala field name is not leaking onto the wire. ########## common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/dumbbellPlot/DumbbellDotConfigSpec.scala: ########## @@ -0,0 +1,128 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.visualization.dumbbellPlot + +import com.fasterxml.jackson.annotation.JsonProperty +import com.kjetland.jackson.jsonSchema.annotations.JsonSchemaInject +import org.apache.texera.amber.operator.metadata.annotations.AutofillAttributeName +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec + +import javax.validation.constraints.NotNull + +class DumbbellDotConfigSpec extends AnyFlatSpec { + + // --------------------------------------------------------------------------- + // Default state + // --------------------------------------------------------------------------- + + "DumbbellDotConfig" should "default dotValue to the empty string" in { + val c = new DumbbellDotConfig + assert(c.dotValue == "") + } + + // --------------------------------------------------------------------------- + // Mutability + // --------------------------------------------------------------------------- + + it should "allow dotValue to be assigned post-construction" in { + val c = new DumbbellDotConfig + c.dotValue = "numeric-col" + assert(c.dotValue == "numeric-col") + } + + // --------------------------------------------------------------------------- + // JSON round-trip — verify both the Scala field name AND the wire key + // --------------------------------------------------------------------------- + + "DumbbellDotConfig JSON round-trip" should + "preserve dotValue via the wire-key `dot` (per @JsonProperty(value = \"dot\"))" in { + val original = new DumbbellDotConfig + original.dotValue = "amount" + val json = objectMapper.writeValueAsString(original) + assert( + json.contains("\"dot\":\"amount\""), + s"expected 'dot':'amount' wire-key in JSON, got: $json" + ) + val restored = objectMapper.readValue(json, classOf[DumbbellDotConfig]) + assert(restored.dotValue == "amount") + } + + it should "deserialize from the wire-key `dot` back into dotValue" in { + val json = """{"dot":"amount"}""" + val restored = objectMapper.readValue(json, classOf[DumbbellDotConfig]) + assert(restored.dotValue == "amount") + } + + // --------------------------------------------------------------------------- + // Annotations + // --------------------------------------------------------------------------- + + "DumbbellDotConfig#dotValue" should + "carry @JsonProperty(value = 'dot', required = true)" in { + val jp = classOf[DumbbellDotConfig] + .getDeclaredField("dotValue") + .getAnnotation(classOf[JsonProperty]) + assert(jp != null) + val actualValue = jp.value + assert(actualValue == "dot", s"expected value='dot', got: '$actualValue'") + assert(jp.required, "dotValue must be marked required") + } + + it should "carry @NotNull (jakarta validation contract)" in { + val notNull = classOf[DumbbellDotConfig] + .getDeclaredField("dotValue") + .getAnnotation(classOf[NotNull]) + assert(notNull != null, "dotValue must carry @NotNull for jakarta validation") + } Review Comment: Fixed in bc33a15ea9 — case-name + assertion message now say "javax.validation". -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
