feat: persistent errors for job processing [DHIS2-15276] #15575
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15575 +/- ##
============================================
- Coverage 66.23% 66.19% -0.04%
+ Complexity 31250 31240 -10
============================================
Files 3485 3485
Lines 129791 129851 +60
Branches 15145 15165 +20
============================================
- Hits 85965 85960 -5
- Misses 36741 36802 +61
- Partials 7085 7089 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
…UID [DHIS2-15276]
|
Kudos, SonarCloud Quality Gate passed! |
| } | ||
|
|
||
| default void addError(ErrorCode code, String uid, String type, Integer index, List<String> args) { | ||
| // default implementation is a NOOP, we don't remember or handle the error |
There was a problem hiding this comment.
I don't follow this reasoning for this empty method.
Do only certain jobs care about errors & they would then override this method? (which would result in the persisting of the error(s)?
And if jobs using this default method have errors, they are silently swallowed?
There was a problem hiding this comment.
This is not related to certain jobs but only one implementation of the JobProgress interfaces wants to record them and here we have no access to the implementation. To avoid having to implement the method in all other implementation with an empty method it is instead implemented like that as default.
There was a problem hiding this comment.
For the default to be an empty method suggests it shouldn't be part of the interface. It feels like we're adding this confusing behaviour to satisfy only 1 implementer.
Could a javadoc comment be added to the method at least to explain why we don't want to handle errors here (handling errors seems like a very common use case) as the default and when would overriding be necessary. This would at least help someone reading the code to hopefully understand the reasoning for it & when it should be overridden.
There was a problem hiding this comment.
I'll do that in the next PR I was planning on doing anyhow
Summary
Main changes to extend the job progress tracking API with a error collection feature.
Errors are attached to the overall
progressJSON object.A new column
errorCodeswas added to the table which contains the codes extracted from theprogressobject.This is purely to allow easier search by error code(s) using existing filter API tech. The column is implicitly maintained when the progress JSON changes.
To accommodate this the persisted JSON changed from an array of processes which previously would be wrapped in a
Progressobject on in the memory model which had thesequenceofProcesses this root object is now explicit in the stored JSON so it can also contain theerrorslist. To be backwards compatibly with the JSON array stored previously the extraction pf the progress JSON will auto-wrap any array values in an appropiate object in the SQL so existing data will continue to work.Other changes
ErrorReportof the metadata import now does contain the plain arguments to the message so they can be forwarded to the job progress error listErrorclass is now calledTrackerImportErrorin OpenAPI to resolve the name clash with the new genericErrorclass addedALLorF_PERFORM_MAINTENANCEauthority or must be performed by the user executing the job/progressand/errorsis restricted toALLorF_PERFORM_MAINTENANCEauthorityFixes
progressJSON object is now stored as ajsonbobject not asstringof the escaped JSON (that worked by accident for store and decode)Automatic Testing
The affected code is covered by many existing test indirectly. No explicit new tests were added yet.
Manual Testing
/api/jobConfigurations/{uid}/errorsand/api/jobConfigurations/{uid}/progressendpoints of the job created for the import and check the error added to the file is recorded (find the UID e.g. by the newestMETADATA_IMPORTjob in the listapi/jobConfigurations/gist?order=created:desc)/errorshave a readablemessageproperty