fix(delete): Fix soft delete crash when sketch was soft_deleted - #3860
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves an issue where attempting to delete a soft-deleted sketch via the CLI fails. It handles RuntimeError exceptions raised when lazy-loading timeline descriptions and statuses on soft-deleted sketches, defaulting these values to 'N/A'. It also adds an end-to-end integration test to verify this scenario. The review feedback correctly identifies a style guide violation regarding exception handling, noting that the exception should be caught using the 'as e:' syntax (i.e., 'except RuntimeError as e:').
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the handling of soft-deleted or inaccessible sketches in the CLI client by catching NotFoundError and gracefully falling back to default values, alongside adding corresponding end-to-end tests. The review feedback correctly identifies several violations of the repository style guide regarding exception handling, specifically the requirement to use the 'as e:' syntax when catching exceptions, and suggests a refactoring to reduce code duplication.
jaegeral
changed the title
jkppr
left a comment
jkppr
left a comment
There was a problem hiding this comment.
Fwiw, catching NotFoundError in the CLI feels a bit like a band-aid for API limitations. If we instead update the server-side API (e.g., _get_sketch_for_admin) to return basic timeline metadata (just names/IDs from DB, skipping OpenSearch calls) for soft-deleted sketches when accessed by admins, we could avoid these try-catch blocks in the CLI entirely.
WDYT?
| sketch_desc = sketch.description | ||
| sketch_status = sketch.status | ||
| sketch_labels = sketch.labels | ||
| timelines = sketch.list_timelines() |
There was a problem hiding this comment.
If I'm not mistaken, the API (_get_sketch_for_admin) returns timelines: []. This means for admins, the dry-run here will show no timelines, but --force_delete will still permanently delete them from the DB.
That seems a bit risky because the admin won't see what they are actually deleting. Or is this intentional?
There was a problem hiding this comment.
oh yeah it might be good to at least mention the id and status of the timeline, we do not need to expose the name and description for admins.
There was a problem hiding this comment.
DOne ptal, I now return redacted info and I also added another permission check in the get_sketch_for_admin just to be on the safe side
| f"Warning: Sketch {sketch.id} appears to be soft-deleted or inaccessible." | ||
| ) | ||
| if not force_delete: | ||
| click.echo("If you want to permanently delete it, use --force_delete") |
There was a problem hiding this comment.
Isn't this guidance a bit misleading? We only enter this except NotFoundError block if the sketch doesn't exist at all, or if the user is a non-admin (since admins can check archived status on soft-deleted sketches without triggering a 404).
In both cases, telling them to try --force_delete will just lead to another failure (either because they aren't an admin, or because the sketch genuinely doesn't exist). Maybe we should just report it as not found or inaccessible without suggesting --force_delete?
There was a problem hiding this comment.
yep let me fix it
…dacted infos for admins calling list_timelines()
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves error handling and access control when deleting and retrieving sketches, particularly for soft-deleted sketches and admin views. Key changes include refactoring the CLI delete command to gracefully handle NotFoundError, updating the admin sketch retrieval API to redact timeline details if the admin lacks explicit read permissions, and adding corresponding integration and unit tests. The review feedback suggests adhering to PEP 8 inline comment formatting and ensuring robust handling of potential None values when extracting search index names.
jkppr
left a comment
jkppr
left a comment
There was a problem hiding this comment.
2 minor nits, otherwise lgtm
Co-authored-by: Janosch <99879757+jkppr@users.noreply.github.com>
Co-authored-by: Janosch <99879757+jkppr@users.noreply.github.com>
2 participants
This pull request resolves an issue where attempting to delete a soft-deleted sketch via the CLI fails.
It handles exceptions raised when lazy-loading timeline info on soft-deleted sketches, defaulting these values to 'N/A'.
Fwiw, we made the decision back when we implemented deletion, that only admins can see soft-deleted sketches (also in the sketch-list with
include_deleted=true. And currently we do not have a flag forinclude_deletedin the pull single sketch endpoint (which would be only usable by admins).