Conversation
Agent-Logs-Url: https://github.com/rawkode/prescience/sessions/6715d3fd-a403-48c1-9dd7-c555031c7e92 Co-authored-by: rawkode <145816+rawkode@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the Rust client’s handling of required string fields by rejecting empty/whitespace-only values earlier (at construction time for core types and before dispatch for client request builders), improving error clarity and preventing invalid requests from reaching the gRPC layer.
Changes:
- Change several type constructors (
Relationship,Caveat,RelationshipFilter,SubjectFilter) to returnResult<_, Error>and validate empty/whitespace-only inputs. - Add pre-dispatch validation in permissions-related request builders (e.g.,
check_permission,lookup_*,expand_permission_tree) for required string fields. - Update integration tests for new constructor signatures and add unit tests for validation behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
tests/integration.rs |
Updates integration tests to handle Result-returning constructors via unwrap() where appropriate. |
src/types/relationship.rs |
Adds validation to Relationship::new and Caveat::new, changes them to return Result, and adds unit tests. |
src/types/filter.rs |
Adds validation to RelationshipFilter::new / SubjectFilter::new, changes them to return Result, and adds unit tests. |
src/client/permissions.rs |
Adds early validation for required string fields in request builders and introduces unit tests using a lazy tonic channel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl Caveat { | ||
| /// Creates a new caveat with the given name and context. | ||
| pub fn new(name: impl Into<String>, context: HashMap<String, ContextValue>) -> Self { | ||
| Self { | ||
| name: name.into(), | ||
| context, | ||
| /// | ||
| /// Returns `Err` if `name` is empty or whitespace-only. | ||
| pub fn new(name: impl Into<String>, context: HashMap<String, ContextValue>) -> Result<Self, Error> { | ||
| let name = name.into(); | ||
| if name.trim().is_empty() { | ||
| return Err(Error::InvalidArgument( | ||
| "caveat name must not be empty".into(), | ||
| )); | ||
| } | ||
| Ok(Self { | ||
| name, | ||
| context, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Caveat::new now enforces a non-empty name, but TryFrom<proto::Relationship> (below) still constructs Caveat { name: ... } directly, bypassing this validation. Consider switching that conversion to call Caveat::new(...) (and propagate the error) so invariants hold for values coming from protobufs as well.
| impl Relationship { | ||
| /// Creates a new relationship without a caveat. | ||
| /// | ||
| /// Returns `Err` if `relation` is empty or whitespace-only. | ||
| pub fn new( | ||
| resource: ObjectReference, | ||
| relation: impl Into<String>, | ||
| subject: SubjectReference, | ||
| ) -> Self { | ||
| Self { | ||
| ) -> Result<Self, Error> { | ||
| let relation = relation.into(); | ||
| if relation.trim().is_empty() { | ||
| return Err(Error::InvalidArgument( | ||
| "relation must not be empty".into(), | ||
| )); | ||
| } | ||
| Ok(Self { | ||
| resource, | ||
| relation: relation.into(), | ||
| relation, | ||
| subject, | ||
| optional_caveat: None, | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Relationship::new now rejects empty/whitespace relation, but TryFrom<crate::proto::Relationship> for Relationship currently builds Relationship { relation: proto.relation, ... } directly, which can reintroduce invalid state. To keep the type invariant consistent (similar to ObjectReference::try_from calling ObjectReference::new), update the conversion to use Relationship::new(...) (and also validate any embedded caveat via Caveat::new).
| fn into_future(self) -> Self::IntoFuture { | ||
| Box::pin(async move { | ||
| if self.permission.trim().is_empty() { | ||
| return Err(Error::InvalidArgument( | ||
| "permission must not be empty".into(), | ||
| )); | ||
| } | ||
|
|
There was a problem hiding this comment.
The same trim().is_empty() validation (with identical error strings) is now duplicated across multiple request builders in this module. Consider factoring this into a small helper (e.g., validate_non_empty(field, value)) to avoid message drift and to keep future validations consistent.
| /// Creates a new filter for the given resource type. | ||
| pub fn new(resource_type: impl Into<String>) -> Self { | ||
| Self { | ||
| resource_type: resource_type.into(), | ||
| /// | ||
| /// Returns `Err` if `resource_type` is empty or whitespace-only. | ||
| pub fn new(resource_type: impl Into<String>) -> Result<Self, Error> { | ||
| let resource_type = resource_type.into(); |
There was a problem hiding this comment.
RelationshipFilter::new now validates resource_type, but the struct still exposes pub resource_type, so callers can bypass validation via a struct literal. If the intent is to guarantee non-empty types across the API, consider making the field private (as with ObjectReference) and providing accessors/builders instead.
| impl SubjectFilter { | ||
| /// Creates a new subject filter for the given type. | ||
| pub fn new(subject_type: impl Into<String>) -> Self { | ||
| Self { | ||
| subject_type: subject_type.into(), | ||
| /// | ||
| /// Returns `Err` if `subject_type` is empty or whitespace-only. | ||
| pub fn new(subject_type: impl Into<String>) -> Result<Self, Error> { |
There was a problem hiding this comment.
SubjectFilter::new now validates subject_type, but the struct still exposes pub subject_type, so invalid values can still be created via struct literals (bypassing the constructor). If you want the invariant to hold everywhere, make the field private and expose accessors/builders instead.
|
@copilot address feedback |
…filter fields Agent-Logs-Url: https://github.com/rawkode/prescience/sessions/499f9c83-0b7e-47f9-b9f8-3be344b07102 Co-authored-by: rawkode <145816+rawkode@users.noreply.github.com>
Addressed all 5 review comments in f3d05b1:
|
|
@copilot rebase on main and resolve conflicts |
Agent-Logs-Url: https://github.com/rawkode/prescience/sessions/d01f8e99-a42e-4221-8a0e-a6d95be376b9 Co-authored-by: rawkode <145816+rawkode@users.noreply.github.com>
Done in db3ef42. Merged
|
|
@copilot rebase and conflicts again |
…ew test call sites Agent-Logs-Url: https://github.com/rawkode/prescience/sessions/84848e26-5627-4241-ae87-321d08bb5c3d Co-authored-by: rawkode <145816+rawkode@users.noreply.github.com>
Done in 745601b. Merged the latest
|
|
@copilot nonsense: |
…eliminate GitHub conflict Co-authored-by: rawkode <145816+rawkode@users.noreply.github.com>
... Fixed in dfeb732. The root cause was that previous merge commits only had one parent (they were created with The fix: used |
Several required string fields (
permission,relation,resource_type,subject_type) accepted empty or whitespace-only values, allowing invalid inputs to propagate to the network layer with unhelpful errors.Type constructors — now return
Result<Self, Error>Consistent with
ObjectReference::new()andZedToken::new(), these constructors now validate at construction time:Relationship::new()— rejects empty/whitespacerelationCaveat::new()— rejects empty/whitespacenameRelationshipFilter::new()— rejects empty/whitespaceresource_typeSubjectFilter::new()— rejects empty/whitespacesubject_typeClient request builders — validated before network dispatch
check_permission()— validatespermissionlookup_resources()— validatesresource_typeandpermissionlookup_subjects()— validatespermissionandsubject_typeexpand_permission_tree()— validatespermissionAll rejections return
Error::InvalidArgumentwith a field-specific message before any gRPC call is made.Tests
18 new unit tests added covering empty and whitespace-only rejection for all affected constructors and builders. Client builder tests use a lazily-connected
tonic::Channelto validate without a live SpiceDB instance.