Fix string-to-integer coercion in query matching#79
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
==========================================
- Coverage 54.05% 54.02% -0.04%
==========================================
Files 21 21
Lines 4266 4263 -3
==========================================
- Hits 2306 2303 -3
Misses 1960 1960
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
davidlehn
reviewed
May 29, 2026
davidlehn
left a comment
Member
There was a problem hiding this comment.
- Does this need more edge case checks?
- Might consider at least having a zero and negative int check. Probably Number.MIN/MAX_SAFE_INTEGER too.
- Does this need the reverse checks of data with a plain int and query with an int string? I think the code would handle that?
- Parsed JSON itself with native ints will (probably?) be limited to Number.MIN/MAX_SAFE_INTEGER. But if both sides had strings with non safe int values, I think this starts to lose precision and would cause matching failures. In any case, havoc will probably happen. Is that important to handle here?
Example:
> Number.MAX_SAFE_INTEGER
9007199254740991
> a = parseInt('9007199254740992')
9007199254740992
> b = parseInt('9007199254740993')
9007199254740992
> a.toString()
'9007199254740992'
> b.toString()
'9007199254740992'
> a.toString() === b.toString()
true- One solution might be to use
BigInt(x), but that's introducing radix prefix issues and returning BigInts vs ints for comparison and no doubt has odd issues. - Another solution is to specifically not handle these issues and after the parseInt, check if Number.isSafeInteger(i) is false, then return x. Would need some tests to make sure that works and simply refuses to match those non-safe string values for now.
Member
|
@davidlehn using |
Co-authored-by: David I. Lehn <dil@lehn.org>
dlongley
approved these changes
Jun 5, 2026
dlongley
left a comment
Member
There was a problem hiding this comment.
I'm going to merge this now. We can add the extra checks, whatever we decide those are, in a subsequent PR.
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.
Tests
Added to
tests/unit/query.match.spec.js(with a supporting credential intests/unit/mockCredentials.js):"3.14"must not match an integer query for3"21abc"must not match an integer query for21"21"still matches21(valid coercion preserved)no-decimal-coercion boundary)
Addresses #74