Cocoa-Puffs opened a new pull request, #6784: URL: https://github.com/apache/fineract/pull/6784
## Description ### Problem After a command was processed, `CommandSource.updateForAudit` copied every identifier from the `CommandProcessingResult` over the values the command was submitted with, setting `null` when the handler returned nothing for a field. Neither `m_portfolio_command_source` nor the audit API could be searched by submitted values: for example, a loan transaction adjustment stored the replacement transaction's id in `resource_id`, losing the adjusted transaction's id. ### Change - The existing identifier columns (`office_id`, `group_id`, `client_id`, `loan_id`, `savings_account_id`, `product_id`, `transaction_id`, `resource_id`, `subresource_id`, `resource_external_id`, `subresource_external_id`, `loan_external_id`) now keep the submitted values and are never overwritten. - What the command returned is stored in matching new `result_*` columns. - A new `result_ids_written` flag marks rows written after this change. Older rows keep the returned values in the original columns, as before. - Migration `0276_add_result_columns_to_command_source.xml` only adds nullable columns and a boolean with a constant default. There is no backfill, so it is fast on large tables. ### API `/audits`, `/v2/audits` and `/makercheckers` report every identifier twice. All new fields and filters are additions; existing ones behave as before. - **Returned values, unprefixed.** `resourceId`, `subresourceId`, `clientId` and `loanId` are unchanged. `officeId`, `groupId`, `savingsAccountId`, `productId`, `transactionId`, `resourceExternalId`, `subresourceExternalId` and `loanExternalId` are new. For a command that was never processed they hold the submitted values, as the existing fields always did. - **Submitted values, prefixed `submitted`** (e.g. `submittedResourceId`). All twelve are new. - **Filters:** every identifier, returned or submitted, can be used as a query parameter. - **Sorting:** every identifier can be used as an `orderBy` value on `/audits` and `/v2/audits`. The audit table reads `result_*` or the original column for each row (`AuditColumns`), so the API and the database report the same values. **Rows written before the upgrade.** These do not have their submitted values: before this change, they were overwritten once processing reached the point of recording what the command returned. `submitted*` is therefore `null` on old rows, unless their status shows the command never got that far (awaiting approval, rejected, under processing). Old failed rows are included in the `null` case, because a command that failed at its final save had already been overwritten. A search by submitted value only finds commands that still have it. The Swagger description of each `submitted*` field says so. **Generated clients.** The new query parameters are listed after the existing ones. The generated `fineract-client` / `fineract-client-feign` methods take every query parameter by position, so callers passing positional arguments need to add them, or switch to the generated `...QueryParams` builder, as `AuditHelper` now does. ### Deviation from the issue There is no `result_identifier` column. `resource_identifier` was never a stored column, so nothing overwrote it, and its value is still in the `result` JSON. The proposed `result_identifier` / `result_external_id` are covered by `result_resource_id` / `result_resource_external_id`, alongside `result_*` for every other identifier that used to be overwritten. ### Alternative considered: backfilling old rows The alternative was a migration that copies the identifiers of existing rows into the new `result_*` columns. That would make every row look the same and remove the need for the `result_ids_written` flag and the per-row choice in the audit queries. The office filter could also use an index on `result_office_id`, avoiding the cost described below. We chose not to do this because the migration would rewrite every row of `m_portfolio_command_source`. That table only grows: it holds one row per processed command, and the purge job is disabled by default. On PostgreSQL an `UPDATE` writes a new copy of each row, and on a large installation the upgrade could take hours to days at startup, with the application down the whole time. Adding nullable columns and a boolean with a constant default only changes metadata, so the upgrade stays fast whatever the table size. A backfill would also not recover anything: older rows never stored their submitted values, so they cannot be searched by them either way. ### Known cost For users outside head office, paged audit requests (`/audits?paged=true`, every `/v2/audits` call) and office-filtered `/audits` and `/makercheckers` requests can no longer use the `office_id` index. The total-count query behind paging now scans the whole command table, where before it read only the branch's rows. Head-office users and unpaged requests are unaffected, and no business logic queries this table by office. A stored generated column was rejected for the same reason as the backfill: it rewrites the table. If needed, an index on `result_office_id`, with the office conditions split into two OR branches, can be added later without a table rewrite. Filters on submitted values compare the original column directly, so they can use that column's indexes. ### Note for SQL authors Migration `1036_link_unlinked_charge_adjustments.xml` (loan module) reads `resource_id` as the returned transaction id. It is unaffected: on an upgrade that applies both, `0276` runs first but only adds columns, and no command is processed while Liquibase runs, so 1036 still sees every row as written by the old code (`result_ids_written = false`). New SQL must read `result_*` for returned identifiers on flagged rows. ## Checklist Please make sure these boxes are checked before submitting your pull request - thanks! - [ ] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests) - [ ] 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. - [ ] Create/update [unit or integration tests](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made. - [ ] 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 - [ ] [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). - [ ] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately. - [ ] I followed the [AI Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy). Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide). -- 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]
