feat(evaluator): add lower/upper string casing operators#1982
Open
anxkhn wants to merge 4 commits into
Open
Conversation
Add two single-argument JSONLogic operators, `lower` and `upper`, that fold a
string to ASCII lower/upper case. Composed with existing operators (==, in,
starts_with, ...) they enable case-insensitive targeting without duplicating
every comparison operator, e.g.:
{ "==": [{ "lower": [{ "var": "email" }] }, "user@example.com"] }
Casing is restricted to ASCII (A-Z/a-z) so results are deterministic and
identical across all flagd providers; other bytes are unchanged. Includes
targeting schema, operator-table and custom-operation docs, and unit tests.
Closes open-feature#1916
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
dosubot
Bot
added
the
size:L
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
👮 Files not reviewed due to content moderation or server errors (1)
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/pkg/evaluator/string_casing_test.go`:
- Around line 243-256: The no-error test cases in string_casing_test.go are
short-circuiting the loop because the custom wantErr closures for the
bare-string and single-element-array cases return false, so the subsequent
assert.Equalf on wantProperty is never reached. Update those wantErr helpers in
the relevant test table entries to indicate success (or replace them with
assert.NoError-style checks) so the test continues past the error check and
actually verifies the returned string values for the string-casing evaluator
cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b9e9b08-7a3d-4bf6-b859-85607cd99883
📒 Files selected for processing (7)
core/pkg/evaluator/json.gocore/pkg/evaluator/string_casing.gocore/pkg/evaluator/string_casing_test.godocs/reference/custom-operations/string-casing-operation.mddocs/reference/flag-definitions.mddocs/schema/v0/targeting.jsonmkdocs.yml
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Author
|
Good catch. The two happy-path |
The lower and upper operators shared near-identical implementations: each method repeated the parse/error-log/return-nil body, and lowerString and upperString repeated the same ASCII-folding loop. SonarCloud flagged this as duplicated code on the PR's new lines. Factor the shared logic into helpers so the two operators differ only by their case mapping: - evaluateCasing(op, values, mapByte) holds the common parse-and-apply body. - asciiCase(s, mapByte) holds the common per-byte rewrite loop. - toASCIILower/toASCIIUpper isolate the only real difference (the letter range and fold direction). Behavior and operator names are unchanged; existing tests pass. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Each lower/upper table case repeated the full model.Flag literal (key, state, default variant, variants, and a multi-line targeting rule), and two of the upper cases were byte-identical. SonarCloud flagged this as duplicated new code (new_duplicated_lines_density above the 3% gate) on string_casing_test.go. Extract a casingFlag(targeting) helper so each case supplies only its targeting rule and collapse the targeting JSON to a single line. Test names, contexts, and expectations are unchanged; go test -race and golangci-lint still pass. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
|
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
1 participant
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.
This adds two single-argument JSONLogic operators,
lowerandupper, that fold a string to ASCII lower/upper case. Composed with the existing operators (==,in,starts_with, ...) they enable case-insensitive targeting without duplicating every comparison operator with an ignore-case variant, which is the shape requested in the issue:{ "==": [{ "lower": [{ "var": "email" }] }, "user@example.com"] }Casing is restricted to ASCII (
A-Z/a-z) per @toddbaert's recommendation on the issue:strings.ToLower/ToUppergive simple 1:1 mapping in Go, but Java/Python/Rust/JS use full (and Java locale-sensitive) mappings, so a fold likeß -> SSwould diverge across providers. ASCII-only is trivially identical everywhere and covers the real use case (emails, non-IDN domains). All non-ASCII bytes are left unchanged. This is documented as the intended scope.Changes
core/pkg/evaluator/string_casing.go:lower/upperoperators, mirroring thestring_comparison.go(starts_with/ends_with) structure. Single arg accepted as bare string or 1-element array; non-string/invalid input logs and returnsnil, matching the existing operators' error idiom.core/pkg/evaluator/json.go: register both operators inNewResolver.docs/schema/v0/targeting.json:stringCasingRuleadded toanyRule.core/pkg/evaluator/string_casing_test.go: table-driven tests (compose with==/starts_with, ASCII-only proof, non-string fallback).Closes #1916
Notes
strings.ToLower/ToUpper.make test-core(-race),golangci-lintv2.7.2, andmarkdownlintall pass locally; evaluator coverage ~87.6%.