Prevent deletion of event definitions which are still being referenced #14792
Conversation
patrickmann
changed the title
graylog2-server/src/main/java/org/graylog/events/rest/EventDefinitionsResource.java
Outdated
Show resolved
Hide resolved
graylog2-server/src/main/java/org/graylog/events/rest/EventDefinitionsResource.java
Show resolved
Hide resolved
| @@ -23,6 +23,11 @@ public ValidationFailureException(ValidationResult validationResult) { | |||
| this.validationResult = validationResult; | |||
| } | |||
|
|
|||
| public ValidationFailureException(ValidationResult validationResult, String message) { | |||
| super(message); | |||
There was a problem hiding this comment.
Is that message used anywhere?
There was a problem hiding this comment.
We need that message to return an error result from EventDefinitionsResource::delete.
There was a problem hiding this comment.
Unless I'm missing something, it's really not being used, so I think we should rather remove that constructor.
There was a problem hiding this comment.
graylog2-server/src/main/java/org/graylog/events/rest/EventDefinitionsResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code-wise (FE) it looks good!
Testing results
- ❌ When I delete events using the bath delete, the event title is not shown (for both, one or more events)
Monosnap.screencast.2023-03-07.11-46-17.mp4
- ✅ When I delete an event that is used in event correlation via batch delete it is not deleted
- ✅ When I delete an event that is used in event correlation via batch delete an error message is shown
- ✅ When I delete an event that is used in event correlation via delete action it is not deleted
- ✅ When I delete an event that is used in event correlation via delete action an error message is shown
- ✅ When I delete an event that has dependencies on other events it is deleted successfully
- ✅ When I delete an event that has no dependencies and is not used in correlation it is deleted successfully
Monosnap.screencast.2023-03-07.11-51-13.mp4
There is one case that I think could be improved from a user perspective
⚠️ When I delete multiple events (Event 1 and Event 2), where Event 2 is using Event 1 in correlation and Event 1 & Event 2 are not used in any other event, currently the deletion of Event 1 will fail, resulting in an error message and the deletion modal still being open.
I think in this case we can safely delete Event 1 as well, as it is not used anywhere else, this is what I would expect as a user. However, I am not sure how much effort it is to implement this logic.
Monosnap.screencast.2023-03-07.11-58-43.mp4
|
@grotlue re the last test case: That would indeed be better, but would require a fair bit of refactoring in the backend. I'd prefer to leave it as is. |
Here we should show the number of items affected by bulk action. Fixed that |
Graylog2#14792) * add event resolver * minor refactoring * checkpoint * Throw ValidationResultException * Add a message to ValidationFailureException * cl * show dependency error * improve message text * fix linter * show number of item affected by action * review feedback * remove unused constructor in ValidationFailureException --------- Co-authored-by: Ousmane Samba <ousmane@graylog.com> Co-authored-by: Othello Maurer <othello@graylog.com>
Resolves #14302
Check for dependencies when deleting event definitions. Return information about the dependent events, when deletion is not possible.
Introduces a new pluggable EventResolver, since event definitions are pluggable.
Also see related PR Graylog2/graylog-plugin-enterprise#4765