[PR] FINERACT-2953: Keep datatable entry writes to the named entity's row [fineract]

["pwargulak (via GitHub)" <gi...@apache.org>]

pwargulak opened a new pull request, #6663:
URL: https://github.com/apache/fineract/pull/6663

   ## Description
   
   [FINERACT-2953](https://issues.apache.org/jira/browse/FINERACT-2953)
   
   `PUT` and `DELETE /datatables/{datatable}/{apptableId}/{datatableId}` check that `apptableId` exists and is within the user's office scope, but choose the row they act on by `datatableId`. The batch routes and `DELETE /survey/{surveyName}/{clientId}/{fulfilledId}` reach the same code. As a result:
   
   - on a **one-to-one** datatable, a PUT with a second id reads `apptableId`'s row and then updates the row of the entity whose id is `datatableId`;
   - on a **one-to-one** datatable, a DELETE with a second id runs `DELETE … WHERE "id" = ?` and answers 403 with the raw database error (`column "id" does not exist`);
   - on a **one-to-many** datatable, a DELETE by entry id deletes the entry with that id whatever entity it belongs to.
   
   This change:
   
   - refuses an entry id on a one-to-one datatable, for update and delete, with 403 `error.msg.datatable.entry.id.not.allowed.for.one.to.one` (a `PlatformDataIntegrityException`, like the existing `error.msg.attempting.multiple.update` for a one-to-many update without an id);
   - limits a one-to-many delete by id to `apptableId`'s entries (`WHERE id = ? AND <fk> = ?`) and answers 404 `error.msg.datatable.data.not.found` when nothing is deleted, as the update already does. **A delete of an id that does not exist now answers 404 instead of 200**;
   - keys the UPDATE on the primary key of the row it has just read for `apptableId`, instead of `datatableId ?? apptableId`. On stock datatables this is the same value. It differs only for a table registered via `/datatables/register` whose primary key is the foreign key and that also has a plain `id` column: such a table reads as one-to-many, and the old code updated the row keyed by `datatableId`;
   - updates the Swagger descriptions and `apiLive.htm` for the two routes.
   
   Tests: `DatatableWriteServiceImplTest` +6 (refusals, scoped delete SQL, 0-row delete, the PK key for both table shapes); `DatatableBusinessEventTest` stubs the multi-row check and the delete count; new `FeignDatatableEntryIdTest` (5 cases on client datatables), 4 of which fail on `develop`.
   
   Run locally on PostgreSQL: the provider's `dataqueries` and `batch` unit tests; `FeignDatatableEntryIdTest`, `DatatableIntegrationTest`, `BatchApiTest`, `SavingsAccountTransactionDatatableIntegrationTest`, `EntityDatatableChecksIntegrationTest`, `DatatableAdvancedQueryTest` and `FeignClientEntityDatatableCheckSubtypeTest` all pass; spotless, checkstyle, spotbugs and modernizer are clean.
   
   ## 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).
   - [x] 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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2953: Keep datatable entry writes to the named entity's row [fineract]

["adamsaghy (via GitHub)" <gi...@apache.org>]

adamsaghy merged PR #6663:
URL: https://github.com/apache/fineract/pull/6663


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2953: Keep datatable entry writes to the named entity's row [fineract]

["pwargulak (via GitHub)" <gi...@apache.org>]

pwargulak commented on PR #6663:
URL: https://github.com/apache/fineract/pull/6663#issuecomment-6286303863

   @adamsaghy re-signed commits correctly


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2953: Keep datatable entry writes to the named entity's row [fineract]

["pwargulak (via GitHub)" <gi...@apache.org>]

pwargulak commented on PR #6663:
URL: https://github.com/apache/fineract/pull/6663#issuecomment-6267558388

   Thanks @galovics, all five addressed in the new commit:
   
   - `whereValue`: it was still the result's `entityId`, but your shape is cleaner: the `DELETE` is now built once (`WHERE <fk> = ?`, plus `AND id = ?` only when `datatableId != null`), and the entity id is `datatableId ?? appTableId` inline.
   - 404 message: agreed, it named the wrong id. A new `DatatableNotFoundException(datatable, appTableId, datatableId)` names both; it keeps the code `error.msg.datatable.data.not.found`, and the update's not-found for an entry id uses it too, so both routes read the same.
   - Client-visible changes: added a release-note paragraph to the description (404 for a missing or foreign entry, 403 for an entry id on one-to-one). On `x, x`: in openMF/web-app (`develop`, 947023e) the two-id PUT and DELETE are called only from `datatable-multi-row.component.ts`, and `edit-client.component.ts` sends the entry id only when `section.isMultiRow`; every one-to-one write sends one id. I found no other client that sends it.
   - PK hunk: it is part of this ticket. It is the same read-one-row, write-another defect: on a registered table whose PK is the FK and which also has an `id` column, the read is filtered `fk = a AND id = b` (a's row) while the old `UPDATE … WHERE pk = b` wrote b's row. The cast is gone: the PK value read is bound as is, and only the reported entity id takes it when it is numeric. A non-numeric PK used to be bound as a `Long`; now it gets its own value.
   - Tests: new integration cases for `DELETE /survey/{name}/{clientId}/{fulfilledId}` and for the `/batches` route, both deleting another client's entry (404, the entry survives). The two 404 cases are merged into one. The unit tests that restated the SQL (`…IsScopedToApplicationTableId`, `…KeysOnItsId`) are dropped; what is left covers the refusals, the 0-row delete firing no event, and the registered-table PK, which no IT reaches.
   


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org