Clearer error messages when editing actions #7359
Conversation
| @@ -17,6 +21,9 @@ def wrap_query_error(err: Exception) -> Exception: | |||
|
|
|||
| # :TRICKY: Return a custom class for every code by looking up the short name and creating a class dynamically. | |||
| if hasattr(err, "code"): | |||
| if err.code == 427: | |||
There was a problem hiding this comment.
Where does this 427 come from? Maybe add a comment on how CH handles this and why we raise it this way?
There was a problem hiding this comment.
So, this is the error coming from CH, whose name is CHQueryErrorCannotCompileRegexp , which occurs when the regex being passed in is invalid.
| @@ -5,6 +5,10 @@ | |||
| from posthog.exceptions import EstimatedQueryExecutionTimeTooLong | |||
|
|
|||
|
|
|||
| class CHQueryErrorCannotCompileRegexp(ServerException): | |||
There was a problem hiding this comment.
We seem to be using this for multiple cases, not sure if the name reflects the right use for this. I would also suggest adding a useful default code & message
There was a problem hiding this comment.
This is basically for capturing the error CH sends to us. So, using the same name as the CH error code, to prevent confusion.
The error message we raise is ValidationError
There was a problem hiding this comment.
There's always a code and message generated from CH, so we shouldn't need to supply even a default one ourselves.
| "SELECT count(1) FROM events WHERE team_id = %(team_id)s AND {}".format(query), | ||
| {"team_id": action.team_id, **params}, | ||
| ) | ||
| except CHQueryErrorCannotCompileRegexp: |
There was a problem hiding this comment.
Related to comment above, but why do we raise a Regexp compile error here? Is the only error that happen here related to a regex error?
There was a problem hiding this comment.
There can be several errors when querying CH (see the file above where I define this for the rest of them - all 500 of them), and this on is specific to the user input being provided for selectors / regexes.
There was a problem hiding this comment.
Is the only error that happen here related to a regex error?
Yes
|
Not a fan that this "swallows" legit errors and these won't reach sentry. This in turn means we won't ever discover or fix issues relating to bad selector complication (most of these are not user errors). If we wanted to do validation, this would be better handled in the action creation api as validation instead. Or better yet, in the frontend similar to #4680 |
| query_result = self._query_events_list(filter, team, request, limit=limit) | ||
| except CHQueryErrorCannotCompileRegexp: | ||
| raise ValidationError( | ||
| "Invalid filter for events. Check that your selectors and regexes are defined properly." |
There was a problem hiding this comment.
This is misleading - we fail to compile legit selectors on our end. This is not an user error.
|
Nice, that route is much better, closing this. |
Changes
Return better error messages when creating actions with invalid selectors.
This creates 2 toasts: one for events querying, another for action counts. It's not deterministic which toast "wins", hence putting the error message in both place.
Unsure if there's a better way to handle this 👀
Before:
After:
How did you test this code?
Run locally