[MODINVOICE-478] Prevent conflicts when saving multiple invoice lines with data import #413
Conversation
… with data import
SerhiiNosko
requested review from
Aliaksandr-Fedasiuk,
KaterynaSenchenko and
okolawole-ebsco
|
Kudos, SonarCloud Quality Gate passed! |
| .map(invoiceLine -> persistInvoiceLine(invoiceLine, requestContext)) | ||
| .collect(Collectors.toList()); | ||
| return GenericCompositeFuture.join(futures).mapEmpty(); | ||
| public Future<List<InvoiceLine>> createInvoiceLines(List<InvoiceLine> invoiceLines, RequestContext requestContext) { |
There was a problem hiding this comment.
Hello @damien-git, could you say please can we return invoiceLines creation result (something like InvoiceHelper.createInvoiceAndLines() method) with errors for particular invoice lines in case POST requests fail for some of them? So that we could build such errors for those invoice lines in the CreateInvoiceEventHandler (by INVOICE_LINES_ERRORS_KEY). It will allow to show error only for invoice lines that could not be created, but not for lines that were created.
There was a problem hiding this comment.
I think an error at this point (when InvoiceLineService.createInvoiceLines() is called) means that there is an error for all lines, because other related operations for all lines are not done yet. For instance, the invoice-order relations, the invoice po numbers and the invoice update. Even if an invoice line record is created, the whole operation to create the invoice line (which includes updating the invoice) is not finished. I don't think there is a way around, although I guess we could add code that deletes created lines (if any) when the code fails after the start of the line record creations. Note that this would be an internal error, so it should not happen in normal circumstances.
Purpose
MODINVOICE-478 - Data import saving invoice lines in parallel can cause conflicts when invoices are updated
Approach
New implementation to save invoice and invoice lines with data import.
There will no longer be potential conflicts when the invoice is updated, which happens several times in the process.
Line creations are processed together, which should make the whole operation significantly faster.
Error reporting is a little different: some (internal) errors that were previously reported with a line number will no longer be associated with a number. Other errors (for validation) will be better reported and will prevent the creation of the invoice and lines.
TODOS and Open Questions
TODO: Integration tests, especially
data-import/src/main/resources/folijet/data-import/features/import-edi-invoice.feature. I was not able to test it because I could not get Kafka messages to be sent to IDEA from mod-data-import.Also, we should check that error reporting still matches user expectations.
Learning
I learned a way to validate objects with their Json schema. We should do that every time we don't get the objects directly with the API (such as when using Kafka).
Also I found a way to implement a helper to use semaphores without repeating that ugly pattern everywhere.
Pre-Merge Checklist:
Before merging this PR, please go through the following list and take appropriate action.
If there are breaking changes, please STOP and consider the following:
Ideally, all the PRs involved in breaking changes would be merged on the same day
to avoid breaking the folio-testing environment.
Communication is paramount if that is to be achieved,
especially as the number of inter-module and inter-team dependencies increase.
While it's helpful for reviewers to help identify potential problems,
ensuring that it's safe to merge is ultimately the responsibility of the PR assignee.