fix(delete): Fix soft delete crash when sketch was soft_deleted - #3860
fix(delete): Fix soft delete crash when sketch was soft_deleted#3860jaegeral wants to merge 9 commits into
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?
| 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?
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).