Skip to content

Commit d11793b

Browse files
aliu39evanh
authored andcommitted
ref(replay): improve error messages for invalid searches that raise CouldNotParseValue (#82048)
Closes #80574 CouldNotParseValue is a custom error we raise in [parsers.py](https://github.com/getsentry/sentry/blob/9241bebfa46dc4d8676cc80219c3786b7006acba/src/sentry/replays/lib/new_query/parsers.py). They contain messages useful for users, but we need to include it in this exc handler first. Also adds the parsed value to the msg Relates to - #78636
1 parent ce839ef commit d11793b

File tree

4 files changed

+50
-29
lines changed

4 files changed

+50
-29
lines changed

src/sentry/replays/endpoints/project_replay_clicks_index.py

+3-1
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,9 @@ def handle_search_filters(
213213
except OperatorNotSupported:
214214
raise ParseError(f"Invalid operator specified for `{search_filter.key.name}`")
215215
except CouldNotParseValue:
216-
raise ParseError(f"Could not parse value for `{search_filter.key.name}`")
216+
raise ParseError(
217+
f"Could not parse value '{search_filter.value.value}' for `{search_filter.key.name}`"
218+
)
217219

218220
if look_back == "AND":
219221
look_back = None

src/sentry/replays/lib/new_query/parsers.py

-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ def parse_duration(value: str) -> int:
3030
milliseconds = parse_int(value)
3131
if milliseconds % 1000:
3232
# TODO: remove once we support milliseconds.
33-
# TODO: this error isn't actually returned to the frontend, it's caught and then we raise a ParseError
3433
raise CouldNotParseValue(
3534
f"Replays only supports second-resolution timestamps at this time. Try '{milliseconds // 1000}s' instead."
3635
)

src/sentry/replays/usecases/query/__init__.py

+11-3
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,9 @@ def handle_viewed_by_me_filters(
7171

7272
value = search_filter.value.value
7373
if not isinstance(value, str) or value.lower() not in ["true", "false"]:
74-
raise ParseError(f"Could not parse value for `{search_filter.key.name}`")
74+
raise ParseError(
75+
f"Could not parse value '{search_filter.value.value}' for `{search_filter.key.name}`"
76+
)
7577
value = value.lower() == "true"
7678

7779
if request_user_id is None:
@@ -103,14 +105,20 @@ def handle_search_filters(
103105
# are top level filters they are implicitly AND'ed in the WHERE/HAVING clause. Otherwise
104106
# explicit operators are used.
105107
if isinstance(search_filter, SearchFilter):
108+
106109
try:
107110
condition = search_filter_to_condition(search_config, search_filter)
108111
if condition is None:
109112
raise ParseError(f"Unsupported search field: {search_filter.key.name}")
110113
except OperatorNotSupported:
111114
raise ParseError(f"Invalid operator specified for `{search_filter.key.name}`")
112-
except CouldNotParseValue:
113-
raise ParseError(f"Could not parse value for `{search_filter.key.name}`")
115+
except CouldNotParseValue as e:
116+
err_msg = f"Could not parse value '{search_filter.value.value}' for `{search_filter.key.name}`."
117+
if e.args:
118+
err_msg += (
119+
f" {e.args[0]}" # avoid using str(e) as it may expose stack trace info
120+
)
121+
raise ParseError(err_msg)
114122

115123
if look_back == "AND":
116124
look_back = None

tests/sentry/replays/test_organization_replay_index.py

+36-24
Original file line numberDiff line numberDiff line change
@@ -584,7 +584,9 @@ def test_get_replays_user_filters(self):
584584
self.mock_event_links(seq1_timestamp, project.id, "debug", replay1_id, uuid.uuid4().hex)
585585
)
586586
self.store_replays(
587-
mock_replay_viewed(seq1_timestamp.timestamp(), project.id, replay1_id, viewed_by_id=1)
587+
mock_replay_viewed(
588+
seq1_timestamp.timestamp(), project.id, replay1_id, viewed_by_id=self.user.id
589+
)
588590
)
589591

590592
with self.feature(self.features):
@@ -610,8 +612,6 @@ def test_get_replays_user_filters(self):
610612
"duration:<18s",
611613
"duration:>=17s",
612614
"duration:<=17s",
613-
# "duration:[16s,17s]", # TODO: need to add duration_in_filters in event_search.py
614-
# "!duration:[16s,18s]",
615615
"duration:17000ms", # If duration value is not equal to a whole number of seconds, the endpoint fails.
616616
"duration:<1m",
617617
"duration:<1min",
@@ -682,12 +682,14 @@ def test_get_replays_user_filters(self):
682682
"count_infos:2",
683683
"count_infos:>1",
684684
"count_infos:<3",
685-
"viewed_by_id:1",
686-
"!viewed_by_id:2",
687-
"viewed_by_id:[1,2]",
688-
"seen_by_id:1",
689-
"!seen_by_id:2",
690-
"seen_by_id:[1,2]",
685+
f"viewed_by_id:{self.user.id}",
686+
f"!viewed_by_id:{self.user.id+1}",
687+
f"viewed_by_id:[{self.user.id+3},{self.user.id}]",
688+
f"seen_by_id:{self.user.id}",
689+
f"!seen_by_id:{self.user.id + 1}",
690+
f"seen_by_id:[{self.user.id + 3},{self.user.id}]",
691+
"viewed_by_me:true",
692+
"seen_by_me:true",
691693
]
692694

693695
for query in queries:
@@ -736,8 +738,10 @@ def test_get_replays_user_filters(self):
736738
"!c:*st",
737739
"!activity:8",
738740
"activity:<2",
739-
"viewed_by_id:2",
740-
"seen_by_id:2",
741+
f"viewed_by_id:{self.user.id+1}",
742+
f"seen_by_id:{self.user.id+1}",
743+
"viewed_by_me:false",
744+
"seen_by_me:false",
741745
"user.email:[[email protected]]",
742746
743747
]
@@ -938,7 +942,7 @@ def test_get_replays_filter_bad_operator(self):
938942
with self.feature(self.features):
939943
for query in queries:
940944
response = self.client.get(self.url + f"?field=id&query={query}")
941-
assert response.status_code == 400
945+
assert response.status_code == 400, query
942946

943947
def test_get_replays_filter_bad_value(self):
944948
"""Test replays conform to the interchange format."""
@@ -956,20 +960,28 @@ def test_get_replays_filter_bad_value(self):
956960
with self.feature(self.features):
957961
for query in queries:
958962
response = self.client.get(self.url + f"?query={query}")
959-
assert response.status_code == 400
960-
field = query.split(":")[0]
961-
assert field.encode() in response.content
963+
assert response.status_code == 400, query
962964

963-
# No such thing as a bad field with the tag filtering behavior.
964-
#
965-
# def test_get_replays_filter_bad_field(self):
966-
# """Test replays conform to the interchange format."""
967-
# self.create_project(teams=[self.team])
965+
def test_get_replays_filter_bad_duration_error_messages(self):
966+
# TODO: remove once we support ms timestamps
967+
self.create_project(teams=[self.team])
968+
queries = [
969+
"duration:1004ms",
970+
"duration:7.3s",
971+
"duration:1.33min",
972+
]
968973

969-
# with self.feature(self.features):
970-
# response = self.client.get(self.url + "?query=xyz:a")
971-
# assert response.status_code == 400
972-
# assert b"xyz" in response.content
974+
with self.feature(self.features):
975+
for query in queries:
976+
response = self.client.get(self.url + f"?query={query}")
977+
assert response.status_code == 400, query
978+
assert (
979+
b"Replays only supports second-resolution timestamps at this time"
980+
in response.content
981+
), query
982+
assert b"duration" in response.content, query
983+
984+
# Note: there's no such thing as a bad field with the tag filtering behavior.
973985

974986
def test_get_replays_unknown_field(self):
975987
"""Test replays unknown fields raise a 400 error."""

0 commit comments

Comments
 (0)