Skip to content

Fix nested array null filtering with stripNulls - #525

Open
Tejas Lamba (TejasLamba2006) wants to merge 1 commit into
microsoft:mainfrom
TejasLamba2006:fix-nested-null-filtering
Open

Tejas Lamba (TejasLamba2006) wants to merge 1 commit into
microsoft:mainfrom
TejasLamba2006:fix-nested-null-filtering

Conversation

@TejasLamba2006

@TejasLamba2006 Tejas Lamba (TejasLamba2006) commented Nov 8, 2026 •

Copy link
Copy Markdown

When stripNulls filters an outer array and assigns the result to obj[k], recursion still visited the old unfiltered array held in value. Nested replacements were disconnected from the returned structure.

The recursive call now uses obj[k] directly instead of the stale value reference, so it always operates on the post-filter array.

Added regression tests for nested arrays (groups: number[][]) and flat arrays to prevent regressions.

Related to #496.

When stripNulls filters an outer array and assigns the result to obj[k],
recursion still visited the old unfiltered array held in value. Switch the
recursive call to use obj[k] directly so it always operates on the
post-filter value. Added regression tests for nested and flat arrays.
Copilot AI balanced review requested due to automatic review settings November 8, 2026 10:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

A maintainer needs to reconcile this change with the duplicate fix in open draft PR #496 and preserve contributor credit before merging.

No open issues 🎉

Review effort: Max

@robgruen

Copy link
Copy Markdown
Contributor

Thanks Tejas Lamba (@TejasLamba2006) for this focused fix. The nested-array regression catches the stale-reference bug, and the implementation looks sound in local testing.

One improvement I'd recommend is using createTypeScriptJsonValidator in the regression tests instead of a custom validator that only checks Array.isArray. Keep attemptRepair = false and close the validator in finally. That would verify not just the resulting array contents, but also that the cleaned response actually satisfies groups: number[][].

It would be useful to cover both:

  • {"groups":[null,[null,1]]} -> {"groups":[[1]]}
  • {"groups":[[null,1],null,[null,null]]} -> {"groups":[[1],[]]}

JOJO (@moneymaker26754-crypto), thank you for the earlier work in #496, which already includes these real-validator regression cases and a fix for the same root cause. Please coordinate the overlapping contributions; if the work is consolidated into one PR, let's preserve credit for both authors, including the implementation and regression coverage.

Since #496 is itself an open draft PR rather than an issue, "Related to #496" would describe the relationship more accurately than "Fixes #496".

@robgruen robgruen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please see my improvement comment.

@robgruen

Copy link
Copy Markdown
Contributor

Can you two (Tejas Lamba (@TejasLamba2006) , JOJO (@moneymaker26754-crypto)) please consolidate into #497 .

@TejasLamba2006

Copy link
Copy Markdown
Author

Sounds good, happy to consolidate into #497. JOJO (@moneymaker26754-crypto) if you want to pull the nested-array stripNulls fix into your draft, feel free to cherry-pick from this branch or I can push it there directly. Either way works for me, just want to make sure both fixes land.

Will update the "Fixes #496" reference to "Related to #496" as suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants