adamsaghy opened a new pull request, #6786:
URL: https://github.com/apache/fineract/pull/6786
## Description
Follow-up to #6774, where `PaymentTypeIntegrationTest` failed with a 404 on
`PUT /paymenttypes/{id}`. The generated Feign model sent `"isSystemDefined":
null`, the server wrote NULL into `m_payment_type.is_system_defined`,
PostgreSQL rejected it with a NOT NULL violation, and the service turned that
into `PaymentTypeNotFoundException`.
### 1. Constraint violations on update were reported as 404
`PaymentTypeWriteServiceImpl.updatePaymentType` caught `JpaSystemException |
DataIntegrityViolationException` and threw `PaymentTypeNotFoundException`. It
now uses `ErrorHandler.getMappable(e,
"error.msg.paymenttypes.unknown.data.integrity.issue", ...)`, so the client
gets a data integrity error (HTTP 403). This is the same code as the delete
path's fallback and the same pattern as the documents reference implementation.
A payment type that really doesn't exist still gets a 404 from
`findById(...).orElseThrow(...)`.
### 2. `isSystemDefined: null` on update: treated as "leave unchanged"
**Why `@NotNull` was not enforced:** the `@Valid` on the service method
parameters has no effect.
- Spring method validation only proxies beans marked `@Validated`, and
nothing in the codebase uses it.
- Jersey bean validation only applies `@Valid` on JAX-RS resource method
parameters, as currencies and business date do. The payment type resource can't
simply do the same, because `id` is `@NotNull` and is only set from the path
inside the resource method.
**Why "leave unchanged" rather than 400:**
- On create, a null `isSystemDefined` already means `false`:
`PaymentTypeCreateRequestMapper` uses `Boolean.TRUE.equals(...)`. The field was
`required` in the OpenAPI spec, so the generated client sent `null` whenever a
caller didn't set it, and many existing callers create payment types that way.
Enforcing `@NotNull` would make all of those creates fail with 400.
- The documents reference implementation's update also treats null fields as
"unchanged".
- Treating null as `false` would quietly clear the flag on seeded
system-defined payment types whenever a client edits one without sending it.
Changes:
- `updatePaymentType` applies `isSystemDefined` only when it is non-null.
- `PaymentTypeUpdateRequest.isSystemDefined` drops `@NotNull` and
`@Builder.Default false`, so an omitted field also means "unchanged".
`@NoArgsConstructor` used to default an omitted field to `false`.
- As a result, `isSystemDefined` is no longer `required` on
`PaymentTypeUpdateRequest` in the OpenAPI spec. The create request is unchanged.
### Tests
- New `PaymentTypeWriteServiceImplTest` covers:
- a null flag and an omitted flag (both keep the stored value)
- an explicit `false` (overwrites the stored value)
- `DataIntegrityViolationException` and `JpaSystemException` (both become
a data integrity error)
- a missing payment type (still not found)
- `PaymentTypeIntegrationTest` has two new tests:
- A PUT sending a literal `"isSystemDefined": null` returns 200 and the
flag stays `true`. It uses a hand-written Feign interface, as
`SmsApiResourceIntegrationTest` does, because the generated model drops nulls.
- A PUT that omits `position` (`order_position` is NOT NULL) returns 403
with `error.msg.paymenttypes.unknown.data.integrity.issue`. It returned 404
before.
I ran both suites locally against PostgreSQL and they pass. I have not run
them on MySQL or MariaDB.
### Not in this PR
- The `@Valid` on the payment type create/delete service methods, and on the
documents service, still has no effect.
- `PaymentTypeCreateRequest.isSystemDefined` keeps a `@NotNull` that the
mapper's null-to-false default contradicts.
## Checklist
- [x] Write the commit message as per [our
guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
- [x] Acknowledge that we will not review PRs that are not passing the build
_("green")_ - it is your responsibility to get a proposed PR to pass the build,
not primarily the project's maintainers.
- [x] Create/update [unit or integration
tests](https://fineract.apache.org/docs/current/#_testing) for verifying the
changes made.
- [x] Follow our [coding
conventions](https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions).
- [ ] Add required Swagger annotation and update API documentation at
fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with
details of any API changes
- [x] [This PR must not be a "code
dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit).
Large changes can be made in a branch, with assistance. Ask for help on the
[developer mailing list](https://fineract.apache.org/#contribute).
- [x] If merging this PR resolves a JIRA issue, I will mark that issue as
resolved and set "Fix Version/s" appropriately.
- [x] I followed the [AI
Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
