You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "edk12564 (via GitHub)" <gi...@apache.org> on 2026/04/07 00:57:54 UTC
[PR] FINERACT-2501: Add more operationIds to harden feign method names [fineract]
edk12564 opened a new pull request, #5744:
URL: https://github.com/apache/fineract/pull/5744
JIRA: FINERACT-2501
Description
According to [FINERACT-2501](https://issues.apache.org/jira/browse/FINERACT-2501), deduplication numbers in Feign client methods are unstable. More on this below.
The Problem
Fineract uses Swagger/OpenAPI to auto-generate Feign client methods from our endpoints. Since many endpoints share the same method name (e.g., retrieveAll()), Swagger appends a deduplication number to create unique operationIds. For example, retrieveAll35() for SavingsProductsApiResource.retrieveAll().
These deduplication numbers are unstable. When a new endpoint with a duplicate name is added, the numbers can shift. In my testing, after adding a new dummy retrieveAll() endpoint, retrieveAll35() no longer pointed to SavingsProductsApiResource. It actually shifted to retrieveAll36().
Since we hardcode these deduplicated method names in our Cucumber tests, any shift will silently call the wrong API, breaking tests.
Changes
Added operationIds to Accounting, Products, SMS, Transfers, Reporting, Tellers and Cashiers, Progressive Loans, Groups, Centers and Shares domains.
Does not overlap with @Ambika-Sony on Savings, Loans, Infrastructure, Organization, User Management. Completed my half of operationId additions. So far ~50% of all endpoints in Fineract have operationIds.
Results
Compilation/Build are successful.
<img width="849" height="18" alt="Screenshot 2026-04-06 at 7 56 36 PM" src="https://github.com/user-attachments/assets/c3e36b4d-45c4-4277-b5ef-2ecfcce6ff7d" />
<img width="1245" height="189" alt="Screenshot 2026-04-06 at 7 56 05 PM" src="https://github.com/user-attachments/assets/a5ec5bb5-a70b-4fe0-9e49-fd66b8ed9411" />
## 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).
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-2501: Add more operationIds to harden feign method names [fineract]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on PR #5744:
URL: https://github.com/apache/fineract/pull/5744#issuecomment-4201026445
Hi Sandro! Thanks for the comment.
That's a really interesting point. It is true that once these operationIds are finished being added (remainder to be added by @Ambika-Sony and from here https://github.com/apache/fineract/pull/5597), new endpoints in Fineract could result in deduplication again if named generically. If I am understanding correctly, your idea is to write a script that reads the fineract.json, compares it to a predefined spec, and makes edits to the generated fineract.json with operationIds generated from path + http method. Is that right?
Overall I agree some kind of automated check better for future maintainability purposes. But we have to be careful with implementation. There might be a few problems:
- we'd have to update our predefined spec manually when new endpoints are added, since we cannot use the build to verify against itself
- there are some edge cases where the method name does not properly convey the functionality. however, changing the method breaks other items in the codebase (for example, in batch steps which directly call endpoints). i was using operationId as a way of translating these bad method names into something usable for tests. for example, in FinancialActivityAccountsApiResource, we have createGLAccount. however, this endpoint actually just creates a mapping between a FinancialActivityAccount and a GLAccount that already exists. there is another createGLAccount that handles creating GL accounts.
- path + http method might be too verbose. for example, retrieveTransactionByLoanExternalIdAndTransactionExternalId would become loansExternalIdLoanExternalIdTransactionsExternalIdExternalTransactionId().
I think looking over each endpoint and its functionality might take a long time, but it's allowing me to properly translate these method names into usable names for tests. Which I think is still valuable.
What do you think about this?
--
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-2501: Add more operationIds to harden feign method names [fineract]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on PR #5744:
URL: https://github.com/apache/fineract/pull/5744#issuecomment-4209784065
> > Hi @edk12564 and @adamsaghy, thank you for checking in. I am currently working on the final endpoints, though I'm facing a few minor issues with merge conflicts and checkstyle compliance after the recent rebase onto the 1.15.0 structure. I’m currently resolving these to ensure the build stays 'green' while I add the remaining IDs. I expect to have the updated PR submitted for review shortly. Thanks for your patience!"
> > […](#)
> > On Tue, 7 Apr, 2026, 11:06 pm edk12564, _**@**_._**> wrote: _edk12564_ left a comment ([apache/fineract#5744](https://github.com/apache/fineract/pull/5744)) <[#5744 (comment)](https://github.com/apache/fineract/pull/5744#issuecomment-4201026445)> Hi Sandro! Thanks for the comment. That's a really interesting point. It is true that once these operationIds are finished being added (remainder to be added by @Ambika-Sony https://github.com/Ambika-Sony and from here #5597 <#5597>), new endpoints in Fineract could result in deduplication again if named generically. If I am understanding correctly, your idea is to write a script that reads the fineract.json, compares it to a predefined spec, and makes edits to the generated fineract.json with operationIds generated from path + http method. Is that right? Overall I agree some kind of automated check better for future maintainability purposes. But we have to be careful with implementation. There might be a few problems: - we'd have t
o update our predefined spec manually when new endpoints are added, since we cannot use the build to verify against itself - there are some edge cases where the method name does not properly convey the functionality. however, changing the method breaks other items in the codebase (for example, in batch steps which directly call endpoints). i was using operationId as a way of translating these bad method names into something usable for tests. for example, in FinancialActivityAccountsApiResource, we have createGLAccount. however, this endpoint actually just creates a mapping between a FinancialActivityAccount and a GLAccount that already exists. there is another createGLAccount that handles creating GL accounts. - path + http method might be too verbose. for example, retrieveTransactionByLoanExternalIdAndTransactionExternalId would become loansExternalIdLoanExternalIdTransactionsExternalIdExternalTransactionId(). I think looking over each endpoint and its functionality might take a lo
ng time, but it's allowing me to properly translate these method names into usable names for tests. Which I think is still valuable. What do you think about this? — Reply to this email directly, view it on GitHub <[#5744 (comment)](https://github.com/apache/fineract/pull/5744#issuecomment-4201026445)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/BIN7S2GFFOE5RAHPQNIUC634UU4CBAVCNFSM6AAAAACXOZEDH2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMBRGAZDMNBUGU . You are receiving this because you were mentioned.Message ID: _**@**_.**_>
>
> @edk12564 @Ambika-Sony Are you working on the same thing? which pr should be merged first?
Hi there @adamsaghy. I think we should be working on different things. @Ambika-Sony requested to work on the Savings, Loans, Infrastructure, Organization, User Management related APIs so I made sure to avoid those in this PR. Should be good to merge!
--
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-2501: Add more operationIds to harden feign method names [fineract]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on PR #5744:
URL: https://github.com/apache/fineract/pull/5744#issuecomment-4210085864
Hi @adamsaghy ! I also found something from that one failing integration test earlier. After examining, it seems to be a flaky test due to a product short name collision in WorkingCapitalLoanDisbursementTest's createProduct(). I think we should probably be using uniqueRandomStringGenerator from Utils instead of generating a random UUID for the product short name. Do you think I could open a ticket to make this change?
--
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-2501: Add more operationIds to harden feign method names [fineract]
Posted by "San-43 (via GitHub)" <gi...@apache.org>.
San-43 commented on PR #5744:
URL: https://github.com/apache/fineract/pull/5744#issuecomment-4196799333
@edk12564, a quick thought – and I might be missing some background – but wouldn't it be better to handle this within the OpenAPI generation process? Perhaps a deterministic fallback for those missing `operationIds` would be the way to go.
Something like:
- keep explicit `operationIds` where we want to preserve public/client-facing names
- generate stable fallback IDs for any endpoint that doesn't define one
My concern with adding them manually across resources is that it fixes only the endpoints touched here, while the underlying instability is still there for any endpoint that doesn't have an explicit `operationId`. That feels harder to maintain over time than fixing it once in the generation step.
I think the more robust fix is build-time and centralized: let `:fineract-provider:resolve` generate the spec as today, then add a normalization/validation step on the generated `fineract.json` that:
- preserves existing explicit `operationIds` unchanged
- generates deterministic fallback IDs only for missing ones, based on `path + http method`
- fails the build if the final `operationIds`are duplicated
That would solve the full problem for all endpoints, including future ones, without requiring a long tail of manual annotation updates. For example, `/v1/groups/{groupId}/clients + post `could deterministically become `groups_groupId_clients_post`.
--
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-2501: Add more operationIds to harden feign method names [fineract]
Posted by "Ambika-Sony (via GitHub)" <gi...@apache.org>.
Ambika-Sony commented on PR #5744:
URL: https://github.com/apache/fineract/pull/5744#issuecomment-4208728150
Hi @edk12564 and @adamsaghy, thank you for checking in.
I am currently working on the final endpoints, though I'm facing a few
minor issues with merge conflicts and checkstyle compliance after the
recent rebase onto the 1.15.0 structure. I’m currently resolving these to
ensure the build stays 'green' while I add the remaining IDs.
I expect to have the updated PR submitted for review shortly. Thanks for
your patience!"
On Tue, 7 Apr, 2026, 11:06 pm edk12564, ***@***.***> wrote:
> *edk12564* left a comment (apache/fineract#5744)
> <https://github.com/apache/fineract/pull/5744#issuecomment-4201026445>
>
> Hi Sandro! Thanks for the comment.
>
> That's a really interesting point. It is true that once these operationIds
> are finished being added (remainder to be added by @Ambika-Sony
> <https://github.com/Ambika-Sony> and from here #5597
> <https://github.com/apache/fineract/pull/5597>), new endpoints in
> Fineract could result in deduplication again if named generically. If I am
> understanding correctly, your idea is to write a script that reads the
> fineract.json, compares it to a predefined spec, and makes edits to the
> generated fineract.json with operationIds generated from path + http
> method. Is that right?
>
> Overall I agree some kind of automated check better for future
> maintainability purposes. But we have to be careful with implementation.
> There might be a few problems:
>
> - we'd have to update our predefined spec manually when new endpoints
> are added, since we cannot use the build to verify against itself
> - there are some edge cases where the method name does not properly
> convey the functionality. however, changing the method breaks other items
> in the codebase (for example, in batch steps which directly call
> endpoints). i was using operationId as a way of translating these bad
> method names into something usable for tests. for example, in
> FinancialActivityAccountsApiResource, we have createGLAccount. however,
> this endpoint actually just creates a mapping between a
> FinancialActivityAccount and a GLAccount that already exists. there is
> another createGLAccount that handles creating GL accounts.
> - path + http method might be too verbose. for example,
> retrieveTransactionByLoanExternalIdAndTransactionExternalId would become
> loansExternalIdLoanExternalIdTransactionsExternalIdExternalTransactionId().
>
> I think looking over each endpoint and its functionality might take a long
> time, but it's allowing me to properly translate these method names into
> usable names for tests. Which I think is still valuable.
>
> What do you think about this?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/fineract/pull/5744#issuecomment-4201026445>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BIN7S2GFFOE5RAHPQNIUC634UU4CBAVCNFSM6AAAAACXOZEDH2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMBRGAZDMNBUGU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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-2501: Add more operationIds to harden feign method names [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #5744:
URL: https://github.com/apache/fineract/pull/5744#issuecomment-4205816143
@edk12564 Please rebase this PR with latest `develop` branch. Many of the checks were skipped due to incorrect dependency in Github Actions CI jobs!
--
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-2501: Add more operationIds to harden feign method names [fineract]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on PR #5744:
URL: https://github.com/apache/fineract/pull/5744#issuecomment-4208316839
> @edk12564 Please rebase this PR with latest `develop` branch. Many of the checks were skipped due to incorrect dependency in Github Actions CI jobs!
Sounds good! Just rebased !!
--
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-2501: Add more operationIds to harden feign method names [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #5744:
URL: https://github.com/apache/fineract/pull/5744
--
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-2501: Add more operationIds to harden feign method names [fineract]
Posted by "San-43 (via GitHub)" <gi...@apache.org>.
San-43 commented on PR #5744:
URL: https://github.com/apache/fineract/pull/5744#issuecomment-4202246833
Hello @edk12564, I wasn't thinking about maintaining a separate predefined spec. I was thinking more about a build-time step in the existing OpenAPI generation flow.
To make the idea more concrete, I was imagining something in the existing `:fineract-provider:resolve` flow, for example a `doLast `step that normalizes the generated spec after Swagger writes it:
```groovy
doLast {
def result = new ClassForNormalization().normalize(file("${buildDir}/resources/main/static/fineract.json"),
file("${buildDir}/resources/main/static/fineract.yaml"), sourceSets.main.runtimeClasspath.files)
}
```
The idea would be to preserve explicit `operationIds`, generate deterministic fallback IDs only for missing ones, and fail the build if the final IDs are duplicated.
So I'm not suggesting replacing the manual `operationIds` you're adding here. I agree those still have value, especially for endpoints where path + method would be awkward or less meaningful, is just that from my perspective manual hardening alone does not fully solve the maintainability problem, because untouched or future endpoints can still fall back to unstable generated names. If you think it makes sense, we could open a Jira ticket for that follow-up.
--
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-2501: Add more operationIds to harden feign method names [fineract]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on PR #5744:
URL: https://github.com/apache/fineract/pull/5744#issuecomment-4202310865
> Hello @edk12564, I wasn't thinking about maintaining a separate predefined spec. I was thinking more about a build-time step in the existing OpenAPI generation flow. To make the idea more concrete, I was imagining something in the existing `:fineract-provider:resolve` flow, for example a `doLast `step that normalizes the generated spec after Swagger writes it:
>
> ```groovy
> doLast {
> def result = new ClassForNormalization().normalize(file("${buildDir}/resources/main/static/fineract.json"),
> file("${buildDir}/resources/main/static/fineract.yaml"), sourceSets.main.runtimeClasspath.files)
> }
> ```
>
> The idea would be to preserve explicit `operationIds`, generate deterministic fallback IDs only for missing ones, and fail the build if the final IDs are duplicated. So I'm not suggesting replacing the manual `operationIds` you're adding here. I agree those still have value, especially for endpoints where path + method would be awkward or less meaningful, is just that from my perspective manual hardening alone does not fully solve the maintainability problem, because untouched or future endpoints can still fall back to unstable generated names. If you think it makes sense, we could open a Jira ticket for that follow-up.
@San-43 That sounds good to me! I think the only potential issue I saw was in generation. Detection not only works but is necessary to fix the underlying problem just like you said. Please go ahead!
I will also mention though that completing operationIds on the remainder of the deduplicated feign methods remains a blocker on this. So we might need to wait on the rest of the endpoints to get modified. @Ambika-Sony Are you still working on this?
--
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-2501: Add more operationIds to harden feign method names [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #5744:
URL: https://github.com/apache/fineract/pull/5744#issuecomment-4209606351
> Hi @edk12564 and @adamsaghy, thank you for checking in. I am currently working on the final endpoints, though I'm facing a few minor issues with merge conflicts and checkstyle compliance after the recent rebase onto the 1.15.0 structure. I’m currently resolving these to ensure the build stays 'green' while I add the remaining IDs. I expect to have the updated PR submitted for review shortly. Thanks for your patience!"
> […](#)
> On Tue, 7 Apr, 2026, 11:06 pm edk12564, ***@***.***> wrote: *edk12564* left a comment ([apache/fineract#5744](https://github.com/apache/fineract/pull/5744)) <[#5744 (comment)](https://github.com/apache/fineract/pull/5744#issuecomment-4201026445)> Hi Sandro! Thanks for the comment. That's a really interesting point. It is true that once these operationIds are finished being added (remainder to be added by @Ambika-Sony <https://github.com/Ambika-Sony> and from here #5597 <#5597>), new endpoints in Fineract could result in deduplication again if named generically. If I am understanding correctly, your idea is to write a script that reads the fineract.json, compares it to a predefined spec, and makes edits to the generated fineract.json with operationIds generated from path + http method. Is that right? Overall I agree some kind of automated check better for future maintainability purposes. But we have to be careful with implementation. There might be a few problems: - we'd have t
o update our predefined spec manually when new endpoints are added, since we cannot use the build to verify against itself - there are some edge cases where the method name does not properly convey the functionality. however, changing the method breaks other items in the codebase (for example, in batch steps which directly call endpoints). i was using operationId as a way of translating these bad method names into something usable for tests. for example, in FinancialActivityAccountsApiResource, we have createGLAccount. however, this endpoint actually just creates a mapping between a FinancialActivityAccount and a GLAccount that already exists. there is another createGLAccount that handles creating GL accounts. - path + http method might be too verbose. for example, retrieveTransactionByLoanExternalIdAndTransactionExternalId would become loansExternalIdLoanExternalIdTransactionsExternalIdExternalTransactionId(). I think looking over each endpoint and its functionality might take a lo
ng time, but it's allowing me to properly translate these method names into usable names for tests. Which I think is still valuable. What do you think about this? — Reply to this email directly, view it on GitHub <[#5744 (comment)](https://github.com/apache/fineract/pull/5744#issuecomment-4201026445)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BIN7S2GFFOE5RAHPQNIUC634UU4CBAVCNFSM6AAAAACXOZEDH2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMBRGAZDMNBUGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>
@edk12564 @Ambika-Sony Are you working on the same thing? which pr should be merged first?
--
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