[PN-18166] add handler to check mismatch between payload and paperTracking product#240
[PN-18166] add handler to check mismatch between payload and paperTracking product#240DacunzoMa wants to merge 1 commit intofeature/PN-15211from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new handler step that validates product-type consistency between the persisted tracking (PaperTrackings) and the incoming event payload/statusCode mapping, and wires it into the main handler pipelines.
Changes:
- Added
CheckTrackingProducthandler step to validate product-type mismatch and raise a validation exception. - Injected/executed the new step in
AbstractHandlersFactorypipelines and updated concrete factories accordingly. - Added/updated unit tests and extended
ErrorCausewith new entries.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/it/pagopa/pn/papertracker/service/handler_step/generic/CheckTrackingProduct.java | New validation step for product-type consistency |
| src/main/java/it/pagopa/pn/papertracker/service/handler_step/generic/AbstractHandlersFactory.java | Inserts the new step into standard handler chains |
| src/main/java/it/pagopa/pn/papertracker/service/handler_step/AR/HandlersFactoryAr.java | Updates constructor wiring for new dependency |
| src/main/java/it/pagopa/pn/papertracker/service/handler_step/RIR/HandlersFactoryRir.java | Updates constructor wiring for new dependency |
| src/main/java/it/pagopa/pn/papertracker/service/handler_step/RIS/HandlersFactoryRis.java | Updates constructor wiring for new dependency |
| src/main/java/it/pagopa/pn/papertracker/service/handler_step/RS/HandlersFactoryRs.java | Updates constructor wiring for new dependency |
| src/main/java/it/pagopa/pn/papertracker/service/handler_step/_890/HandlersFactory890.java | Updates constructor wiring; some overridden pipelines may still bypass the new step |
| src/main/java/it/pagopa/pn/papertracker/middleware/dao/dynamo/entity/ErrorCause.java | Adds new error causes used by validation flows |
| src/test/java/it/pagopa/pn/papertracker/service/handler_step/generic/CheckTrackingProductTest.java | New unit tests for the validation step |
| src/test/java/it/pagopa/pn/papertracker/service/handler_step/AR/HandlersFactoryArTest.java | Updates handler-chain tests to include the new step |
| src/test/java/it/pagopa/pn/papertracker/service/handler_step/RIR/HandlersFactoryRirTest.java | Updates handler-chain tests with new stubs; some assertions still don’t verify the new step |
| src/test/java/it/pagopa/pn/papertracker/service/handler_step/_890/HandlersFactory890Test.java | Updates factory construction to include the new dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return Mono.empty(); | ||
| } | ||
|
|
||
| String errorMsg = String.format("Product type mismatch for trackingId %s: expected %s, but got %s", context.getTrackingId(), paperTrackingProduct, productType); |
There was a problem hiding this comment.
The error message reports productType (derived from statusCode) as the "got" value, but this step also validates payloadProduct. If the failure is due to payload mismatch (e.g., statusCode maps to ALL), the message will be misleading. Consider including both payload product and statusCode-expected product (or at least use payloadProduct as the actual value).
| String errorMsg = String.format("Product type mismatch for trackingId %s: expected %s, but got %s", context.getTrackingId(), paperTrackingProduct, productType); | |
| String errorMsg = String.format( | |
| "Product type mismatch for trackingId %s: expected tracking product %s and statusCode-derived product %s, but got payload product %s", | |
| context.getTrackingId(), | |
| paperTrackingProduct, | |
| productType, | |
| payloadProduct | |
| ); |
| STOCK_890_REFINEMENT_ERROR("Errore nel perfezionamento della spedizione 890"),; | ||
| STOCK_890_REFINEMENT_ERROR("Errore nel perfezionamento della spedizione 890"), | ||
| VALUE_AFTER_REFINEMENT("Evento arrivato dopo la conclusione della spedizione o mentre si stava aspettando l’OCR (DONE o AWAITING_OCR)"), | ||
| VALUES_NOT_MATCHING("Errore nella validazione degli allegati della sequenza: mancano degli allegati"), |
There was a problem hiding this comment.
VALUES_NOT_MATCHING’s description mentions missing sequence attachments, but it’s now used for product-type mismatch in CheckTrackingProduct. This will surface an inaccurate cause description downstream; update the description (or introduce a more appropriate ErrorCause) so it matches the new validation.
| VALUES_NOT_MATCHING("Errore nella validazione degli allegati della sequenza: mancano degli allegati"), | |
| VALUES_NOT_MATCHING("Errore nella validazione: i valori acquisiti non corrispondono a quelli attesi (es. tipo prodotto)"), |
src/main/java/it/pagopa/pn/papertracker/middleware/dao/dynamo/entity/ErrorCause.java
Show resolved
Hide resolved
|
|
||
| StepVerifier.create(checkTrackingProduct.execute(context)) | ||
| .expectErrorMatches(throwable -> throwable instanceof PnPaperTrackerValidationException && | ||
| throwable.getMessage().contains("Product type mismatch for trackingId null: expected AR, but got ALL")) |
There was a problem hiding this comment.
This assertion expects the error message to say "got ALL", but in this scenario the actual mismatch is between tracking product (AR) and payload product (RIR) while the statusCode productType is ALL. Once the production error message is fixed to report the real mismatching value(s), update this test accordingly.
| throwable.getMessage().contains("Product type mismatch for trackingId null: expected AR, but got ALL")) | |
| throwable.getMessage().contains("Product type mismatch for trackingId null: expected AR, but got RIR")) |
...st/java/it/pagopa/pn/papertracker/service/handler_step/generic/CheckTrackingProductTest.java
Show resolved
Hide resolved
src/test/java/it/pagopa/pn/papertracker/service/handler_step/RIR/HandlersFactoryRirTest.java
Show resolved
Hide resolved
| String paperTrackingProduct = context.getPaperTrackings().getProductType(); | ||
| String payloadProduct = context.getPaperProgressStatusEvent().getProductType(); | ||
| String statusCode = context.getPaperProgressStatusEvent().getStatusCode(); | ||
| String productType = EventStatusCodeEnum.fromKey(statusCode).getProductType().getValue(); |
There was a problem hiding this comment.
EventStatusCodeEnum.fromKey(statusCode) can return null (see fromKey implementation), so calling .getProductType() here can throw a NullPointerException for unknown/unsupported status codes. Handle the null case explicitly (e.g., return a validation error with a clear message/cause) before dereferencing.
| String productType = EventStatusCodeEnum.fromKey(statusCode).getProductType().getValue(); | |
| EventStatusCodeEnum eventStatus = EventStatusCodeEnum.fromKey(statusCode); | |
| if (eventStatus == null) { | |
| String errorMsg = String.format("Unsupported statusCode %s for trackingId %s", statusCode, context.getTrackingId()); | |
| Map<String, Object> additionalDetails = Map.of("statusCode", statusCode, | |
| "statusTimestamp", Objects.toString(context.getPaperProgressStatusEvent().getStatusDateTime(), null) | |
| ); | |
| //TODO: aggiornare dopo il merge del branch 18163 | |
| return Mono.error(new PnPaperTrackerValidationException( | |
| errorMsg, | |
| PaperTrackingsErrorsMapper.buildPaperTrackingsError( | |
| context.getPaperTrackings(), | |
| context.getPaperProgressStatusEvent().getStatusCode(), | |
| ErrorCategory.INCONSISTENT_STATE, | |
| ErrorCause.VALUES_NOT_MATCHING, | |
| errorMsg, | |
| FlowThrow.SEQUENCE_VALIDATION, | |
| ErrorType.ERROR, | |
| context.getEventId() | |
| ))); | |
| } | |
| String productType = eventStatus.getProductType().getValue(); |
src/main/java/it/pagopa/pn/papertracker/service/handler_step/generic/CheckTrackingProduct.java
Show resolved
Hide resolved
| public HandlersFactory890( | ||
| MetadataUpserter metadataUpserter, | ||
| CheckTrackingProduct checkTrackingProduct, | ||
| DeliveryPushSender deliveryPushSender, | ||
| FinalEventBuilder890 finalEventBuilder, |
There was a problem hiding this comment.
HandlersFactory890 overrides some handlers (buildStockIntermediateEventHandler, buildRecag012EventHandler) and those pipelines currently don’t include the new checkTrackingProduct step, so product mismatch validation will be bypassed for those event types. If the check is intended to be global, add checkTrackingProduct to those overridden handler step lists as well.
No description provided.