fix(validate): Check attributes in query even during error (#118018)

- Even when there's an error check the attributes
- Parse errors will still cause an early exit and attributes to not be
checked though

Author: wmak

src/sentry/api/endpoints/organization_events_validate.py

@@ -45,6 +45,11 @@ class AttributeValidation(NamedValidation):
     attrType: str | None
 
 
+@dataclass(kw_only=True)
+class QueryValidation(Validation):
+    fields: list[AttributeValidation] = dataclass_field(default_factory=list)
+
+
 @dataclass(kw_only=True)
 class ValidationResponse:
     valid: bool
@@ -53,7 +58,7 @@ class ValidationResponse:
     field: list[AttributeValidation] = dataclass_field(default_factory=list)
     orderby: list[AttributeValidation] = dataclass_field(default_factory=list)
     projects: list[Validation] = dataclass_field(default_factory=list)
-    query: list[Validation | AttributeValidation] = dataclass_field(default_factory=list)
+    query: QueryValidation
 
 
 def serialize_type(search_type: constants.SearchType) -> str:
@@ -187,7 +192,7 @@ def get(self, request: Request, organization: Organization) -> Response:
         if not self.has_feature(organization, request):
             return Response(status=400)
 
-        response = ValidationResponse(valid=True)
+        response = ValidationResponse(valid=True, query=QueryValidation(valid=True, error=None))
 
         try:
             snuba_params = self.get_snuba_params(
@@ -239,23 +244,36 @@ def get(self, request: Request, organization: Organization) -> Response:
 
         # Validate query
         query_string = request.GET.get("query", "")
-        query_validity: list[AttributeValidation] = []
         query_attributes_to_lookup: dict[AttributeKey.Type.ValueType, list[ResolvedAttribute]] = {}
         query_columns = []
         try:
-            # While resolve_query also runs parse_search_query, we don't need the resolved_query just want to dry-run it
-            # to get any errors
-            resolver.resolve_query(query_string)
-            parsed_terms = resolver.parse_search_query(query_string)
+            try:
+                parsed_terms = resolver.parse_search_query(query_string)
+            except InvalidSearchQuery as err:
+                # If we fail to parse, try again but truncate the query to hopefully get some terms
+                if err.extra is not None:
+                    try:
+                        parsed_terms = resolver.parse_search_query(
+                            query_string[: err.extra.get("idx", 0) - 1]
+                        )
+                    except InvalidSearchQuery:
+                        # If we fail again don't bubble the error up
+                        parsed_terms = []
+                else:
+                    parsed_terms = []
             query_columns = resolver.collect_terms(parsed_terms)
-            query_validity, query_attributes_to_lookup, valid = self.validate_columns(
+            response.query.fields, query_attributes_to_lookup, valid = self.validate_columns(
                 query_columns, resolver
             )
             if not valid:
                 response.valid = valid
+            # While resolve_query also runs parse_search_query, we don't need the resolved_query just want to dry-run it
+            # to get any errors
+            resolver.resolve_query(query_string)
         except InvalidSearchQuery as error:
             response.valid = False
-            response.query.append(Validation(error=str(error), valid=False))
+            response.query.error = str(error)
+            response.query.valid = False
 
         # Lookup unknown fields and add to validities
         # Combine the lookup dictionaries
@@ -297,12 +315,18 @@ def get(self, request: Request, organization: Organization) -> Response:
                             column_validity.append(validity)
                         if (
                             resolved.public_alias in query_columns
-                            and validity not in query_validity
+                            and validity not in response.query.fields
                         ):
-                            query_validity.append(validity)
+                            response.query.fields.append(validity)
 
         response.field.extend(column_validity)
-        response.query.extend(query_validity)
+        # If the response is still valid check if there's a field validity we wanna use
+        if response.query.valid:
+            for field in response.query.fields:
+                if not field.valid:
+                    response.query.valid = False
+                    response.query.error = field.error
+                    break
 
         # Validate orderby
         orderby_validity = []

src/sentry/api/event_search.py

@@ -2018,12 +2018,14 @@ def parse_search_query(
         idx = e.column()
         prefix = query[max(0, idx - 5) : idx]
         suffix = query[idx : (idx + 5)]
-        raise InvalidSearchQuery(
+        err = InvalidSearchQuery(
             "{} {}".format(
                 f"Parse error at '{prefix}{suffix}' (column {e.column():d}).",
                 "This is commonly caused by unmatched parentheses. Enclose any text in double quotes.",
-            )
+            ),
         )
+        err.extra = {"idx": idx}
+        raise err
 
     return SearchVisitor(
         config,

src/sentry/exceptions.py

@@ -1,3 +1,5 @@
+from typing import Any
+
 from django.core.exceptions import SuspiciousOperation
 from rest_framework.exceptions import ParseError
 
@@ -61,6 +63,7 @@ class ApiTokenLimitError(Exception):
 
 
 class InvalidSearchQuery(Exception):
+    extra: dict[str, Any] | None = None
     pass
 
 

tests/sentry/api/endpoints/test_organization_events_validate.py

@@ -345,33 +345,56 @@ def test_invalid_query(self) -> None:
                 "project": [self.project.id],
                 "dataset": "spans",
                 "field": ["span.duration"],
-                "query": "Thing AND",
+                "query": "project:foo AND",
             }
         )
 
         assert response.status_code == 400, response.content
         assert not response.data["valid"]
-        assert response.data["query"] == [
-            {"valid": False, "error": "Condition is missing on the right side of 'AND' operator"}
-        ]
+        assert response.data["query"] == {
+            "valid": False,
+            "error": "Condition is missing on the right side of 'AND' operator",
+            "fields": [
+                {"attrType": "string", "error": None, "name": "project", "valid": True},
+            ],
+        }
 
         response = self.do_request(
             {
                 "project": [self.project.id],
                 "dataset": "spans",
                 "field": ["span.duration"],
-                "query": "span.duration:>hello",
+                "query": "project:foo AND p90(hello",
             }
         )
 
         assert response.status_code == 400, response.content
         assert not response.data["valid"]
-        assert response.data["query"] == [
+        assert response.data["query"] == {
+            "valid": False,
+            "error": "Parse error at ' p90(hello' (column 20). This is commonly caused by unmatched parentheses. Enclose any text in double quotes.",
+            "fields": [
+                {"attrType": "string", "error": None, "name": "project", "valid": True},
+                {"attrType": "string", "error": None, "name": "message", "valid": True},
+            ],
+        }
+
+        response = self.do_request(
             {
-                "valid": False,
-                "error": "span.duration: Invalid number: >hello. Expected number then optional k, m, or b suffix (e.g. 500k).",
+                "project": [self.project.id],
+                "dataset": "spans",
+                "field": ["span.duration"],
+                "query": "span.duration:>hello",
             }
-        ]
+        )
+
+        assert response.status_code == 400, response.content
+        assert not response.data["valid"]
+        assert response.data["query"] == {
+            "valid": False,
+            "error": "span.duration: Invalid number: >hello. Expected number then optional k, m, or b suffix (e.g. 500k).",
+            "fields": [],
+        }
 
     def test_valid_query(self) -> None:
         response = self.do_request(
@@ -385,20 +408,24 @@ def test_valid_query(self) -> None:
 
         assert response.status_code == 200, response.content
         assert response.data["valid"]
-        assert response.data["query"] == [
-            {
-                "error": None,
-                "name": "span.duration",
-                "valid": True,
-                "attrType": "number",
-            },
-            {
-                "error": None,
-                "name": "p95(span.duration)",
-                "valid": True,
-                "attrType": "number",
-            },
-        ]
+        assert response.data["query"] == {
+            "valid": True,
+            "error": None,
+            "fields": [
+                {
+                    "error": None,
+                    "name": "span.duration",
+                    "valid": True,
+                    "attrType": "number",
+                },
+                {
+                    "error": None,
+                    "name": "p95(span.duration)",
+                    "valid": True,
+                    "attrType": "number",
+                },
+            ],
+        }
 
     def test_mixed_validity_query(self) -> None:
         response = self.do_request(
@@ -412,32 +439,36 @@ def test_mixed_validity_query(self) -> None:
 
         assert response.status_code == 400, response.content
         assert not response.data["valid"]
-        assert response.data["query"] == [
-            {
-                "error": None,
-                "name": "span.duration",
-                "valid": True,
-                "attrType": "number",
-            },
-            {
-                "error": "Unknown attribute",
-                "name": "hello",
-                "valid": False,
-                "attrType": None,
-            },
-            {
-                "error": "Unknown attribute",
-                "name": "world",
-                "valid": False,
-                "attrType": None,
-            },
-            {
-                "error": "Unknown attribute",
-                "name": "or",
-                "valid": False,
-                "attrType": None,
-            },
-        ]
+        assert response.data["query"] == {
+            "error": "Unknown attribute",
+            "valid": False,
+            "fields": [
+                {
+                    "error": None,
+                    "name": "span.duration",
+                    "valid": True,
+                    "attrType": "number",
+                },
+                {
+                    "error": "Unknown attribute",
+                    "name": "hello",
+                    "valid": False,
+                    "attrType": None,
+                },
+                {
+                    "error": "Unknown attribute",
+                    "name": "world",
+                    "valid": False,
+                    "attrType": None,
+                },
+                {
+                    "error": "Unknown attribute",
+                    "name": "or",
+                    "valid": False,
+                    "attrType": None,
+                },
+            ],
+        }
 
     def test_user_tags_in_storage_for_query(self) -> None:
         self.store_spans(
@@ -460,9 +491,13 @@ def test_user_tags_in_storage_for_query(self) -> None:
 
         assert response.status_code == 200, response.content
         assert response.data["valid"]
-        assert response.data["query"] == [
-            {"error": None, "name": "my.custom.tag", "valid": True, "attrType": "string"},
-        ]
+        assert response.data["query"] == {
+            "error": None,
+            "valid": True,
+            "fields": [
+                {"error": None, "name": "my.custom.tag", "valid": True, "attrType": "string"},
+            ],
+        }
 
     def test_invalid_field_in_fields_and_query(self) -> None:
         response = self.do_request(
@@ -484,14 +519,18 @@ def test_invalid_field_in_fields_and_query(self) -> None:
                 "attrType": None,
             },
         ]
-        assert response.data["query"] == [
-            {
-                "error": "Unknown attribute",
-                "name": "hello",
-                "valid": False,
-                "attrType": None,
-            },
-        ]
+        assert response.data["query"] == {
+            "error": "Unknown attribute",
+            "valid": False,
+            "fields": [
+                {
+                    "error": "Unknown attribute",
+                    "name": "hello",
+                    "valid": False,
+                    "attrType": None,
+                },
+            ],
+        }
 
     def test_multiple_invalid_issues(self) -> None:
         response = self.do_request(
@@ -514,14 +553,18 @@ def test_multiple_invalid_issues(self) -> None:
                 "attrType": None,
             },
         ]
-        assert response.data["query"] == [
-            {
-                "error": "Unknown attribute",
-                "name": "hello",
-                "valid": False,
-                "attrType": None,
-            },
-        ]
+        assert response.data["query"] == {
+            "error": "Unknown attribute",
+            "valid": False,
+            "fields": [
+                {
+                    "error": "Unknown attribute",
+                    "name": "hello",
+                    "valid": False,
+                    "attrType": None,
+                },
+            ],
+        }
         assert response.data["orderby"] == [
             {
                 "error": "Orderby must also be a selected field",