test(amber): add unit test coverage for LogicalLink#4956
Open
aglinxinyuan wants to merge 4 commits intoapache:mainfrom
Open
test(amber): add unit test coverage for LogicalLink#4956aglinxinyuan wants to merge 4 commits intoapache:mainfrom
aglinxinyuan wants to merge 4 commits intoapache:mainfrom
Conversation
Pin both construction paths and the production Jackson behavior:
- Primary case-class constructor: four fields exposed as constructed.
- Secondary @JsonCreator constructor: wraps raw String op ids in
OperatorIdentity (the Jackson deserialization path used for
user-saved workflow JSON).
- Case-class equals / hashCode across the four fields, including
field-discriminating tests (fromOpId, toPortId.internal) and
acceptance of self-loop links (same fromOpId / toOpId, distinct
ports — higher layers reject cycles, the data type does not).
- @JsonCreator path accepts dashes / dots / digits and the empty
string without normalization (no validation in the data type).
- Production-mapper Jackson coverage via JSONUtils.objectMapper:
- treeToValue with raw String op-id JSON dispatches to the
@JsonCreator string overload and produces the expected fields.
- @JsonProperty pins the on-the-wire key names (`fromOpId` /
`toOpId` / `fromPortId` / `toPortId`).
- writeValueAsString → readValue is NOT round-trippable: the
string overload can't accept the object-shape OperatorIdentity
that writeValueAsString emits. Asymmetry pinned so a future fix
(object @JsonCreator overload or custom @JsonDeserialize) flips
this on purpose.
- Missing string op-id fields silently default to
OperatorIdentity(null) — no validation in the @JsonCreator
path; future hardening should fail this on purpose.
Closes apache#4955
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new ScalaTest spec to cover org.apache.texera.workflow.LogicalLink behaviors that are important for workflow graph correctness and JSON (de)serialization compatibility, without changing production code.
Changes:
- Introduces
LogicalLinkSpeccovering primary/secondary construction paths and case-class equality/hashCode semantics. - Adds Jackson-focused tests using
JSONUtils.objectMapper, including explicit coverage for current (non-)round-trip behavior and missing-field defaults.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4956 +/- ##
============================================
- Coverage 42.22% 42.12% -0.10%
+ Complexity 2180 2179 -1
============================================
Files 980 980
Lines 36287 36257 -30
Branches 3783 3783
============================================
- Hits 15321 15273 -48
- Misses 20038 20054 +16
- Partials 928 930 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…eview Address Copilot feedback on apache#4956: - Replace the brittle `json.contains(...)` raw-string check on the writeValueAsString output with `objectMapper.readTree(json)` parse + structural assertions (`tree.path("fromOpId").isObject` / `path("id").asText() == "op-A"`). No longer depends on exact key ordering or escaping. - Drop the `node.set[ObjectNode](..., objectMapper.valueToTree(...))` pattern. The `[ObjectNode]` type parameter forces a runtime cast that assumes PortIdentity always serializes to an ObjectNode, which could throw ClassCastException if PortIdentity's serialization changes. Use `node.set("fromPortId", objectMapper.valueToTree[ JsonNode](...))` instead. - Replace `OperatorIdentity(null.asInstanceOf[String])` with the cleaner `link.fromOpId.id == null` assertion. Same intent (verify the op-id field is null after deserialization with missing JSON field), no `null.asInstanceOf[String]` noise.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot follow-up on apache#4956: - The @JsonProperty pin previously claimed "all four fields" but only fromOpId/toOpId actually carry the annotation. Split into two tests: one pinning the @JsonProperty-annotated op-id keys, and a separate one explicitly pinning that fromPortId/toPortId derive from Scala parameter names (no annotation). The second test makes the parameter-rename failure mode visible: a parameter rename without an accompanying @JsonProperty annotation would silently break saved workflows. The "writeValueAsString does NOT round-trip" thread is reconciled by updating the linked issue (apache#4955) body to clarify that the spec characterizes the current asymmetry rather than asserting a passing round-trip; the follow-up fix that adds a custom JsonDeserialize for fromOpId/toOpId would flip the characterization-pin into a real round-trip.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
Adds
LogicalLinkSpeccovering the case class atamber/src/main/scala/org/apache/texera/workflow/LogicalLink.scala. It has two construction paths (the primary case-class constructor + a secondary@JsonCreatoroverload that takes rawStringop-ids) and ships across the wire / persisted in saved workflows, but had no unit test.@JsonCreatorconstructorStringop-ids inOperatorIdentity; equal to a primary-constructor link with the same content.equals/hashCodefromOpIdmismatch,toPortId.internalmismatch).fromOpId == toOpIdis structurally allowed (cycles are rejected at higher layers, not here).JSONUtils.objectMapper)treeToValuefrom a JSON object with raw-string op-ids dispatches to the@JsonCreatorstring overload and produces the expected fields.@JsonPropertyJSON field namesfromOpId/toOpId/fromPortId/toPortIdpinned on the wire so a renamed Scala field can't silently break saved workflows.writeValueAsString↔readValueasymmetrywriteValueAsStringemits OperatorIdentity as{"id":"op-A"}(object form), but the@JsonCreatorString overload that Jackson dispatches to on read can't accept an object forfromOpId. The asymmetry is pinned so a future fix (object@JsonCreatoroverload or custom@JsonDeserialize) flips this on purpose.OperatorIdentity(null)— no validation in the@JsonCreatorpath. Pinned so a future non-null check fails this on purpose.No production code changed; this is test-only.
Any related issues, documentation, discussions?
Closes #4955
How was this PR tested?
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.7)