Added new elements to delete event #392
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -285,13 +285,7 @@ def delete_by_id(host_id_list): | |||||
|
|
||||||
| hosts_to_delete = [] | ||||||
| for host in query.all(): | ||||||
| hosts_to_delete.append(host) | ||||||
|
There was a problem hiding this comment.
I guess we need to figure out how to eagerly load the host here, essentially doing manually what happens on the attribute access. I would not eagerly load all of them at once, just do something like object identity check here. There was a problem hiding this comment. If we’re not going to eagerly load in this PR, please abbreviate this to
Suggested change
|
||||||
|
|
||||||
| if not hosts_to_delete: | ||||||
| return flask.abort(status.HTTP_404_NOT_FOUND) | ||||||
|
|
@@ -305,7 +299,13 @@ def delete_by_id(host_id_list): | |||||
| logger.debug("Deleted hosts: %s", hosts_to_delete) | ||||||
|
There was a problem hiding this comment. This works only thanks to the fact that logger has some own exception handling mechanism. This logging call actually calls _Host._repr for every host, even for those that are already deleted. This function internally calls Host.id that triggers the lazy loading and bang, an uncaught exception occurs. Hopefully, this is internally caught by the logger output to stderr and the execution continues normally. This is probably for proper logging calls not to cause unexpected termination. But if logger’s interpolation weren’t used ( So, sorry, @dehort. I understand that you don’t want to have this pending any longer, but we’re really on a thin ice here. The logging doesn’t crash just because it behaves unexpectedly wise, and catching the error there when emitting an event, that’s just randomly putting a try-expect on a first side-effect that triggers the lazy loading. If you really need to have this merged in the current state. Please, at least, let @aleccohan to fix this in a subsequent PR, having that properly in a Sprint. This is not something we want in our codebase. It can go well with writing automatic test for this. |
||||||
|
|
||||||
| for deleted_host in hosts_to_delete: | ||||||
| try: | ||||||
|
There was a problem hiding this comment. This change actually modifies the behavior. Before the change, if a host has been already deleted, it is not added to the hosts_to_delete list. That caused aborting with 404 if no hosts were actually deleted, and also those not actually deleted hosts not to be logged. Now, all hosts to delete are logged even though they have been already deleted and raise the ObjectDeletedError here. There was a problem hiding this comment. This might change the behavior a bit but i'm not sure how big of a change it really is. With this change, the only difference I see is that we might not send back a 404 in the case where all the hosts that were requested to be deleted, were already deleted. (This situation is likely pretty rare anyhow.) I suspect with the try/catch added around the emit_event(), then we shouldn't produce a delete event in the case where a host was deleted already (that would be the correct behavior in my opinion). In either case (200 return code vs 404 return code), the host would be deleted and on event would be logged. I'm not sure that's a big difference. |
||||||
| emit_event(events.delete(deleted_host)) | ||||||
| except sqlalchemy.orm.exc.ObjectDeletedError: | ||||||
| logger.exception( | ||||||
| "Encountered sqlalchemy.orm.exc.ObjectDeletedError exception during delete_by_id operation. Host was " | ||||||
| "already deleted." | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| @api_operation | ||||||
|
|
||||||
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.
Better! 😀