feat(deletes): support item attribute conditions in API and consumer#7570
Conversation
…-attribute-conditions-in-consumer
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| # Re-export for backward compatibility | ||
| __all__ = [ | ||
| "ATTRIBUTES_TO_COALESCE", | ||
| "COLUMN_PREFIX", | ||
| "NORMALIZED_COLUMNS_EAP_ITEMS", | ||
| "PROTO_TYPE_TO_ATTRIBUTE_COLUMN", | ||
| "PROTO_TYPE_TO_CLICKHOUSE_TYPE", | ||
| "attribute_key_to_expression", | ||
| "apply_virtual_columns", | ||
| ] |
There was a problem hiding this comment.
I wanted to minimize number of changes in this PR, but I think on a follow-up I'll just totally delete all these re-exports and point anything that depends on them to snuba.protos.common
onewland
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Try to get rid of specialized eap_items bucketing logic in snuba/web/delete_query in #7570 I think this is a good compromise between running a full query pipeline and duplicating bucketing logic in delete_query. It will continue to execute column translations as configuration is changed in the entity/storage YAMLs, increasing decoupling but doesn't bring in a ton of extra scope. Why not run the entire query pipeline? Unfortunately, this would require coupling all deletes to *entity* definitions (these mappers exist on the entity), but we configure and pass around delete queries based on the storage. Tested end-to-end with this flow: https://www.notion.so/sentry/e2e-attribute-testing-2ab8b10e4b5d80cea5cfdc255cff5305
| exp = equals(virtual_column, literal(attr_values[0])) | ||
| else: | ||
| literal_values = [literal(v) for v in attr_values] | ||
| exp = in_cond( | ||
| virtual_column, | ||
| literals_tuple(alias=None, literals=literal_values), | ||
| ) | ||
| and_conditions.append(exp) | ||
|
|
||
| where_clause = combine_and_conditions(and_conditions) | ||
| return _preprocess_for_items(storage, where_clause) |
There was a problem hiding this comment.
Bug: The _construct_condition function calls attribute_key_to_expression without handling the MalformedAttributeException it can raise, leading to an unhandled exception from user-provided data.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The _construct_condition function in snuba/web/delete_query.py calls attribute_key_to_expression with an AttributeKey proto object that comes directly from the client request. The validation in _validate_attribute_conditions is insufficient as it does not check the structure of the AttributeKey proto itself. If a client sends a request with a malformed AttributeKey (e.g., with type set to TYPE_UNSPECIFIED), attribute_key_to_expression will raise a MalformedAttributeException. Since this exception is not caught anywhere in the deletion code path, it will result in an unhandled exception and crash the server process.
💡 Suggested Fix
Wrap the call to attribute_key_to_expression in a try-except block to catch MalformedAttributeException. Convert this exception into a graceful error response to the client, similar to the pattern used in snuba/web/rpc/v1/resolvers/R_eap_items/common/common.py. This prevents the server from crashing on invalid client input.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: snuba/web/delete_query.py#L384-L402
Potential issue: The `_construct_condition` function in `snuba/web/delete_query.py`
calls `attribute_key_to_expression` with an `AttributeKey` proto object that comes
directly from the client request. The validation in `_validate_attribute_conditions` is
insufficient as it does not check the structure of the `AttributeKey` proto itself. If a
client sends a request with a malformed `AttributeKey` (e.g., with `type` set to
`TYPE_UNSPECIFIED`), `attribute_key_to_expression` will raise a
`MalformedAttributeException`. Since this exception is not caught anywhere in the
deletion code path, it will result in an unhandled exception and crash the server
process.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 6901848
There was a problem hiding this comment.
This is in the validation step right? So if it fails it should be fine?
There was a problem hiding this comment.
That's right. EndpointDeleteTraceItems, which is the only entrypoint for EAP deletes, calls into this method in order to return the count of rows that will be deleted. We should maybe wrap it in that endpoint to make it a 400 instead of a 500 but I don't think it particularly matters what the API returns since it'll get logged to Sentry either way.
2 participants
This is a bit of a redo of #7537, to avoid making logic decisions based on the contents of a string which could contain either a column or an subscriptable reference.
Changes:
attribute_key_to_expression(without which, there would be a circular import issue)DeleteQueryMessagehas two new fields:attribute_conditionsandattribute_conditions_item_typepermit_delete_by_attribute, defaulting to offEnd-to-end verification pending, walk-through: https://www.notion.so/sentry/e2e-attribute-testing-2ab8b10e4b5d80cea5cfdc255cff5305