Sketch delete #3261
Sketch delete #3261
Conversation
jaegeral
added
API client
Rest API
|
/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 introduces a proof-of-concept for deleting sketches, which is a valuable addition. However, there are several critical issues that need to be addressed before this can be merged. The main API endpoint for deletion currently aborts all requests, and the tsctl implementation seems to be using an incorrect approach for deletion. There's also a broken CLI command and some unclear code in the API client.
I've focused on correctness and maintainability in this review, referencing the provided style guide where applicable (e.g., for docstrings and string formatting).
Summary of Findings
- API Deletion Blocked: The primary API endpoint for deleting sketches (
timesketch/api/v1/resources/sketch.py) contains anabort()call that prevents any deletion from occurring, returning a 400 Bad Request instead. This needs to be resolved for the feature to work. tsctl sketch-deleteImplementation: The newtsctl sketch-deletecommand intimesketch/tsctl.pyattempts to delete a sketch by directly interacting with database ORM models and calls a non-existentsketch.delete()method. This approach will not correctly delete sketches (especially OpenSearch data) and should likely use the Timesketch API client instead.- Unclear
z_deletemethod in API Client: Theapi_client/python/timesketch_api_client/sketch.pyfile introduces az_deletemethod alongside modifications to the existingdeletemethod. The purpose and differences (especially in handlingforce_deletevia URL parameters vs. request body) are unclear and could lead to confusion or incorrect API calls. - Broken
add_labelCLI Command: The Click decorators for theadd_labelcommand incli_client/python/timesketch_cli_client/commands/sketch.pyhave been removed, which will make the command non-functional. - Pending Tests and Documentation: The PR description indicates that unit tests, e2e tests, and documentation updates are still pending. These are crucial for ensuring the quality and usability of this new feature.
Merge Readiness
This pull request is a good start for the sketch deletion feature, but it's currently in a DRAFT state and has several critical issues that prevent the core functionality from working. The API endpoint for deletion is blocked, and the tsctl command for deletion is not correctly implemented. Additionally, an existing CLI command (add_label) appears to be broken by changes in this PR.
I recommend addressing these critical and high-severity issues before this PR can be considered for merging. Specifically, the abort() call in the API needs to be removed/fixed, tsctl should use the API client for deletion, the z_delete method in the API client needs clarification, and the add_label command needs its decorators restored. Completing the pending tests and documentation will also be essential.
As an AI assistant, I am not authorized to approve pull requests. Please ensure further review and approval from other team members after the identified issues are resolved.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces sketch deletion functionality (soft and hard deletes) across the API, Python client, and CLI tools. The API changes for hard deletion are quite thorough. Key areas for attention include the tsctl hard delete logic, a test assertion for deleting archived sketches, and minor improvements to a CLI docstring and an e2e test's completeness. The use of status codes for certain error conditions in the API could also be reviewed for semantic accuracy.
…st (here the sketch)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust sketch deletion feature, complete with soft and hard delete options, API client updates, and new CLI commands. The changes are well-structured and include comprehensive tests.
I've identified a critical logic bug in the API client's retry mechanism that could cause performance issues, along with some opportunities to improve the robustness and consistency of the hard-delete logic on the backend. My feedback also includes suggestions to simplify some of the new code for better maintainability.
Overall, this is a solid contribution. Addressing these points will make the new feature even more reliable.
| # now the real deletion | ||
| for timeline in sketch.timelines: | ||
| timeline.set_status(status="deleted") | ||
| searchindex = timeline.searchindex | ||
| # remove the opensearch index | ||
| index_name_to_delete = searchindex.index_name | ||
|
|
||
| try: | ||
| # Attempt to delete the OpenSearch index | ||
| self.datastore.client.indices.delete(index=index_name_to_delete) | ||
| logger.debug( | ||
| "User: %s is going to delete OS index %s", | ||
| current_user, | ||
| index_name_to_delete, | ||
| ) | ||
|
|
||
| # Check if the index is really deleted | ||
| if self.datastore.client.indices.exists(index=index_name_to_delete): | ||
| e_msg = ( | ||
| f"Failed to delete OpenSearch index " | ||
| f"{index_name_to_delete}. Please check logs." | ||
| ) | ||
| logger.error(e_msg) | ||
| abort(HTTP_STATUS_CODE_INTERNAL_SERVER_ERROR, e_msg) | ||
| else: | ||
| logger.debug( | ||
| "OpenSearch index %s successfully deleted.", | ||
| index_name_to_delete, | ||
| ) | ||
|
|
||
| except NotFoundError: | ||
| # This can happen if the index was already deleted or never existed. | ||
| e_msg = ( | ||
| f"OpenSearch index {index_name_to_delete} was not found " | ||
| f"during deletion attempt. It might have been deleted " | ||
| f"already." | ||
| ) | ||
| logger.warning(e_msg) | ||
| except ConnectionError as e: | ||
| e_msg = ( | ||
| f"Connection error while trying to delete OpenSearch index " | ||
| f"{index_name_to_delete}:\n" | ||
| f"{e}" | ||
| ) | ||
| logger.error(e_msg) | ||
| abort( | ||
| HTTP_STATUS_CODE_INTERNAL_SERVER_ERROR, | ||
| e_msg, | ||
| ) | ||
| except Exception as e: # pylint: disable=broad-except | ||
| # Catch any other unexpected errors during deletion | ||
| e_msg = ( | ||
| f"An unexpected error occurred while deleting " | ||
| f"OpenSearch index {index_name_to_delete}: {e}" | ||
| ) | ||
| logger.error(e_msg) | ||
| abort( | ||
| HTTP_STATUS_CODE_INTERNAL_SERVER_ERROR, | ||
| e_msg, | ||
| ) | ||
|
|
||
| db_session.delete(searchindex) | ||
| db_session.delete(timeline) | ||
|
|
||
| db_session.delete(sketch) | ||
| db_session.commit() |
There was a problem hiding this comment.
The hard delete logic appears to be incomplete. The PR description mentions that related objects like views and stories will be explicitly deleted, but the current implementation only explicitly deletes timelines and searchindices from the database session. While SQLAlchemy's cascade settings on the Sketch model might handle the deletion of other related objects (views, stories, aggregations, etc.), relying on it while also performing explicit deletions for timelines is inconsistent. This could lead to orphaned objects if any relationship is missing the correct cascade configuration.
To ensure a complete and robust hard delete, I recommend either:
- Explicitly delete all related objects (views, stories, aggregations, etc.) in loops, similar to how timelines are handled. This matches the intent described in the PR.
- Rely entirely on cascading deletes. Remove the explicit
db_session.delete(timeline)anddb_session.delete(searchindex)calls and ensure all relationships on theSketchmodel have the correctcascadeoptions set. The loop would then only be needed for deleting the OpenSearch indices.
| if not error.check_return_status(response, logger): | ||
| error.error_message( | ||
| response, | ||
| message=f"Failed to delete sketch {self.id}", | ||
| error=RuntimeError, | ||
| ) | ||
| else: | ||
| return error.check_return_status(response, logger) | ||
| return True |
There was a problem hiding this comment.
The logic here is a bit complex and contains an unreachable statement. If error.check_return_status() returns True, the else block is executed, which calls check_return_status again and returns its result. This makes the final return True on line 509 unreachable. You can simplify this block for better readability and to remove the dead code.
if not error.check_return_status(response, logger):
error.error_message(
response,
message=f"Failed to delete sketch {self.id}",
error=RuntimeError,
)
return True| @sketch_group.command("delete", help="Delete a sketch, default will only do a dry-run") | ||
| @click.option( | ||
| "--force_delete", | ||
| required=False, | ||
| is_flag=True, | ||
| help="Only execute the deletion if this is set.", | ||
| ) | ||
| @click.pass_context | ||
| def delete_sketch(ctx: click.Context, force_delete: bool) -> None: | ||
| """Delete a sketch. | ||
|
|
||
| By default, a sketch will not be deleted. To execute the deletion provide the | ||
| flag --force_delete. | ||
|
|
||
| Args: | ||
| ctx (click.Context): The Click context object, containing the sketch. | ||
| force_delete (bool): If true, delete immediately. | ||
| """ | ||
| sketch = ctx.obj.sketch | ||
| # if sketch is archived, exit | ||
| if sketch.is_archived(): | ||
| click.echo("Error Sketch is archived") | ||
| ctx.exit(1) | ||
|
|
||
| # 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 | ||
| ) | ||
|
|
||
| for timeline in sketch.list_timelines(): | ||
| click.echo( | ||
| f" Timeline: {timeline.id} {timeline.name} {timeline.description} {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.") | ||
| except RuntimeError as e: | ||
| click.echo( | ||
| f"Failed to delete sketch {sketch.id} '{sketch.name}'. Error: {e}" | ||
| ) | ||
| return |
There was a problem hiding this comment.
The new sketch-delete command currently supports a dry-run (default) and a hard-delete (with --force_delete). However, it doesn't provide a way to perform a soft-delete, which is the default behavior of the underlying API (sketch.delete(force_delete=False)). This creates an inconsistency between the API's capabilities and the CLI's interface. It would be beneficial to allow users to perform soft deletes from the CLI as well.
| # Check if the index is really deleted | ||
| if self.datastore.client.indices.exists(index=index_name_to_delete): | ||
| e_msg = ( | ||
| f"Failed to delete OpenSearch index " | ||
| f"{index_name_to_delete}. Please check logs." | ||
| ) | ||
| logger.error(e_msg) | ||
| abort(HTTP_STATUS_CODE_INTERNAL_SERVER_ERROR, e_msg) |
There was a problem hiding this comment.
Checking for the existence of an index immediately after deletion can be unreliable due to the asynchronous nature of OpenSearch's delete operations. This check could lead to a race condition where the index still appears to exist for a short time after the delete call has succeeded, causing the function to incorrectly abort with a 500 error. It's generally safe to trust that the delete operation was successful if it didn't raise an exception. I recommend removing this check to improve the robustness of the hard-delete operation.
# The index deletion is asynchronous, so checking for existence
# immediately after can be racy. It's better to trust the delete
# call if it doesn't raise an exception.
logger.debug(
"OpenSearch index %s successfully deleted.",
index_name_to_delete,
)| except Exception as e: # pylint: disable=broad-except | ||
| # Catch any other unexpected errors during deletion | ||
| e_msg = ( | ||
| f"An unexpected error occurred while deleting " | ||
| f"OpenSearch index {index_name_to_delete}: {e}" | ||
| ) | ||
| logger.error(e_msg) | ||
| abort( | ||
| HTTP_STATUS_CODE_INTERNAL_SERVER_ERROR, | ||
| e_msg, | ||
| ) |
There was a problem hiding this comment.
Using a broad except Exception can mask underlying issues and make debugging harder. For instance, a TypeError due to a programming error would be caught here and returned as a generic 500 error. It would be better to catch a more specific exception. The opensearch-py client's exceptions inherit from opensearchpy.OpenSearchException. Catching that would be safer and more specific, while still handling a wide range of potential datastore errors.
except opensearchpy.OpenSearchException as e:
# Catch any other unexpected errors during deletion
e_msg = (
f"An unexpected error occurred while deleting "
f"OpenSearch index {index_name_to_delete}: {e}"
)
logger.error(e_msg)
abort(
HTTP_STATUS_CODE_INTERNAL_SERVER_ERROR,
e_msg,
)
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This PR introduces the capability to delete sketches in Timesketch, both via the API and the command-line clients (
tsctland the Python API client). The implementation includes support for both a 'soft' delete (marking the sketch as deleted in the database) and a 'hard' delete (permanently removing the sketch, its associated data, and the underlying OpenSearch indices). The changes span the API endpoint, the Python API client library, and thetsctlcommand-line tool, along with updates to tests.Highlights
force_deleteflag/parameter) that permanently removes the sketch, related database objects, and OpenSearch indices./api/v1/sketches/<sketch_id>/) to accept aforce_deleteparameter (via body or URL query) to control the deletion mode. It also adds checks to prevent deletion of archived sketches or those with specific protective labels (like 'lithold').deletemethod in theSketchclass to accept theforce_deleteboolean parameter and pass it to the API endpoint.sketch-deletecommand to both the Python CLI client andtsctl. These commands provide a command-line interface for deleting sketches, including a--force-deleteflag for hard deletion and a default dry-run mode.Changelog
Click here to see the changelog
deletemethod to accept aforce_deleteboolean parameter.?force=trueto the API request URL ifforce_deleteis True.deletemethod to explain theforce_deleteparameter.sketch-deleteunder thesketch_group.--force_deleteflag for thesketch-deletecommand.--force-deleteis not used.deletemethod inSketchResourceto acceptsketch_idand an optionalforce_deleteboolean parameter (defaulting to False).force_deleteparameter (via argument or URL query), permissions, and potential error codes.test_attempt_to_delete_archived_sketchtest.HTTP_STATUS_CODE_BAD_REQUEST).sketch-delete.--force-deleteflag for thesketch-deletecommand.--force-deleteis not used.force-deleteis true.Open:
test_delete_sketchwhen trying to verify the sketch is gone