feat: filtering bytearrays with member clause #2425
Conversation
WalkthroughOhayo, sensei! The changes in this pull request modify the Changes
Possibly related PRs
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2425 +/- ##
==========================================
- Coverage 67.92% 67.89% -0.04%
==========================================
Files 364 364
Lines 47832 47927 +95
==========================================
+ Hits 32490 32539 +49
- Misses 15342 15388 +46 ☔ View full report in Codecov by Sentry. |
This change is scoped to the protobuf/grpc for the member clause to support bytearrays (as it is not a primitive). GraphQL is completely separate |
![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
Outside diff range and nitpick comments (1)
crates/torii/grpc/proto/types.proto (1)
115-120: Ohayo sensei! 🌞The new
MemberValuemessage looks great! 👍The
oneofconstruct provides a clean way to represent either a primitive value or a string. This flexibility will be useful in various scenarios.Just a friendly reminder to ensure that the code interacting with
MemberClauseis updated to handle the newMemberValuestructure. This may include updates to serialization, deserialization, and any business logic that relies on thevaluefield.Also, consider adding some unit tests to verify the behavior of
MemberValueand its integration withMemberClause.Keep up the great work! 💪
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- crates/torii/grpc/proto/types.proto (1 hunks)
- crates/torii/grpc/src/server/mod.rs (3 hunks)
- crates/torii/grpc/src/types/mod.rs (4 hunks)
Additional comments not posted (7)
crates/torii/grpc/proto/types.proto (1)
126-126: Ohayo sensei! 🌞The
valuefield update inMemberClauselooks perfect! 🎉Changing the type from
PrimitivetoMemberValueis the right move to support the new functionality.To ensure a smooth integration, please verify the following:
- Update the code that constructs
MemberClauseinstances to provide thevaluefield as aMemberValue.- Modify the query processing logic to handle filtering on both primitive values and strings based on the
MemberValuetype.- Test the changes thoroughly, especially the scenarios where the
valuefield is set as a string.Here's a quick script to help you locate the relevant code:
This script searches for files that import and use the
MemberClausemessage and prints the relevant lines. It should give you a good starting point for the verification process.Let me know if you have any questions! 😊
Verification successful
Ohayo sensei! 🌞
It seems our previous attempt to find
MemberClauseusage didn't go as planned. No worries though, we'll try a different approach! Let's cast a wider net to catch those elusiveMemberClausereferences.This script will:
- Find all .proto files in the repository
- Search within those files for 'MemberClause' or 'member_clause' (case-insensitive)
- Display the filename and the matching lines
Let's see what we can find with this new approach, sensei! 🕵️♂️
Ohayo sensei! 🌞
Great news! The
MemberClausechange looks well-contained. 🎉Our investigation shows that the
MemberClauseis only directly referenced within thetypes.protofile. This means the change fromPrimitivetoMemberValuefor thevaluefield is nicely isolated.Here's what we found:
types.proto: Contains theMemberClausemessage definition and uses it as a field.- No direct usage of
MemberClausein other .proto files.While this is promising, I'd recommend a quick manual check:
- Verify any code that implements or uses the
MemberClausemessage, especially in non-proto files.- Ensure that any serialization/deserialization logic is updated if necessary.
Overall, the change appears to be safe and well-scoped. Nice work, sensei! 👍
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find files that import and use the `MemberClause` message rg --type proto 'MemberClause' -l | xargs -I {} sh -c 'echo "File: {}"; rg --type proto "MemberClause" {}'Length of output: 138
Script:
#!/bin/bash # Find .proto files and search for MemberClause fd -e proto | xargs -I {} sh -c 'echo "File: {}"; rg -i "memberclause|member_clause" {}'Length of output: 275
crates/torii/grpc/src/types/mod.rs (3)
58-62: Ohayo sensei! The newMemberValueenum looks great.It enhances the flexibility of the
MemberClauseto handle bothPrimitiveandStringvalues. This is a nice improvement in the data representation.
294-294: Ohayo sensei! TheMemberClauseupdate is spot on.Using the new
MemberValueenum for thevaluefield aligns perfectly with the enum's purpose. It enablesMemberClauseto handle a wider range of value types. Great work!
308-317: Ohayo sensei! TheFromtrait implementation forMemberValueis awesome.The conversion logic handles both
PrimitiveandStringvariants elegantly, ensuring smooth integration with the protocol buffer types. This is a crucial piece for the overall functionality. Excellent job!crates/torii/grpc/src/server/mod.rs (3)
44-44: Ohayo sensei! This import looks good to me.The
ValueTypeenum import is necessary for the code changes that follow.
508-516: Ohayo sensei! The match statement for handlingvalue_typeis implemented correctly.The code segment properly extracts the comparison value based on the
ValueTypevariant and returns an error ifvalue_typeis missing. Great work!
628-639: Ohayo sensei! This match statement is implemented correctly, just like the previous one.The code segment properly extracts the comparison value based on the
ValueTypevariant and returns an error ifvalue_typeis missing. It's consistent with the similar code at line range 508-516.
| let comparison_value = | ||
| match member_clause.value.ok_or(QueryError::MissingParam("value".into()))?.value_type { | ||
| Some(ValueType::String(value)) => value, | ||
| Some(ValueType::Primitive(value)) => { | ||
| let primitive: Primitive = value.try_into()?; | ||
| primitive.to_sql_value()? | ||
| } | ||
| None => return Err(QueryError::MissingParam("value_type".into()).into()), | ||
| }; |
There was a problem hiding this comment.
Ohayo sensei! Consider refactoring the duplicated code into a separate function.
The code segments at line ranges 508-516 and 628-639 are very similar. They both match on value.value_type to extract the comparison value. To improve code reuse and maintainability, consider extracting the common logic into a separate function. Here's a suggestion:
fn extract_comparison_value(value: &proto::types::Value) -> Result<String, Error> {
match value.value_type {
Some(ValueType::String(value)) => Ok(value),
Some(ValueType::Primitive(value)) => {
let primitive: Primitive = value.try_into()?;
Ok(primitive.to_sql_value()?)
}
None => Err(QueryError::MissingParam("value_type".into()).into()),
}
}Then, you can replace the duplicated code with calls to this function:
let comparison_value = extract_comparison_value(&member_clause.value.ok_or(QueryError::MissingParam("value".into()))?)?;let comparison_value = extract_comparison_value(&member.value.ok_or(QueryError::MissingParam("value".into()))?)?;This will make the code more DRY and easier to maintain. What do you think, sensei?
Also applies to: 628-639
Summary by CodeRabbit
New Features
MemberClauseto support both primitive values and strings, improving data representation flexibility.MemberValueenum for better handling of different value types.Bug Fixes
DojoWorldimplementation by adding type checks formember_clause.valueandmember.value.These updates enhance functionality and reliability within the application, allowing for more versatile data management.