Don't suggest resources #1714
Don't suggest resources #1714
Conversation
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1714 +/- ##
==========================================
+ Coverage 82.15% 82.19% +0.04%
==========================================
Files 334 334
Lines 19489 19535 +46
==========================================
+ Hits 16011 16057 +46
Misses 3478 3478
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // Any type that is not Resource (avoid suggesting resource UUIDs) | ||
| node_filters: vec![ | ||
| RelationNodeFilter { | ||
| node_type: NodeType::Entity.into(), | ||
| ..Default::default() | ||
| }, | ||
| RelationNodeFilter { | ||
| node_type: NodeType::Label.into(), | ||
| ..Default::default() | ||
| }, | ||
| RelationNodeFilter { | ||
| node_type: NodeType::User.into(), | ||
| ..Default::default() | ||
| }, | ||
| ], |
There was a problem hiding this comment.
I think that the expectation for this feature is that only entities are suggested.
There was a problem hiding this comment.
🤔 That makes sense for me as well, should we confirm it with someone else? Or just change it to filter only entities?
There was a problem hiding this comment.
yup, good idea. let me confirm with @ebrehault (a.k.a: our API chief!)
There was a problem hiding this comment.
As another data point: the current tests check that Users are also suggested, but I think it might be a mistake since the comments explicitly mention entities. It's probably just copypasting what the function returned as the expected value without double-checking.
P.S: I still think returning only entities is the right thing to do here.
| @@ -345,7 +347,21 @@ impl ShardReader { | |||
| shard_id: String::default(), // REVIEW: really? | |||
There was a problem hiding this comment.
Since you are modifying this part of the code, do you mind removing this line? Is not necessary since we use ..Default::default() at the end.
javitonino
force-pushed
the
8271-dont-suggest-resources
branch
2 times, most recently
from
af4060a
to
03f9535
Compare
There was a problem hiding this comment.
Nice first contribution to NucliaDB @javitonino 👏🏼 for many more 🍾
Description
Avoid returning suggestions that match a prefix search on the resource ID (can happen under normal use for UUIDs & searches starting with a-f)
How was this PR tested?
Describe how you tested this PR.