feat: Add internal gRPC storage service API (v1beta1) #239
Conversation
WalkthroughThis PR reorganizes the Buf code generation configuration by separating OpenAPI v2 generation into a dedicated template file (buf.gen.swagger.yaml), removes it from the main buf.gen.yaml, updates the Makefile to invoke both templates, and introduces a new comprehensive gRPC storage service specification defining storage operations for tuples, authorization models, stores, assertions, and changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
Related Documentation Checked 7 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
proto/storage/v1beta1/storage.pb.gois excluded by!**/*.pb.goproto/storage/v1beta1/storage_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
.gitignore(1 hunks)Makefile(1 hunks)buf.gen.swagger.yaml(1 hunks)buf.gen.yaml(0 hunks)storage/v1beta1/storage.proto(1 hunks)
💤 Files with no reviewable changes (1)
- buf.gen.yaml
🔇 Additional comments (6)
storage/v1beta1/storage.proto (3)
11-81: Comprehensive and well-documented service definition.The StorageService organization, RPC signatures, and documentation are clear and logically structured. The separation into tuple operations, model operations, store operations, assertions, and health checks is intuitive. Stream semantics (e.g., Read) and pagination patterns (ReadPage) are appropriately chosen.
327-382: StorageErrorReason enum provides structured error handling.The StorageErrorReason enum with mappings to gRPC status codes and the example in the comments (lines 335–341) is a solid pattern for domain-specific error reporting. This enables clients to distinguish storage-layer failures from generic gRPC errors.
5-20: Remove unused import or clarify visibility approach — current suggestion is not valid protobuf syntax.The
google/api/visibility.protoimport is unused. Visibility restrictions for gRPC services are configured in external service config files usingVisibilityRuleentries with selectors, not as proto option statements. The suggested syntaxoption (google.api.visibility).restriction = "INTERNAL"is invalid—google.api.Visibilityis a service config message, not a proto option.If marking StorageService as internal-only is required, use a service config YAML file (not a proto option). Otherwise, remove the unused import.
Likely an incorrect or invalid review comment.
.gitignore (1)
5-5: Minor addition; no concerns.Adding
.devcontainer/to the ignore list is standard practice for development environment configurations.buf.gen.swagger.yaml (1)
1-15: Well-configured OpenAPI v2 template.The configuration cleanly separates Swagger/OpenAPI v2 generation into a dedicated template, enabling the two-generation flow defined in the Makefile. Plugin version is pinned, output path aligns with downstream processing (patch-swagger-doc), and
allow_merge=trueis appropriate for multi-proto specifications.Makefile (1)
3-5: Two-step Buf generation correctly excludes internal service from public docs.The split into two
buf generatecommands—one withbuf.gen.yaml(for Go bindings, gRPC, validation, gateway) and one withbuf.gen.swagger.yamlusing--exclude-path storage/v1beta1—is a sound strategy to keep the internal StorageService out of public OpenAPI documentation while still generating its bindings for internal use. Public-facing proto files inopenfga/v1/do not import types fromstorage/v1beta1, so the--exclude-pathapproach is safe and won't create documentation inconsistencies.
| // PaginationOptions controls pagination behavior. | ||
| message PaginationOptions { | ||
| // page_size is the maximum number of items to return per page. | ||
| // Must be greater than 0. Default is determined by the storage backend. | ||
| int32 page_size = 1; | ||
| // from is a continuation token from a previous response. | ||
| // Empty string starts from the beginning. | ||
| string from = 2; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does protobuf proto3 support field-level validation constraints for numeric ranges?
💡 Result:
Short answer: Not in core proto3 itself — but you can add field-level numeric-range constraints using widely used annotation-based plugins and option sets (e.g., protoc-gen-validate / (validate.rules) or Spine/other custom options).
Details (concise):
- proto3 language/spec does not include built‑in validation options for ranges; it only defines types and literals. [1]
- The common approach is to annotate .proto fields with custom options provided by validation plugins (protoc‑gen‑validate supports lt/lte/gt/gte, in/not_in, const, etc., for numeric types). Generated code enforces the constraints. [2]
- Other projects (e.g., Spine) also provide range/min/max options as proto field options you can use. [3]
References:
- Protobuf language/spec (no built-in validation options). [1]
- protoc‑gen‑validate numeric rules docs/examples (lt/lte/gt/gte, in/not_in, const). [2]
- Spine validation options (range/min/max examples). [3]
Document page_size validation and enforce it at runtime or via protoc-gen-validate
proto3 does not enforce field-level constraints; ensure the server rejects page_size <= 0 or add a validation rule such as:
int32 page_size = 1 [(validate.rules).int32 = {gt: 0}];
🤖 Prompt for AI Agents
In storage/v1beta1/storage.proto around lines 291 to 299, the
PaginationOptions.page_size field lacks validation and proto3 won't enforce >0;
update the .proto to add a protoc-gen-validate rule such as int32 page_size = 1
[(validate.rules).int32 = {gt: 0}]; and/or ensure the server-side handler
rejects page_size <= 0 (returning an appropriate validation error) and applies
the backend default when page_size is omitted or zero.
|
Re:
I don't think we need to - we can use VisibilityRule to hide them from the OpenAPI output |
@rhamzeh I've tried using |
3 participants
Description
This PR introduces a new internal gRPC storage service API that provides direct access to OpenFGA's storage backend operations. The service is designed for internal use and is intentionally excluded from public API documentation.
Rationale
The storage service API provides a gRPC interface to OpenFGA's storage layer, enabling custom datastore implementations.
Changes
New Storage Service API (
storage/v1beta1/storage.proto)StorageService: A comprehensive gRPC service for storage backend operations
Read,ReadPage,ReadUserTuple,ReadUsersetTuples,ReadStartingWithUser,WriteReadAuthorizationModel,ReadAuthorizationModels,FindLatestAuthorizationModel,WriteAuthorizationModelCreateStore,DeleteStore,GetStore,ListStoresWriteAssertions,ReadAssertionsReadChangesIsReadyKey Features:
openfga.v1types for consistencyOnMissingDelete,OnDuplicateInsert)StorageErrorReasonenumConfiguration Changes
buf.gen.swagger.yamlto excludestorage/v1beta1from public OpenAPI documentationbuf.gen.yaml(for code generation) andbuf.gen.swagger.yaml(for docs)Generated Code
Testing
makeReferences
Review Checklist
mainSummary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.