-
Notifications
You must be signed in to change notification settings - Fork 0
Harden input validation for empty/whitespace string fields across request builders and type constructors #14
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?
Changes from all commits
6816f1c
2d522ac
f3d05b1
db3ef42
745601b
dfeb732
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,24 +7,32 @@ use crate::types::{Relationship, ZedToken}; | |
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub struct RelationshipFilter { | ||
| /// Resource type to filter on. | ||
| pub resource_type: String, | ||
| resource_type: String, | ||
| /// Optional resource ID. | ||
| pub optional_resource_id: Option<String>, | ||
| optional_resource_id: Option<String>, | ||
| /// Optional relation name. | ||
| pub optional_relation: Option<String>, | ||
| optional_relation: Option<String>, | ||
| /// Optional subject filter. | ||
| pub optional_subject_filter: Option<SubjectFilter>, | ||
| optional_subject_filter: Option<SubjectFilter>, | ||
| } | ||
|
|
||
| impl RelationshipFilter { | ||
| /// 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(); | ||
|
Comment on lines
20
to
+24
|
||
| if resource_type.trim().is_empty() { | ||
| return Err(Error::InvalidArgument( | ||
| "resource_type must not be empty".into(), | ||
| )); | ||
| } | ||
| Ok(Self { | ||
| resource_type, | ||
| optional_resource_id: None, | ||
| optional_relation: None, | ||
| optional_subject_filter: None, | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| /// Adds a resource ID filter. | ||
|
|
@@ -44,6 +52,26 @@ impl RelationshipFilter { | |
| self.optional_subject_filter = Some(filter); | ||
| self | ||
| } | ||
|
|
||
| /// Returns the resource type. | ||
| pub fn resource_type(&self) -> &str { | ||
| &self.resource_type | ||
| } | ||
|
|
||
| /// Returns the optional resource ID filter. | ||
| pub fn resource_id_filter(&self) -> Option<&str> { | ||
| self.optional_resource_id.as_deref() | ||
| } | ||
|
|
||
| /// Returns the optional relation filter. | ||
| pub fn relation_filter(&self) -> Option<&str> { | ||
| self.optional_relation.as_deref() | ||
| } | ||
|
|
||
| /// Returns the optional subject filter. | ||
| pub fn subject_filter_ref(&self) -> Option<&SubjectFilter> { | ||
| self.optional_subject_filter.as_ref() | ||
| } | ||
| } | ||
|
|
||
| impl From<&RelationshipFilter> for crate::proto::RelationshipFilter { | ||
|
|
@@ -62,21 +90,29 @@ impl From<&RelationshipFilter> for crate::proto::RelationshipFilter { | |
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub struct SubjectFilter { | ||
| /// The subject object type. | ||
| pub subject_type: String, | ||
| subject_type: String, | ||
| /// Optional subject object ID. | ||
| pub optional_subject_id: Option<String>, | ||
| optional_subject_id: Option<String>, | ||
| /// Optional relation on the subject. | ||
| pub optional_relation: Option<String>, | ||
| optional_relation: Option<String>, | ||
| } | ||
|
|
||
| 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> { | ||
|
Comment on lines
100
to
+104
|
||
| let subject_type = subject_type.into(); | ||
| if subject_type.trim().is_empty() { | ||
| return Err(Error::InvalidArgument( | ||
| "subject_type must not be empty".into(), | ||
| )); | ||
| } | ||
| Ok(Self { | ||
| subject_type, | ||
| optional_subject_id: None, | ||
| optional_relation: None, | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| /// Adds a subject ID filter. | ||
|
|
@@ -90,6 +126,21 @@ impl SubjectFilter { | |
| self.optional_relation = Some(relation.into()); | ||
| self | ||
| } | ||
|
|
||
| /// Returns the subject type. | ||
| pub fn subject_type(&self) -> &str { | ||
| &self.subject_type | ||
| } | ||
|
|
||
| /// Returns the optional subject ID filter. | ||
| pub fn subject_id_filter(&self) -> Option<&str> { | ||
| self.optional_subject_id.as_deref() | ||
| } | ||
|
|
||
| /// Returns the optional relation filter on the subject. | ||
| pub fn relation_filter(&self) -> Option<&str> { | ||
| self.optional_relation.as_deref() | ||
| } | ||
| } | ||
|
|
||
| impl From<&SubjectFilter> for crate::proto::SubjectFilter { | ||
|
|
@@ -133,3 +184,44 @@ impl ReadRelationshipResult { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn relationship_filter_valid() { | ||
| let f = RelationshipFilter::new("document").unwrap(); | ||
| assert_eq!(f.resource_type(), "document"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn relationship_filter_empty_type_rejected() { | ||
| let err = RelationshipFilter::new("").unwrap_err(); | ||
| assert!(matches!(err, Error::InvalidArgument(_))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn relationship_filter_whitespace_type_rejected() { | ||
| let err = RelationshipFilter::new(" ").unwrap_err(); | ||
| assert!(matches!(err, Error::InvalidArgument(_))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn subject_filter_valid() { | ||
| let f = SubjectFilter::new("user").unwrap(); | ||
| assert_eq!(f.subject_type(), "user"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn subject_filter_empty_type_rejected() { | ||
| let err = SubjectFilter::new("").unwrap_err(); | ||
| assert!(matches!(err, Error::InvalidArgument(_))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn subject_filter_whitespace_type_rejected() { | ||
| let err = SubjectFilter::new(" ").unwrap_err(); | ||
| assert!(matches!(err, Error::InvalidArgument(_))); | ||
| } | ||
| } | ||
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.
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.