Skip to content

ref(search): Add EAP API attribute visibility checks#116091

Open
nsdeschenes wants to merge 6 commits into
masterfrom
nd/feat-attributes-visibility-core
Open

ref(search): Add EAP API attribute visibility checks#116091
nsdeschenes wants to merge 6 commits into
masterfrom
nd/feat-attributes-visibility-core

Conversation

@nsdeschenes
Copy link
Copy Markdown
Contributor

@nsdeschenes nsdeschenes commented May 22, 2026

The goal of this PR is to lie down the foundational work of removing/hiding internal attributes from non-staff/non-super-user requests.

To determine if an attribute should be hidden or not, we pull that information from the Sentry conventions package from the visibility field from attribute metadata.

These changes are limited to the eap search resolver, as such this PR does not doing any enforcement of these attributes, that will come in follow up PRs. As well, this PR adds in some tests.

Add a shared helper for hiding internal Sentry convention attributes from API surfaces and let SearchResolver track attributes hidden by API visibility configuration.

This keeps default resolver behavior unchanged unless an API caller opts into visibility enforcement.
The `as ATTRIBUTE_METADATA` pattern tells mypy this is an intentional
re-export, fixing the attr-defined errors in tests that access it via
`eap_utils.ATTRIBUTE_METADATA`.
@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 22, 2026

EXP-966

@nsdeschenes nsdeschenes marked this pull request as ready for review May 22, 2026 14:17
@nsdeschenes nsdeschenes requested review from a team as code owners May 22, 2026 14:17

if column_definition:
if self.config.api_attribute_visibility_item_type is not None:
from sentry.search.eap.utils import can_expose_attribute_to_api
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any specific reason why we import this here and not at the top? Unless it otherwise leads to a circular import, I think we should move it to the top

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i tried this out, seems to have broken many things 😬

Comment thread src/sentry/search/eap/utils.py
Copy link
Copy Markdown
Contributor

@adrianviquez adrianviquez left a comment

Choose a reason for hiding this comment

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

Couple of comments, lmk what you think and can take a 2nd look!

The candidates set already prevents duplicates since we check
`replacement not in candidates` before adding to pending.
@nsdeschenes nsdeschenes requested a review from adrianviquez May 22, 2026 16:18
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 1c0058e. Configure here.

Comment thread src/sentry/search/eap/resolver.py
Reverts the import move to avoid a circular import risk between
resolver and utils modules.
When public_alias_override is set (e.g. equation resolution), the
column_definition.public_alias becomes a synthetic label like
"equation|…" which can_expose_attribute_to_api cannot evaluate
against conventions, prefixes, or mappings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants