[PR] FINERACT-2948: Fix GET /glaccounts searchParam causing HTTP 500 [fineract]

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

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

   ## Description
   
   `GET /glaccounts` has two long-standing defects in `GLAccountReadPlatformServiceImpl`:
   
   1. **`searchParam` causes HTTP 500** — the `LIKE` clause wraps the JDBC placeholder in literal `%` characters (`name like %?%`), resulting in invalid placeholder syntax.
   
   2. **GL accounts without a tag return an incorrect `tagId`** — accounts without a tag are returned with `"tagId": {"id": 0}` instead of omitting `tagId`. The `wasNull()` check reads the wrong column, and `CodeValueData` is created unconditionally even when no tag is present.
   
   ## 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: issues-unsubscribe@fineract.apache.org

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

Re: [PR] FINERACT-2948: Fix GET /glaccounts searchParam causing HTTP 500 [fineract]

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

SamaSVM commented on PR #6647:
URL: https://github.com/apache/fineract/pull/6647#issuecomment-6415710236

   @galovics  Thanks for the review.
   
   **1. `concat('%', ?, '%')` → bound wildcards**
   
   Agreed. This is simpler and matches the existing convention (`ClientReadPlatformServiceImpl:177`, `GroupReadPlatformServiceImpl:211`, `SearchReadServiceImpl:82`), where `"%" + param + "%"` is bound as a parameter:
   
   ```java
   sql += " ( name like ? or gl_code like ? )";
   final String searchPattern = "%" + searchParam + "%";
   ```
   
   This also allowed me to remove the `sql.contains("concat(...)")` assertion, which was unnecessarily tied to the exact SQL text.
   
   **2. `ChartOfAccountsWorkbook` — good catch, and it's more than cosmetic**
   
   This is a regression that the PR would have introduced.
   
   Before the mapper fix, `codeId` for an untagged account was `0L` (due to the `wasNull()` bug). As a result, `getTagId()` returned `CodeValueData(0L, null)`, and the row was generated as `Name-1-null-0`. The `setLookupTable` logic detected this through `accountNameAndTagAr.get(3).equals("0")`.
   
   After `getTagId()` was correctly changed to return `null`, the row became `Name-1-null-null`. The `"0"` check no longer matched, so the literal `"null"` flowed into `LOOKUP_TAG_ID_COL` → VLOOKUP → `Long.parseLong("null")` in `ChartOfAccountsImportHandler:134`, resulting in a `NumberFormatException` during chart-of-accounts import.
   
   I fixed this by writing an empty string instead:
   
   ```java
   + (tag == null ? "" : tag.getName()) + "-" + (tag == null ? "" : tag.getId())
   ```
   
   I chose `""` rather than removing the tag suffix because `setLookupTable` unconditionally accesses `get(2)` and `get(3)`, so a shorter string would result in an `IndexOutOfBoundsException`.
   
   With `""`, both branches fall through to their respective `else` clauses and write `""` — the same cell value that the existing special-case branches already produced. `ImportHandlerUtils.readAsString` then returns `null` for an empty formula result, so `Long.parseLong` is never reached.
   
   **3. `searchGLAccountsBySearchParamTest`**
   
   Right — `name` and `glCode` had the same value, so the "found by glCode" case never actually exercised the `gl_code like` branch.
   
   I added a `createGLAccount(name, glCode)` overload and changed the two accounts to use distinct `..._NAME` / `..._CODE` values. This ensures that the second lookup can only match through `gl_code`.
   
   I also moved the cleanup into `try/finally`.
   
   **4. `GLAccountReadPlatformServiceImplTest`**
   
   Agreed. I removed the hand-rolled `ResultSet` and the `shouldMapMissingTagAsNullCodeId` test, since the integration test already verifies `getTagId() == null` against a real database.
   
   What's left is one short test that verifies the bound parameters without depending on the SQL text:
   
   ```java
   assertFalse(sql.contains("%?%"));
   assertArrayEquals(new Object[] { "%cash%", "%cash%" }, paramsCaptor.getValue());
   ```


-- 
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