Skip to content

fix: emit array-of-primitive entries in JSONToMap flatten mode#5045

Merged
aglinxinyuan merged 5 commits into
apache:mainfrom
Ma77Ball:fix/siletnDrop
May 15, 2026
Merged

fix: emit array-of-primitive entries in JSONToMap flatten mode#5045
aglinxinyuan merged 5 commits into
apache:mainfrom
Ma77Ball:fix/siletnDrop

Conversation

@Ma77Ball
Copy link
Copy Markdown
Contributor

@Ma77Ball Ma77Ball commented May 13, 2026

What changes were proposed in this PR?

JSONUtils.JSONToMap with flatten=true silently dropped primitive elements that lived inside an array. The docstring's worked example claimed {"E":["X","Y"]} would flatten to {"E1":"X","E2":"Y"}, but the implementation only emitted entries from inside its isObject / isArray branches, so a recursive call landing on a value node returned an empty mapand the data vanished. This PR adds a third top-level branch that emits parentName -> node.asText() whenever JSONToMap is called on a value node with a non-empty parentName, bringing the behavior into line with the docstring. The two JSONUtilsSpec cases that previously pinned the buggy behavior (with comments inviting exactly this tightening) are flipped to assert the documented contract, and a new case covers a mixed array of objects and primitives.

Any related issues, documentation, or discussions?

Closes: #4729

How was this PR tested?

  • Updated JSONUtilsSpec: flipped the two pinning cases (array-of-primitives, top-level array of primitives) to assert the documented output, added a mixed-array case.
  • sbt "WorkflowCore/testOnly org.apache.texera.amber.util.JSONUtilsSpec" passes (15/15).
  • sbt scalafmtAll is clean.

Was this PR authored or co-authored using generative AI tooling?

Co-authored with Claude Opus 4.7 in compliance with ASF

@github-actions github-actions Bot requested a review from bobbai00 May 13, 2026 05:28
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 13, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 42.87%. Comparing base (1ea368d) to head (2ee41d3).

Files with missing lines Patch % Lines
...scala/org/apache/texera/amber/util/JSONUtils.scala 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5045      +/-   ##
============================================
- Coverage     43.06%   42.87%   -0.20%     
+ Complexity     2211     2207       -4     
============================================
  Files          1045     1045              
  Lines         40216    40068     -148     
  Branches       4243     4237       -6     
============================================
- Hits          17321    17181     -140     
+ Misses        21829    21816      -13     
- Partials       1066     1071       +5     
Flag Coverage Δ *Carryforward flag
access-control-service 39.53% <ø> (ø)
agent-service 33.72% <ø> (ø) Carriedforward from ed2f775
amber 43.77% <50.00%> (-0.04%) ⬇️
computing-unit-managing-service 0.00% <ø> (ø)
config-service 0.00% <ø> (ø)
file-service 32.18% <ø> (ø)
frontend 34.05% <ø> (+0.12%) ⬆️ Carriedforward from ed2f775
python 89.00% <ø> (-1.37%) ⬇️ Carriedforward from ed2f775
workflow-compiling-service 47.72% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot removed the request for review from bobbai00 May 15, 2026 07:59
@Ma77Ball
Copy link
Copy Markdown
Contributor Author

/request-review @aglinxinyuan

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes JSONUtils.JSONToMap so that, in flatten=true mode, primitive elements inside arrays are no longer silently dropped. The implementation is brought into alignment with the method’s docstring example (e.g., {"E":["X","Y"]}Map("E1" -> "X", "E2" -> "Y")), and the unit tests are updated to assert the intended contract.

Changes:

  • Emit parentName -> node.asText() when the recursive flattening reaches a value node with a non-empty parentName.
  • Update existing specs that previously pinned the buggy behavior to now assert the documented output.
  • Add a new spec covering a mixed array containing both objects and primitives.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
common/workflow-core/src/main/scala/org/apache/texera/amber/util/JSONUtils.scala Adds a value-node emission branch so primitives inside arrays are included during flatten recursion.
common/workflow-core/src/test/scala/org/apache/texera/amber/util/JSONUtilsSpec.scala Updates/extends tests to validate flattening arrays of primitives (including top-level arrays) and mixed arrays.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@aglinxinyuan aglinxinyuan merged commit 6e68b19 into apache:main May 15, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JSONUtils.JSONToMap silently drops array-of-primitive elements in flatten mode

4 participants