diff --git a/.agent/skills/webstatus-search-grammar/SKILL.md b/.agent/skills/webstatus-search-grammar/SKILL.md index 3e9ecda14..0fc4025bd 100644 --- a/.agent/skills/webstatus-search-grammar/SKILL.md +++ b/.agent/skills/webstatus-search-grammar/SKILL.md @@ -26,7 +26,7 @@ For a technical breakdown of the ANTLR grammar, search node transformation, and - Add the new term to the `search_criteria` rule in the grammar file (e.g., add `| discouraged_term`). - Define the new rule: `discouraged_term: 'is' ':' 'discouraged';`. 2. **Regenerate Parser**: - - Run `make antlr-gen`. This will update the files in `lib/gen/featuresearch/parser/`. + - Run `make antlr-gen`. This will update the files in `lib/gen/featuresearch/parser/`. Note: If you do not have the `devcontainer` open or do not have the right Java dependency versions, **ask the user** to run this step for you. 3. **Update Visitor (`lib/gcpspanner/searchtypes/`)**: - Add a new `SearchIdentifier` for your term in `searchtypes.go` (e.g., `IdentifierIsDiscouraged`). - In **[FeaturesSearchVisitor.go](../../lib/gcpspanner/searchtypes/features_search_visitor.go)**, implement the `VisitDiscouraged_termContext` method. This visitor is the **source-of-truth** for how grammar terms are translated into Spanner SQL. @@ -46,3 +46,9 @@ When you add a new search grammar term or modify parsing: - Trigger the "Updating the Knowledge Base" prompt in `GEMINI.md` to ensure I am aware of the changes. - Ensure that `docs/ARCHITECTURE.md` is updated if there are broader system impacts. + +## Error Handling & Query Validation + +- **Query Execution Errors:** When translating search logic into Spanner SQL or exploring `saved:` references via `expandSavedSearches`, unexpected edge cases (e.g. `backendtypes.ErrSavedSearchNotFound`, or cyclic references) MUST NOT crash the request and MUST NOT return `500 Internal Server Error`. +- **400 Bad Request Mapping:** Endpoint handlers (e.g., `get_features.go`, `create_saved_search.go`) MUST explicitly catch these semantic validation errors and map them to HTTP `400 Bad Request` JSON responses. The frontend relies on these 400s to render in-app warning banners for invalid subsets of an otherwise valid query string. +- **Reference Validation:** Always ensure that `ValidateQueryReferences` (or equivalent dry-run query validators) intercepts invalid cycles or missing queries before a database mutation is committed. diff --git a/antlr/FeatureSearch.g4 b/antlr/FeatureSearch.g4 index e3db3f4c9..b6bfc4a48 100644 --- a/antlr/FeatureSearch.g4 +++ b/antlr/FeatureSearch.g4 @@ -41,6 +41,8 @@ description_term: 'desc' COLON ANY_VALUE; group_term: 'group' COLON ANY_VALUE; snapshot_term: 'snapshot' COLON ANY_VALUE; id_term: 'id' COLON ANY_VALUE; +saved_term: 'saved' COLON ANY_VALUE; +hotlist_term: 'hotlist' COLON ANY_VALUE; term: available_date_term | available_on_term @@ -50,6 +52,8 @@ term: | id_term | snapshot_term | description_term + | saved_term + | hotlist_term | name_term; date_range_query: startDate = DATE '..' endDate = DATE; diff --git a/lib/gcpspanner/feature_search.go b/lib/gcpspanner/feature_search.go index 43e7964fc..1a1c38fa6 100644 --- a/lib/gcpspanner/feature_search.go +++ b/lib/gcpspanner/feature_search.go @@ -119,10 +119,14 @@ func (c *Client) FeaturesSearch( ) (*FeatureResultPage, error) { // Build filterable filterBuilder := NewFeatureSearchFilterBuilder() - filter := filterBuilder.Build(searchNode) + filter, err := filterBuilder.Build(searchNode) + if errors.Is(err, ErrNilSearchNode) { + filter = nil + } else if err != nil { + return nil, errors.Join(ErrInternalQueryFailure, err) + } var offsetCursor *FeatureResultOffsetCursor - var err error if pageToken != nil { offsetCursor, err = decodeInputFeatureResultCursor(*pageToken) if err != nil { diff --git a/lib/gcpspanner/feature_search_query.go b/lib/gcpspanner/feature_search_query.go index 6fc7231a3..57dd126a0 100644 --- a/lib/gcpspanner/feature_search_query.go +++ b/lib/gcpspanner/feature_search_query.go @@ -15,6 +15,7 @@ package gcpspanner import ( + "errors" "fmt" "maps" "strings" @@ -24,6 +25,12 @@ import ( "github.com/GoogleChrome/webstatus.dev/lib/gcpspanner/searchtypes" ) +// ErrNilSearchNode is returned when a nil search node is passed to the builder. +var ErrNilSearchNode = errors.New("search node cannot be nil") + +// ErrInvalidRootNode is returned when the root node of the search AST is invalid. +var ErrInvalidRootNode = errors.New("invalid root node in search AST") + type FeatureSearchFilterBuilder struct { paramCounter int params map[string]any @@ -62,37 +69,46 @@ func (b *FeatureSearchFilterBuilder) addParamGetName(param any) string { } // Build constructs a Spanner query for the FeaturesSearch function. -func (b *FeatureSearchFilterBuilder) Build(node *searchtypes.SearchNode) *FeatureSearchCompiledFilter { +func (b *FeatureSearchFilterBuilder) Build(node *searchtypes.SearchNode) (*FeatureSearchCompiledFilter, error) { // Ensure it is not nil - if node == nil || - // Check for our root node. - node.Keyword != searchtypes.KeywordRoot || + if node == nil { + return nil, ErrNilSearchNode + } + // Check for our root node. + if node.Keyword != searchtypes.KeywordRoot || // Currently root should only have at most one child. // lib/gcpspanner/searchtypes/features_search_visitor.go len(node.Children) != 1 { - return nil + return nil, ErrInvalidRootNode } // Initialize the map and (re)set counter to 0 b.params = make(map[string]any) b.paramCounter = 0 - generatedFilters := b.traverseAndGenerateFilters(node.Children[0]) + generatedFilters, err := b.traverseAndGenerateFilters(node.Children[0]) + if err != nil { + return nil, err + } return &FeatureSearchCompiledFilter{ params: b.params, filters: generatedFilters, - } + }, nil } -func (b *FeatureSearchFilterBuilder) traverseAndGenerateFilters(node *searchtypes.SearchNode) []string { +func (b *FeatureSearchFilterBuilder) traverseAndGenerateFilters(node *searchtypes.SearchNode) ([]string, error) { var filters []string switch { case node.IsKeyword(): // Handle AND/OR keyword childFilters := make([]string, 0, len(node.Children)) // Collect child filters first for _, child := range node.Children { - childFilters = append(childFilters, b.traverseAndGenerateFilters(child)...) + res, err := b.traverseAndGenerateFilters(child) + if err != nil { + return nil, err + } + childFilters = append(childFilters, res...) } // Join child filters using the current node's operator @@ -113,7 +129,11 @@ func (b *FeatureSearchFilterBuilder) traverseAndGenerateFilters(node *searchtype // Handle parenthesized sub-expressions. childFilters := make([]string, 0, len(node.Children)) for _, child := range node.Children { - childFilters = append(childFilters, b.traverseAndGenerateFilters(child)...) + res, err := b.traverseAndGenerateFilters(child) + if err != nil { + return nil, err + } + childFilters = append(childFilters, res...) } // Join without spaces because if there are multiple terms, they will @@ -131,6 +151,9 @@ func (b *FeatureSearchFilterBuilder) traverseAndGenerateFilters(node *searchtype case searchtypes.IdentifierAvailableDate: // Currently not a terminal identifier. break + case searchtypes.IdentifierSavedSearch, searchtypes.IdentifierHotlist: + // Handled upstream by ValidateQueryReferences and recursive expansion. + return nil, fmt.Errorf("unexpected unexpanded search identifier: %s", node.Term.Identifier) case searchtypes.IdentifierAvailableOn: filter = b.availabilityFilter(node.Term.Value, node.Term.Operator) case searchtypes.IdentifierName: @@ -155,7 +178,7 @@ func (b *FeatureSearchFilterBuilder) traverseAndGenerateFilters(node *searchtype } } - return filters + return filters, nil } func searchOperatorToSpannerBinaryOperator(in searchtypes.SearchOperator) string { diff --git a/lib/gcpspanner/feature_search_query_test.go b/lib/gcpspanner/feature_search_query_test.go index 3ac75741a..c085a2a9d 100644 --- a/lib/gcpspanner/feature_search_query_test.go +++ b/lib/gcpspanner/feature_search_query_test.go @@ -15,6 +15,7 @@ package gcpspanner import ( + "errors" "reflect" "slices" "testing" @@ -49,6 +50,25 @@ var ( }, } + unexpandedSavedSearchQuery = TestTree{ + Query: "saved:XYZ", + InputTree: &searchtypes.SearchNode{ + Keyword: searchtypes.KeywordRoot, + Term: nil, + Children: []*searchtypes.SearchNode{ + { + Term: &searchtypes.SearchTerm{ + Identifier: searchtypes.IdentifierSavedSearch, + Operator: searchtypes.OperatorNone, + Value: "XYZ", + }, + Children: nil, + Keyword: searchtypes.KeywordNone, + }, + }, + }, + } + simpleNameQuery = TestTree{ Query: `name:"CSS Grid"`, InputTree: &searchtypes.SearchNode{ @@ -537,7 +557,10 @@ WHERE BrowserName = @param0) AND (fbs.Status = @param1 OR fbs.Status = @param2)) for _, tc := range testCases { t.Run(tc.inputTestTree.Query, func(t *testing.T) { b := NewFeatureSearchFilterBuilder() - filter := b.Build(tc.inputTestTree.InputTree) + filter, err := b.Build(tc.inputTestTree.InputTree) + if err != nil { + t.Fatalf("Build failed: %v", err) + } if !slices.Equal[[]string](filter.Filters(), tc.expectedClauses) { t.Errorf("\nexpected clause [%s]\n actual clause [%s]", tc.expectedClauses, filter.Filters()) } @@ -547,3 +570,32 @@ WHERE BrowserName = @param0) AND (fbs.Status = @param1 OR fbs.Status = @param2)) }) } } + +func TestBuild_Error(t *testing.T) { + b := NewFeatureSearchFilterBuilder() + _, err := b.Build(unexpandedSavedSearchQuery.InputTree) + if err == nil { + t.Fatal("expected error for unexpanded saved search, got nil") + } + expectedErr := "unexpected unexpanded search identifier: saved" + if err.Error() != expectedErr { + t.Errorf("expected error %q, got %q", expectedErr, err.Error()) + } + + // Test nil node + _, err = b.Build(nil) + if !errors.Is(err, ErrNilSearchNode) { + t.Errorf("expected error %v, got %v", ErrNilSearchNode, err) + } + + // Test invalid root node + invalidRootNode := &searchtypes.SearchNode{ + Keyword: searchtypes.KeywordNone, + Term: nil, + Children: nil, + } + _, err = b.Build(invalidRootNode) + if !errors.Is(err, ErrInvalidRootNode) { + t.Errorf("expected error %v, got %v", ErrInvalidRootNode, err) + } +} diff --git a/lib/gcpspanner/searchtypes/features_search_parse_test.go b/lib/gcpspanner/searchtypes/features_search_parse_test.go index abfd751e9..28878b170 100644 --- a/lib/gcpspanner/searchtypes/features_search_parse_test.go +++ b/lib/gcpspanner/searchtypes/features_search_parse_test.go @@ -1592,6 +1592,42 @@ func TestParseQuery(t *testing.T) { }, }, }, + { + InputQuery: "saved:my-saved-query", + ExpectedTree: &SearchNode{ + Keyword: KeywordRoot, + Term: nil, + Children: []*SearchNode{ + { + Children: nil, + Term: &SearchTerm{ + Identifier: IdentifierSavedSearch, + Value: "my-saved-query", + Operator: OperatorEq, + }, + Keyword: KeywordNone, + }, + }, + }, + }, + { + InputQuery: "hotlist:my-hotlist", + ExpectedTree: &SearchNode{ + Keyword: KeywordRoot, + Term: nil, + Children: []*SearchNode{ + { + Children: nil, + Term: &SearchTerm{ + Identifier: IdentifierHotlist, + Value: "my-hotlist", + Operator: OperatorEq, + }, + Keyword: KeywordNone, + }, + }, + }, + }, } for _, testCase := range testCases { diff --git a/lib/gcpspanner/searchtypes/features_search_visitor.go b/lib/gcpspanner/searchtypes/features_search_visitor.go index 744000d58..455ada885 100644 --- a/lib/gcpspanner/searchtypes/features_search_visitor.go +++ b/lib/gcpspanner/searchtypes/features_search_visitor.go @@ -148,6 +148,14 @@ func (v *FeaturesSearchVisitor) createIDNode(idNode antlr.TerminalNode) *SearchN return v.createSimpleNode(idNode, IdentifierID, OperatorEq) } +func (v *FeaturesSearchVisitor) createSavedNode(savedNode antlr.TerminalNode) *SearchNode { + return v.createSimpleNode(savedNode, IdentifierSavedSearch, OperatorEq) +} + +func (v *FeaturesSearchVisitor) createHotlistNode(hotlistNode antlr.TerminalNode) *SearchNode { + return v.createSimpleNode(hotlistNode, IdentifierHotlist, OperatorEq) +} + func (v *FeaturesSearchVisitor) createSnapshotNode(snapshotNode antlr.TerminalNode) *SearchNode { return v.createSimpleNode(snapshotNode, IdentifierSnapshot, OperatorEq) } @@ -502,6 +510,16 @@ func (v *FeaturesSearchVisitor) VisitId_term(ctx *parser.Id_termContext) any { return v.createIDNode(ctx.ANY_VALUE()) } +// nolint: revive // Method signature is generated. +func (v *FeaturesSearchVisitor) VisitSaved_term(ctx *parser.Saved_termContext) any { + return v.createSavedNode(ctx.ANY_VALUE()) +} + +// nolint: revive // Method signature is generated. +func (v *FeaturesSearchVisitor) VisitHotlist_term(ctx *parser.Hotlist_termContext) any { + return v.createHotlistNode(ctx.ANY_VALUE()) +} + // nolint: revive // Method signature is generated. func (v *FeaturesSearchVisitor) VisitSnapshot_term(ctx *parser.Snapshot_termContext) any { return v.createSnapshotNode(ctx.ANY_VALUE()) diff --git a/lib/gcpspanner/searchtypes/searchtypes.go b/lib/gcpspanner/searchtypes/searchtypes.go index fec1436ed..a3b52d412 100644 --- a/lib/gcpspanner/searchtypes/searchtypes.go +++ b/lib/gcpspanner/searchtypes/searchtypes.go @@ -103,4 +103,6 @@ const ( IdentifierGroup SearchIdentifier = "group" IdentifierSnapshot SearchIdentifier = "snapshot" IdentifierID SearchIdentifier = "id" + IdentifierSavedSearch SearchIdentifier = "saved" + IdentifierHotlist SearchIdentifier = "hotlist" )