Skip to content

Add Kind assertions to findInArray and findKeyInMap#2651

Open
jandubois wants to merge 1 commit intomikefarah:masterfrom
jandubois:harden-node-kind-assertions
Open

Add Kind assertions to findInArray and findKeyInMap#2651
jandubois wants to merge 1 commit intomikefarah:masterfrom
jandubois:harden-node-kind-assertions

Conversation

@jandubois
Copy link
Copy Markdown
Contributor

@jandubois jandubois commented Apr 6, 2026

findInArray and findKeyInMap accept any *CandidateNode but produce wrong results when called on the wrong Kind: findInArray uses stride 1, correct for SequenceNodes but dangerous on MappingNodes (where key-value pairs live at even-odd indices); findKeyInMap uses stride 2, correct for MappingNodes but silently skips elements in SequenceNodes.

Commit b0ba958 fixed two call sites that passed MappingNodes to findInArray, but nothing prevents the same mistake from recurring. These assertions make both functions panic immediately on a Kind mismatch, catching misuse in tests rather than producing silent data corruption.

I was tempted to rename findInArray to findInSequence to match SequenceNode, but didn't want to engage in gratuitous refactoring. But still may be worth harmonizing the terminology.

findInArray and findKeyInMap accept any *CandidateNode but produce
wrong results when called on the wrong Kind: findInArray uses stride 1,
correct for SequenceNodes but dangerous on MappingNodes (where
key-value pairs live at even-odd indices); findKeyInMap uses stride 2,
correct for MappingNodes but silently skips elements in SequenceNodes.

Commit b0ba958 fixed two call sites that passed MappingNodes to
findInArray, but nothing prevents the same mistake from recurring.
These assertions make both functions panic immediately on a Kind
mismatch, catching misuse in tests rather than producing silent
data corruption.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mikefarah
Copy link
Copy Markdown
Owner

Panic worries me a little - in case this is still used incorrectly somewhere and it breaks someones CI/CD...I'd prefer a warning log....

@jandubois
Copy link
Copy Markdown
Contributor Author

Assuming you have sufficient test coverage this should panic in your test and prevent you from creating a broken release if you add new code that accidentally calls the wrong method. A warning may just get lost in the CI output.

If you think this can actually trigger in normal usage, but not during your test suite, wouldn't it still be better to throw an error than potentially return incorrect results. Since you mention CI/CD, any warning will also be ignored as long as tests pass.

I would also expect that such a failure would be happening when they attempt to bump their yq dependency, and then they would not merge the bump, but report the error to you.

Ultimately your decision, but I find using a panic more prudent in this situation.

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.

2 participants