Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clearer error messages when editing actions #7359

Closed
wants to merge 4 commits into from
Closed

Clearer error messages when editing actions #7359

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5822ff6
dump
neilkakkar Nov 25, 2021
7ba9f5d
cleanup
neilkakkar Nov 25, 2021
cfe42ef
address comments
neilkakkar Nov 26, 2021
c3e4814
add comment for error
neilkakkar Nov 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ee/clickhouse/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
from posthog.exceptions import EstimatedQueryExecutionTimeTooLong


class CHQueryErrorCannotCompileRegexp(ServerException):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's always a code and message generated from CH, so we shouldn't need to supply even a default one ourselves.

pass


def wrap_query_error(err: Exception) -> Exception:
"Beautifies clickhouse client errors, using custom error classes for every code"
if not isinstance(err, ServerException):
Expand All @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this 427 come from? Maybe add a comment on how CH handles this and why we raise it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is the error coming from CH, whose name is CHQueryErrorCannotCompileRegexp , which occurs when the regex being passed in is invalid.

return CHQueryErrorCannotCompileRegexp(err.message, code=err.code)

name = CLICKHOUSE_ERROR_CODE_LOOKUP.get(err.code, "UNKNOWN")
name = f"CHQueryError{name.replace('_', ' ').title().replace(' ', '')}"
return type(name, (ServerException,), {})(err.message, code=err.code)
Expand Down
35 changes: 34 additions & 1 deletion ee/clickhouse/models/test/test_property.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def test_prop_element(self):
href="/a-url",
attr_class=["small"],
text='bla"bla',
attributes={},
attributes={"data-graph-container": "valid"},
nth_child=1,
nth_of_type=0,
),
Expand All @@ -122,6 +122,14 @@ def test_prop_element(self):
Element(tag_name="button", attr_class=["btn", "btn-tertiary"], nth_child=0, nth_of_type=0),
],
)
# _create_event(
# event="$autocapture",
# team=self.team,
# distinct_id="whatever",
# elements=[
# Element(tag_name="custom", attributes={"data-custom-one": "valid", "data-custom-two": "meh"}, nth_child=0, nth_of_type=0,),
# ],
# )

# selector

Expand Down Expand Up @@ -149,6 +157,31 @@ def test_prop_element(self):
)
self.assertEqual(len(self._run_query(filter)), 1)

filter = Filter(
data={
"properties": [
{
"key": "selector",
"value": "[data-graph-container='valid']",
"operator": "exact",
"type": "element",
}
]
}
)
with self.settings(SHELL_PLUS_PRINT_SQL=True):
self.assertEqual(len(self._run_query(filter)), 1)

filter = Filter(
data={
"properties": [
{"key": "selector", "value": "['data-graph-container']", "operator": "exact", "type": "element"}
]
}
)
with self.settings(SHELL_PLUS_PRINT_SQL=True):
self.assertEqual(len(self._run_query(filter)), 1)

filter = Filter(
data={
"properties": [
Expand Down
13 changes: 9 additions & 4 deletions ee/clickhouse/views/actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@

from rest_framework import serializers
from rest_framework.decorators import action
from rest_framework.exceptions import ValidationError
from rest_framework.request import Request
from rest_framework.response import Response
from rest_framework_csv import renderers as csvrenderers

from ee.clickhouse.client import sync_execute
from ee.clickhouse.errors import CHQueryErrorCannotCompileRegexp
from ee.clickhouse.models.action import format_action_filter
from ee.clickhouse.queries.trends.person import TrendsPersonQuery
from ee.clickhouse.sql.person import INSERT_COHORT_ALL_PEOPLE_THROUGH_PERSON_ID, PERSON_STATIC_COHORT_TABLE
Expand Down Expand Up @@ -90,10 +92,13 @@ def count(self, request: Request, **kwargs) -> Response: # type: ignore
if query == "":
return Response({"count": 0})

results = sync_execute(
"SELECT count(1) FROM events WHERE team_id = %(team_id)s AND {}".format(query),
{"team_id": action.team_id, **params},
)
try:
results = sync_execute(
"SELECT count(1) FROM events WHERE team_id = %(team_id)s AND {}".format(query),
{"team_id": action.team_id, **params},
)
except CHQueryErrorCannotCompileRegexp:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the only error that happen here related to a regex error?

Yes

raise ValidationError("Invalid filters for action count")
return Response({"count": results[0][0]})


Expand Down
8 changes: 6 additions & 2 deletions ee/clickhouse/views/events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@

from django.utils.timezone import now
from rest_framework.decorators import action
from rest_framework.exceptions import NotFound
from rest_framework.exceptions import NotFound, ValidationError
from rest_framework.request import Request
from rest_framework.response import Response

from ee.clickhouse.client import sync_execute
from ee.clickhouse.errors import CHQueryErrorCannotCompileRegexp
from ee.clickhouse.models.action import format_action_filter
from ee.clickhouse.models.event import ClickhouseEventSerializer, determine_event_conditions
from ee.clickhouse.models.person import get_persons_by_distinct_ids
Expand Down Expand Up @@ -100,7 +101,10 @@ def list(self, request: Request, *args: Any, **kwargs: Any) -> Response:
team = self.team
filter = Filter(request=request, team=self.team)

query_result = self._query_events_list(filter, team, request, limit=limit)
try:
query_result = self._query_events_list(filter, team, request, limit=limit)
except CHQueryErrorCannotCompileRegexp:
raise ValidationError("Invalid filter for events")

# Retry the query without the 1 day optimization
if len(query_result) < limit and not request.GET.get("after"):
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/lib/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,9 @@ const api = {
const startTime = new Date().getTime()
try {
response = await fetch(url, { signal })
console.log(response)
} catch (e) {
console.log(e)
throw { status: 0, message: e }
}

Expand Down
3 changes: 2 additions & 1 deletion frontend/src/scenes/events/eventsTableLogic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,8 @@ export const eventsTableLogic = kea<eventsTableLogicType<ApiError, EventsTableLo
try {
apiResponse = await api.get(`api/projects/${values.currentTeamId}/events/?${urlParams}`)
} catch (error) {
actions.fetchOrPollFailure(error)
console.log('error in events: ', error)
actions.fetchOrPollFailure({ statusText: error.detail, ...error })
return
}

Expand Down
2 changes: 1 addition & 1 deletion posthog/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,7 @@ def show_toolbar(request):
)

if SKIP_SERVICE_VERSION_REQUIREMENTS:
print_warning("Skipping service version requirements. This is dangerous and PostHog might not work as expected!")
print_warning(["Skipping service version requirements. This is dangerous and PostHog might not work as expected!"])

SERVICE_VERSION_REQUIREMENTS = [
ServiceVersionRequirement(service="postgresql", supported_version=">=11.0.0,<=14.1.0",),
Expand Down