-
Notifications
You must be signed in to change notification settings - Fork 659
fix(delete): Fix soft delete crash when sketch was soft_deleted #3860
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
base: master
Are you sure you want to change the base?
Changes from all commits
699df7b
6be9822
89fe23d
3cff227
48d2e5d
fd82b96
dd2da18
b417bc9
67cb2d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
|
|
||
| from timesketch_cli_client.commands import attribute as attribute_command | ||
| from timesketch_api_client import search | ||
| from timesketch_api_client.error import NotFoundError | ||
|
|
||
|
|
||
| @click.group("sketch") | ||
|
|
@@ -326,31 +327,72 @@ def delete_sketch(ctx: click.Context, force_delete: bool) -> None: | |
| force_delete: If true, delete immediately. | ||
| """ | ||
| sketch = ctx.obj.sketch | ||
| # if sketch is archived, exit | ||
| if sketch.is_archived(): | ||
|
|
||
| # Initialize with default values. Preserve cached sketch_name if it exists. | ||
| sketch_name = getattr(sketch, "_sketch_name", None) or "<Unknown/Deleted>" | ||
| sketch_desc = "N/A" | ||
| sketch_status = "N/A" | ||
| sketch_labels = "N/A" | ||
| timelines = [] | ||
|
|
||
| try: | ||
| is_archived = sketch.is_archived() | ||
| except NotFoundError as e: # pylint: disable=unused-variable | ||
| click.echo( | ||
| 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") | ||
| ctx.exit(1) | ||
| is_archived = False | ||
|
|
||
| if is_archived: | ||
| click.echo("Error Sketch is archived") | ||
| ctx.exit(1) | ||
|
|
||
| try: | ||
| sketch_name = sketch.name | ||
| sketch_desc = sketch.description | ||
| sketch_status = sketch.status | ||
| sketch_labels = sketch.labels | ||
| timelines = sketch.list_timelines() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm not mistaken, the API (_get_sketch_for_admin) returns That seems a bit risky because the admin won't see what they are actually deleting. Or is this intentional? |
||
| except NotFoundError as e: # pylint: disable=unused-variable | ||
| pass | ||
|
|
||
| # Dryrun: | ||
| if not force_delete: | ||
| click.echo("Would delete the following things (use --force_delete to execute)") | ||
|
|
||
| click.echo( | ||
| f"Sketch: {sketch.id} {sketch.name} {sketch.description} {sketch.status} Labels: {sketch.labels}" # pylint: disable=line-too-long | ||
| f"Sketch: {sketch.id} {sketch_name} {sketch_desc} {sketch_status} Labels: {sketch_labels}" # pylint: disable=line-too-long | ||
| ) | ||
|
|
||
| for timeline in sketch.list_timelines(): | ||
| for timeline in timelines: | ||
| timeline_desc = "N/A" | ||
| timeline_status = "N/A" | ||
| try: | ||
| # timeline.description and timeline.status lazy-load from the API. | ||
| timeline_desc = timeline.description | ||
| timeline_status = timeline.status | ||
| except NotFoundError as e: # pylint: disable=unused-variable | ||
| pass | ||
| click.echo( | ||
| f" Timeline: {timeline.id} {timeline.name} {timeline.description} {timeline.status}" # pylint: disable=line-too-long | ||
| f" Timeline: {timeline.id} {timeline.name} {timeline_desc} {timeline_status}" # pylint: disable=line-too-long | ||
| ) | ||
|
|
||
| if force_delete: | ||
| # --- Check the response for success or error --- | ||
| try: | ||
| sketch.delete(force_delete=force_delete) | ||
| click.echo(f"Sketch {sketch.id} '{sketch.name}' successfully deleted.") | ||
| click.echo(f"Sketch {sketch.id} '{sketch_name}' successfully deleted.") | ||
| except NotFoundError: | ||
|
jaegeral marked this conversation as resolved.
|
||
| click.echo( | ||
| f"Failed to delete sketch {sketch.id} '{sketch_name}'. Error: Sketch was not found (perhaps already permanently deleted?)." # pylint: disable=line-too-long | ||
| ) | ||
| ctx.exit(1) | ||
| except RuntimeError as e: | ||
| click.echo( | ||
| f"Failed to delete sketch {sketch.id} '{sketch.name}'. Error: {e}" | ||
| f"Failed to delete sketch {sketch.id} '{sketch_name}'. Error: {e}" | ||
| ) | ||
| ctx.exit(1) | ||
|
|
||
|
|
||
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.
Isn't this guidance a bit misleading? We only enter this
except NotFoundErrorblock 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_deletewill 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?