-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(preprod): More filter fields for search #106171
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.
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
Conversation
2a3de4f to
ecd2945
Compare
runningcode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved but we should check AI's comments before merging.
| # Which key we should return any free text under | ||
| free_text_key="text", | ||
| # is:foo filters | ||
| is_filter_translation={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh that's an interesting property! funny how some things aren't expected to be translated and some are.
| self.create_preprod_artifact(app_id="signup.app", commit_comparison=cc2) | ||
| self.create_preprod_artifact(app_id="crash.app", commit_comparison=cc3) | ||
|
|
||
| response = self._request({"query": "!branch:Containsfeature"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what character is the box?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.sentry.io/concepts/search/#:~:text=%23%20Note%3A%20the%20characters%20wrapping%20the%20operators%20are%20%60%5Cuf00d%60.
\uf00d it's an on purpose assigned unicode codepoint from the private use area: https://codepoints.net/U+F00D?lang=en
| data = response.json() | ||
| assert data[0]["app_info"]["app_id"] == "crash.app" | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a test that tests != to check what mr. ai suggested? 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!= is actually not syntax the query string supports. The query looks like:
!foo:bar
and the backend translates that to:
("foo", "!=", "bar")
ecd2945 to
80c9e40
Compare
| return Response( | ||
| {"detail": f"Key {key} does not support tag value lookups."}, | ||
| status=400, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tag value lookup fails for has and installable keys
Medium Severity
The check for synthetic/computed keys that don't support tag value lookups only blocks is and platform, but has and installable are also added to allowed_keys by this PR and should be blocked. has is a filter syntax prefix (not a model field), and installable is a computed annotation. If a user requests tag values for either key, the endpoint will fail because queryset.values("has") references a non-existent field, and queryset.values("installable") is called before annotate_installable() adds the annotation.
No description provided.