Coinciding filter results for QP sidebar #5121
Conversation
benjaminpkane
added
enhancement
WalkthroughThe pull request introduces several modifications across multiple files, primarily focused on enhancing the filtering capabilities in the application's state management and query handling. Key changes include the introduction of new parameters and variables in selector families, updates to function signatures, and adjustments to input types in the GraphQL schema. These changes collectively refine how filters are applied and managed in the application, ensuring that queries can be executed with more precise filtering criteria. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
benjaminpkane
force-pushed
the
bugfix/qp-string-filter-results
branch
from
3d82cda
to
f82afdd
Compare
benjaminpkane
force-pushed
the
bugfix/qp-string-filter-results
branch
from
f82afdd
to
3f03ea5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
app/packages/core/src/components/Filters/StringFilter/useSelected.ts (1)
Line range hint
1-43: Consider adding unit tests for frame field filtering logicThe function handles complex conditional logic for determining search behavior, particularly around frame fields and label tags. This warrants comprehensive unit testing to ensure reliability.
Consider adding tests for the following scenarios:
- Frame field with label tags
- Non-frame field with label tags
- Query performance enabled/disabled states
Would you like me to help create these unit tests?
app/packages/core/src/components/Filters/StringFilter/state.ts (1)
71-74: LGTM! Consider adding type safety to filters object creation.The implementation cleanly filters out the current path from filters without mutation. However, consider adding explicit typing for better maintainability.
+type Filters = Record<string, unknown>; const filters = Object.fromEntries( - Object.entries(get(fos.filters) || {}).filter(([p]) => p !== path) + Object.entries(get(fos.filters) as Filters || {}).filter(([p]) => p !== path) );app/packages/state/src/recoil/types.ts (1)
169-171: Consider adding JSDoc documentation for the Filter interfaceThe new
Filterinterface provides a flexible key-value structure for filters. However, adding JSDoc documentation would help other developers understand:
- The purpose of this interface
- Expected key formats
- Valid use cases for array vs single values
Example documentation:
/** * Represents a filter configuration where each key is a filter criterion * and the value can be either a single value or an array of values. * @example * { * "status": "active", * "tags": ["important", "urgent"] * } */app/schema.graphql (1)
458-458: Consider adding documentation for the filters fieldThe addition of the optional
filtersfield of typeBSONis well-structured and consistent with the schema's patterns. However, similar to how theexcludefield has documentation, consider adding a description for thefiltersfield to clarify its purpose and expected format.Add documentation like this:
search: String = null + """ + Additional filtering criteria for the results + """ filters: BSON = null
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/packages/relay/src/queries/__generated__/lightningQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**
📒 Files selected for processing (7)
app/packages/core/src/components/Filters/StringFilter/state.ts(1 hunks)app/packages/core/src/components/Filters/StringFilter/useSelected.ts(1 hunks)app/packages/state/src/recoil/filters.ts(1 hunks)app/packages/state/src/recoil/pathData/lightningString.ts(1 hunks)app/packages/state/src/recoil/types.ts(1 hunks)app/schema.graphql(1 hunks)fiftyone/server/lightning.py(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/packages/state/src/recoil/filters.ts
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/core/src/components/Filters/StringFilter/state.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/components/Filters/StringFilter/useSelected.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/recoil/pathData/lightningString.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/recoil/types.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (7)
app/packages/state/src/recoil/pathData/lightningString.ts (1)
Line range hint 16-36: Verify the handling of filters parameter in lightningQuery
The implementation doesn't show how the filters parameter is used in the query. Let's verify that the lightningQuery properly handles this parameter.
✅ Verification successful
Let me gather more information about the filters parameter and its handling.
Let me search for GraphQL schema files and the actual implementation of the lightning query.
Based on the GraphQL schema and implementation details found, I can now provide a final response:
The filters parameter in lightningQuery is properly handled
The GraphQL schema shows that filters is an optional BSON parameter in the LightningPathInput type, which is correctly passed through to the backend resolver. The Python implementation in fiftyone/server/lightning.py confirms that the filters are processed at the resolver level.
- The
filtersparameter is properly defined in the GraphQL schema asfilters: BSON = null - The backend resolver in
lightning_resolverhandles the input parameters including filters - The implementation in the review comment correctly passes the filters parameter to
lightningQuery
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how lightningQuery handles the filters parameter
# Search for lightningQuery implementation and its usage
ast-grep --pattern 'const lightningQuery = $_'
# Search for other usages of lightningStringResults to ensure they're updated
rg -A 5 'lightningStringResults'
Length of output: 3657
Script:
#!/bin/bash
# Search for the lightning query definition to see the expected parameters
rg -A 10 'lightning = ' --type graphql
# Search for the LightningInput type definition
rg -A 10 'LightningInput' --type graphql
# Check if there are any other implementations using filters parameter
ast-grep --pattern 'filters: $_'
Length of output: 573
Script:
#!/bin/bash
# Find and check GraphQL schema files
fd '\.(graphql|gql)$' -x cat {}
# Search for lightning query implementation in Python
rg -A 10 'def lightning'
# Search for any TypeScript type definitions related to lightning query
rg -A 5 'type LightningInput' --type typescript
Length of output: 16136
app/packages/core/src/components/Filters/StringFilter/useSelected.ts (2)
32-32: LGTM: Clean state selector usage
The introduction of isFrameField follows React and Recoil best practices by using useRecoilValue hook at the component level.
38-40: Verify the impact of this condition on frame-level filtering
The condition has been expanded to exclude search functionality for label tags when dealing with frame fields. This change could affect the user experience for frame-level filtering.
Let's verify the usage of this condition across the codebase:
✅ Verification successful
Based on the analysis of the codebase, I can now provide a definitive response about the impact of the condition on frame-level filtering:
The condition correctly handles frame-level filtering and maintains consistent behavior
The code shows that:
- The
isFrameFieldselector is properly used across the codebase to identify frame fields - The condition
path === "_label_tags" && queryPerformance && !isFrameField && !modalspecifically excludes frame fields from the label tags search functionality, which is the intended behavior - This aligns with the broader codebase's handling of frame fields and label tags, where they are treated as distinct entities with separate filtering logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other occurrences of label tags filtering with frame fields
# to ensure consistent behavior
# Search for related frame field conditions
rg -l "_label_tags.*isFrameField|isFrameField.*_label_tags"
# Search for frame field filtering logic
ast-grep --pattern 'useRecoilValue($$$isFrameField$$$)'
Length of output: 188
Script:
#!/bin/bash
# Let's check the broader context of frame field usage and label tags filtering
# Check for frame field related code
rg "isFrameField" -A 5 -B 5
# Look for label tags filtering implementations
rg "_label_tags" -A 5 -B 5
# Search for frame-level filtering related code
ast-grep --pattern 'const $_ = useRecoilValue($_FrameState)'
Length of output: 134013
app/packages/core/src/components/Filters/StringFilter/state.ts (1)
81-81: Verify the integration with lightningStringResults.
The filters integration looks correct, but let's verify that lightningStringResults properly handles the new filters parameter across different scenarios.
✅ Verification successful
Let me gather more information about the implementation of lightningStringResults to verify its integration.
Let me check one more aspect to verify the complete integration.
Integration with lightningStringResults is properly implemented
The verification shows that:
- The
lightningStringResultsselector is correctly defined with thefiltersparameter in its type definition - It's properly integrated with the underlying
lightningQueryGraphQL selector - The usage in StringFilter/state.ts correctly passes the filters parameter
- The implementation follows the established pattern for handling query results
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the lightningStringResults implementation and usage
# Expected: Find all usages and verify the filters parameter is handled consistently
# Check the implementation of lightningStringResults
ast-grep --pattern 'export const lightningStringResults = selectorFamily({
$$$
get: ({ path, search, exclude, filters }: { $$$, filters: $$ }) => $$$
})'
# Find all other usages of lightningStringResults
rg -A 5 'lightningStringResults\('
Length of output: 771
Script:
#!/bin/bash
# Search for the definition and implementation of lightningStringResults
rg -l "lightningStringResults" --type ts
# Once we find the files, let's look at the implementation
rg -A 20 "export const lightningStringResults = selectorFamily"
# Also check for any test files related to this
fd -e test.ts -e spec.ts -x rg -l "lightningStringResults" {}
Length of output: 2059
Script:
#!/bin/bash
# Check the lightningQuery implementation since it's used in lightningStringResults
rg -A 10 "export const lightningQuery = " --type ts
# Also check for the StringLightningResult type definition
rg "type StringLightningResult" --type ts -A 5
Length of output: 982
app/packages/state/src/recoil/types.ts (2)
167-168: LGTM: Well-defined union type for filter values
The FilterValues type effectively covers all possible primitive value types that could be used in filters, including handling of null/undefined cases.
169-171: Verify the impact of the new Filter interface
The change from CategoricalFilter to this new Filter interface is a significant architectural change.
Let's verify the impact:
app/schema.graphql (1)
458-458: LGTM! The change is well-integrated and backwards compatible
The addition of the optional filters field is:
- Type-safe: Uses the existing
BSONscalar type - Well-integrated: Works with the
lightningquery throughLightningInput - Backwards compatible: Optional field with null default
Let's verify the usage of this new field:
✅ Verification successful
The filters: BSON field addition is consistent with existing schema patterns
The verification confirms that:
- The
BSONscalar is properly defined in the schema - The
filtersfield follows an established pattern, as similar optionalBSONfilter fields exist in other input types (AggregationForm,samplesquery) - The implementation is consistent with the codebase's filtering approach
- The change maintains backward compatibility as it's optional with a null default value
The existing GraphQL query implementation in app/packages/relay/src/queries/lightning.ts will continue to work without modifications since the field is optional.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing GraphQL queries that might need updates
# Look for GraphQL queries using the lightning query
rg -t ts -t js "query.*lightning.*\(" -A 10
# Look for any TypeScript/JavaScript files that might need updates for the new filters field
rg -t ts -t js "LightningPathInput"
Length of output: 2497
Script:
#!/bin/bash
# Let's check the actual implementation to understand how filters are used
# Check for any existing usage of filters in GraphQL queries
rg -t ts -t js "filters.*LightningPathInput" -A 5 -B 5
# Look for any BSON type usage to understand the context
rg -t graphql "scalar BSON"
rg -t graphql "type.*BSON" -A 5
Length of output: 184
Script:
#!/bin/bash
# Let's try without file type restrictions since GraphQL isn't recognized
rg "scalar BSON"
rg "filters.*LightningPathInput" -A 5 -B 5
# Check the schema file directly
cat app/schema.graphql | grep -A 10 -B 10 "filters: BSON"
Length of output: 1397
| path: string; | ||
| search?: string; | ||
| exclude?: string[]; | ||
| filters: SerializableParam; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making filters parameter optional for backward compatibility
The filters parameter is marked as required, which could break existing code that doesn't provide filters. Consider making it optional (filters?: SerializableParam) to maintain backward compatibility.
- filters: SerializableParam;
+ filters?: SerializableParam;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filters: SerializableParam; | |
| filters?: SerializableParam; |
| @@ -37,6 +39,7 @@ class LightningPathInput: | |||
| ) | |||
| first: t.Optional[int] = foc.LIST_LIMIT | |||
| search: t.Optional[str] = None | |||
| filters: t.Optional[BSON] = None | |||
There was a problem hiding this comment.
Ensure proper validation and sanitization of user-provided filters
The filters field in LightningPathInput is being added to accept user-provided filters. It's crucial to validate and sanitize filters before using them in database queries to prevent injection attacks and ensure the security of the application.
| @@ -154,6 +157,7 @@ | |||
| is_object_id_field: bool | |||
| exclude: t.Optional[t.List[str]] = None | |||
| search: t.Optional[str] = None | |||
| filters: t.Optional[BSON] = None | |||
There was a problem hiding this comment.
Validate filters in DistinctQuery to prevent security vulnerabilities
The filters attribute in DistinctQuery holds user-provided data. Ensure that filters are properly validated and sanitized before incorporating them into database queries to prevent potential injection attacks.
| if query.search: | ||
| if query.is_object_id_field: | ||
| add = (_TWENTY_FOUR - len(query.search)) * "0" | ||
| value = {"$gte": ObjectId(f"{query.search}{add}")} | ||
| else: | ||
| value = Regex(f"^{query.search}") | ||
| pipeline.append({"$match": {query.path: value}}) | ||
|
|
There was a problem hiding this comment.
Sanitize query.search to prevent Regular Expression Denial of Service (ReDoS) attacks
When constructing regex patterns from user input in query.search, it's important to escape any special characters to prevent ReDoS attacks.
Apply this diff to escape special characters in query.search:
+import re
...
else:
- value = Regex(f"^{query.search}")
+ escaped_search = re.escape(query.search)
+ value = Regex(f"^{escaped_search}")This change ensures that any special characters in query.search are escaped, mitigating the risk of ReDoS attacks.
Committable suggestion skipped: line range outside the PR's diff.
* coinciding filters results (#5121) * update deserializer to include additional fields; improve bounds checks * refactor to reduce code duplication --------- Co-authored-by: Benjamin Kane <[email protected]> Co-authored-by: Tom Schmidt <[email protected]>
What changes are proposed in this pull request?
Screen.Recording.2024-11-14.at.9.25.11.PM.mov
How is this patch tested? If it is not, please explain why.
Manual
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
filtersparameter.LightningPathInputto include optional filtering criteria.Improvements
Bug Fixes