feat: allow '-' and '/' in object IDs#624
Conversation
Extend the object ID validation rule to permit '-' and '/' ('|'
already allowed) across the Go, JS, and Java validators, matching
characters the server accepts. Type/relation/condition rules
unchanged. Closes #437.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe ID validation regex pattern is updated in Go, Java, and JavaScript implementations to allow ChangesID Regex Update Across Languages
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested Labels
Suggested ReviewersNone identified from the provided context. Poem A dash, a slash, now join the fray, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the OpenFGA DSL object ID validation regex across the Go, JS, and Java implementations to allow - and / (in addition to existing allowed characters like |), aligning the language validators with characters accepted by the server.
Changes:
- Expanded the object ID regex character class to include
-and/in Go, JS, and Java validators. - Added/updated test cases in Go, JS, and Java to assert that IDs containing
-and/are accepted.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/js/validator/validate-rules.ts | Updates the JS object ID validation regex to include - and /. |
| pkg/js/tests/validate-rules.test.ts | Adds JS unit tests covering IDs containing - and /. |
| pkg/java/src/main/java/dev/openfga/language/validation/Validator.java | Updates the Java object ID validation regex to include - and /. |
| pkg/java/src/test/java/dev/openfga/language/validation/ValidationRules.java | Adds Java unit tests covering IDs containing - and /. |
| pkg/go/validation/validation-rules.go | Updates the Go object ID validation regex to include - and /. |
| pkg/go/validation/validation-rules_test.go | Adds Go unit tests covering IDs containing - and /. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rhamzeh
left a comment
There was a problem hiding this comment.
A minor note in case you can quickly spike it - would you mind trying to see if you can move the test cases to the common yaml so that we don't have to write them 3 times and ensure they're all consistent with each other?
Not a blocker - just a quick spike
… regex Escape '-' as '\-' in the object-ID regex (Go, JS, Java) so it is unambiguously a literal rather than a range, and move the object-ID validation test cases into a shared tests/data/object-id-validation-cases.yaml consumed by all three implementations. Adds a regression case ensuring ',' remains invalid. Refs #437
Extend the object ID validation rule to permit '-' and '/' ('|' already allowed) across the Go, JS, and Java validators, matching characters the server accepts. Type/relation/condition rules unchanged. Closes #437.
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
New Features
-and/in non-leading positions across Go, Java, and JavaScript implementations.Tests