-
Notifications
You must be signed in to change notification settings - Fork 2
refactor: replaces allOf+if with clean oneOf structures #185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe JWT JSON schema ( Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @specs/jwt-schema.json:
- Around line 134-141: The "permissions" array in the JWT schema documents "The
provider address must be unique across all permissions entries," but JSON Schema
(draft-07) cannot enforce uniqueness of a specific property across array items;
update the "permissions" description to state this is a semantic/runtime
constraint (e.g., "Uniqueness of provider address across entries is validated at
runtime") or, if runtime validation already exists, retain the note that
uniqueness is enforced outside the schema; reference the "permissions" property
and the "lease-permission" definition when making the clarification.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
specs/jwt-schema.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: go
- GitHub Check: proto
🔇 Additional comments (7)
specs/jwt-schema.json (7)
17-51: Clean refactoring to reference-based definitions.The extraction of
akash-addressandleasesinto reusable definitions improves maintainability and ensures consistency. The description is correctly preserved alongside the$ref.
53-56: Address pattern is correct.The pattern correctly validates bech32 Akash addresses:
akash1prefix (6 chars) followed by 38 lowercase alphanumeric characters, totaling 44 characters as documented.
57-75: Well-structured action-scope definition.The combination of
minItems: 1,uniqueItems: true, and enum-constrained items correctly enforces a non-empty set of valid, unique actions.
76-90: Clean discriminated union using oneOf.The
oneOfstructure withconstdiscriminators on theaccessfield is the idiomatic JSON Schema pattern for discriminated unions. This aligns well with the PR objective and improves tooling support for type generation.
144-158: Consistent discriminated union pattern.The
lease-permissiondefinition mirrors theleasesstructure appropriately, providing a hierarchical permission model with the same full/scoped/granular access levels at the provider scope.
159-225: Provider permission variants are well-structured.Good reuse of the
akash-addressandaction-scopedefinitions. TheadditionalProperties: falseconstraint on all variants ensures strict validation.
226-270: Deployment permission structure with proper dependency constraint.The
dependencieskeyword correctly enforces thatoseqrequiresgseqto be present. The structure is comprehensive with proper constraints.Minor verification:
dseqhasminimum: 1whilegseqandoseqhaveminimum: 0. If sequence numbers in Akash consistently start from 1 (as deployment sequences appear to), consider aligninggseqandoseqto also haveminimum: 1. If 0-based indexing is intentional for group/order sequences, the current schema is correct.
|
PR failure will be fixed in #184 |
4e52bff to
44c7bed
Compare
📝 Description
Using
oneOfinstead ofallOf + ifmakes it easier to see discriminated union in payload shape. Additionally, it improves tooling support, for example json-schema-to-typescript can generate correct types from oneOf schema and we don't need to write it manually.In terms of validation, it supports the same rules as the previous one. However error messages will be different.
🔧 Purpose of the Change
📌 Related Issues
✅ Checklist
I've updated relevant documentation📎 Notes for Reviewers
[Include any additional context, architectural decisions, or specific areas to focus on]