FINERACT-2501: Add operationIds and refactor feign methods to fix unstable deduplication numbers generated by swagger#5597
Conversation
edk12564
changed the title
edk12564
force-pushed
the
FINERACT-2501-in-use
branch
from
e77c0ec to
9e263a0
Compare
|
Looks like changes to WorkingCapitalStepDef weren't in this commit. That's my bad. Repushing now! |
edk12564
force-pushed
the
FINERACT-2501-in-use
branch
2 times, most recently
from
1ba2514 to
4b81df6
Compare
|
@edk12564 Would you mind to introduce operationId for ALL of our APIs, please? |
Of course! Would it be okay to have 2 PRs though between me and @Ambika-Sony . I have been communicating with her to split the ticket since she wanted to help out. We took a 50/50 split on modules earlier. I also saw the self service API got removed, so I'll remove any changes there. I also found a lot of items I missed. I was looking in the wrong build file. Extremely sorry about that. Making edits now. |
|
sure. you can split ;) |
edk12564
force-pushed
the
FINERACT-2501-in-use
branch
from
4b81df6 to
07ca9b6
Compare
I see now after some reading that we can just have 2 PRs since the tests in our PR get run. Sorry about that. I made a lot of progress, and build was working locally. However, I am not done yet. I am just pushing so @Ambika-Sony can access these changes. Since they are required to pass builds/tests. I also found some endpoints in FinancialActivityAccountsApiResource that were named createGLAccount. However, it seems these endpoints actually map a GLAccount to a FinancialActivityAccount, and creating a GL Account is done in GLAccountResourceApi. I changed the operationId here to more closely match it's actual function. Thank you for being patient with me! Also, before merging, we must also make sure the order with Self-Service removal is correct. Currently I left self-service changes in. However, when the full merge to delete goes through, i can refactor to remove those changes. Just depends on which gets merged first.
Also, a quick clarification request on this. Do you mean all API endpoints or just the deduplicated ones? I have been working through the OpenApi spec and choosing deduplicated items. However, I can do this for all endpoints as well! |
|
Hi , thank you for the update! I am completely on board with splitting the
work 50/50. I've seen your latest push, Edward—I will pull those changes
into my local environment shortly to sync up. I'll continue working on my
assigned modules today. Great to be working with you both on this!
…On Thu, 12 Mar, 2026, 8:04 am edk12564, ***@***.***> wrote:
*edk12564* left a comment (apache/fineract#5597)
<#5597 (comment)>
sure. you can split ;)
I see now after some reading that we can just have 2 PRs since the tests
in our PR get run. Sorry about that.
I made a lot of progress, and build was working locally. However, I am not
done yet. I am just pushing so @Ambika-Sony
<https://github.com/Ambika-Sony> can access these changes. Since they are
required to pass builds/tests.
I also found some endpoints in FinancialActivityAccountsApiResource that
were named createGLAccount. However, it seems these endpoints actually map
a GLAccount to a FinancialActivityAccount, and creating a GL Account is
done in GLAccountResourceApi. I changed the operationId here to more
closely match it's actual function.
Thank you for being patient with me!
Also, before merging, we must also make sure the order with Self-Service
removal is correct. Currently I left self-service changes in. However, when
the full merge to delete goes through, i can refactor to remove those
changes. Just depends on which gets merged first.
—
Reply to this email directly, view it on GitHub
<#5597 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BIN7S2GGRKMAB3DPNTFQBOD4QIO2XAVCNFSM6AAAAACWLGS4MSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DANBTGQ4DONJXGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
*Hi,* I have consolidated my work for *FINERACT-2494* into a clean Pull
Request (*#5615*). I’ve merged the latest changes from upstream and
resolved all local conflicts.
I wanted to flag a discrepancy I'm seeing:
-
*Locally:* ./gradlew :fineract-client:compileJava passes successfully.
-
*On GitHub:* The build is currently showing *34 failing checks*.
Given the high number of failures, I suspect there might be
environment-related issues or broader build instabilities. Could you please
take a look at the PR when you have a moment to see if these failures are
related to my changes?
Also, I wanted to confirm the best approach for the unit tests: should I
keep them under *FINERACT-2494*, or would you prefer them moved to
*FINERACT-2501*?
…On Thu, Mar 12, 2026 at 10:29 AM Ambika ***@***.***> wrote:
Hi , thank you for the update! I am completely on board with splitting the
work 50/50. I've seen your latest push, Edward—I will pull those changes
into my local environment shortly to sync up. I'll continue working on my
assigned modules today. Great to be working with you both on this!
On Thu, 12 Mar, 2026, 8:04 am edk12564, ***@***.***> wrote:
> *edk12564* left a comment (apache/fineract#5597)
> <#5597 (comment)>
>
> sure. you can split ;)
>
> I see now after some reading that we can just have 2 PRs since the tests
> in our PR get run. Sorry about that.
>
> I made a lot of progress, and build was working locally. However, I am
> not done yet. I am just pushing so @Ambika-Sony
> <https://github.com/Ambika-Sony> can access these changes. Since they
> are required to pass builds/tests.
>
> I also found some endpoints in FinancialActivityAccountsApiResource that
> were named createGLAccount. However, it seems these endpoints actually map
> a GLAccount to a FinancialActivityAccount, and creating a GL Account is
> done in GLAccountResourceApi. I changed the operationId here to more
> closely match it's actual function.
>
> Thank you for being patient with me!
>
> Also, before merging, we must also make sure the order with Self-Service
> removal is correct. Currently I left self-service changes in. However, when
> the full merge to delete goes through, i can refactor to remove those
> changes. Just depends on which gets merged first.
>
> —
> Reply to this email directly, view it on GitHub
> <#5597 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BIN7S2GGRKMAB3DPNTFQBOD4QIO2XAVCNFSM6AAAAACWLGS4MSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DANBTGQ4DONJXGI>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
|
Hi @Ambika-Sony, I took a look at your PR. I think your build is working locally but failing on CI/CD because you also need to push the changes to in-use feign methods that you pulled from here. But considering this is the case, I wonder if we should just work on this 1 PR together. This would prevent the issue of us pushing the same code at the same time. What do you think about just working on this 1 PR together? I believe Fineract allows multiple users to commits to a PR as long as long as each user squashes and there is only 1 commit per user. It's either that or this PR gets pushed first, then we work on top of those changes. Also, I saw a concerning deletion in your PR. Could you kindly take a look at my review when you have time? I wonder if this was a mistake, since we shouldn't be deleting logic for this type of change.
|
|
I’ll take a closer look at the deletion you pointed out in the review and
restore the logic if it was removed accidentally during conflict resolution
and Working on a single PR together sounds good to me as well. That should
definitely help avoid duplicate changes and CI conflicts. I’ll sync with
your latest updates and adjust my changes accordingly.
…On Thu, Mar 12, 2026 at 11:01 PM edk12564 ***@***.***> wrote:
*edk12564* left a comment (apache/fineract#5597)
<#5597 (comment)>
Hi @Ambika-Sony <https://github.com/Ambika-Sony>, I took a look at your
PR. I think your tests are passing locally but failing on CI/CD because you
also need to push the changes to in-use feign methods that you pulled from
here. But considering this is the case, I wonder if we should just work on
this 1 PR together. This would prevent the issue of us pushing the same
code at the same time.
What do you think about just working on this 1 PR together? I believe
Fineract allows multiple users to commits to a PR as long as long as there
is only 1 commit per user.
I also saw a concerning deletion in your PR. Could you kindly take a look
at my review when you have time? I wonder if this was a mistake, since we
shouldn't be deleting logic for this type of change.
*Hi,* I have consolidated my work for *FINERACT-2494* into a clean Pull
Request (*#5615 <#5615>*). I’ve
merged the latest changes from upstream and resolved all local conflicts. I
wanted to flag a discrepancy I'm seeing: - *Locally:* ./gradlew
:fineract-client:compileJava passes successfully. - *On GitHub:* The
build is currently showing *34 failing checks*. Given the high number of
failures, I suspect there might be environment-related issues or broader
build instabilities. Could you please take a look at the PR when you have a
moment to see if these failures are related to my changes? Also, I wanted
to confirm the best approach for the unit tests: should I keep them under
*FINERACT-2494*, or would you prefer them moved to *FINERACT-2501*?
… <#m_-58939957042714022_>
On Thu, Mar 12, 2026 at 10:29 AM Ambika *@*.*> wrote: Hi , thank you for
the update! I am completely on board with splitting the work 50/50. I've
seen your latest push, Edward—I will pull those changes into my local
environment shortly to sync up. I'll continue working on my assigned
modules today. Great to be working with you both on this! On Thu, 12 Mar,
2026, 8:04 am edk12564, @.*> wrote: > *edk12564* left a comment (
apache/fineract#5597 <#5597>) > <#5597
(comment)
<#5597 (comment)>> >
> sure. you can split ;) > > I see now after some reading that we can just
have 2 PRs since the tests > in our PR get run. Sorry about that. > > I
made a lot of progress, and build was working locally. However, I am > not
done yet. I am just pushing so @Ambika-Sony
<https://github.com/Ambika-Sony> > https://github.com/Ambika-Sony can
access these changes. Since they > are required to pass builds/tests. > > I
also found some endpoints in FinancialActivityAccountsApiResource that >
were named createGLAccount. However, it seems these endpoints actually map
> a GLAccount to a FinancialActivityAccount, and creating a GL Account is >
done in GLAccountResourceApi. I changed the operationId here to more >
closely match it's actual function. > > Thank you for being patient with
me! > > Also, before merging, we must also make sure the order with
Self-Service > removal is correct. Currently I left self-service changes
in. However, when > the full merge to delete goes through, i can refactor
to remove those > changes. Just depends on which gets merged first. > > — >
Reply to this email directly, view it on GitHub > <#5597 (comment)
<#5597 (comment)>>,
> or unsubscribe >
https://github.com/notifications/unsubscribe-auth/BIN7S2GGRKMAB3DPNTFQBOD4QIO2XAVCNFSM6AAAAACWLGS4MSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DANBTGQ4DONJXGI
> . > You are receiving this because you were mentioned.Message ID: > *@*.***>
>
—
Reply to this email directly, view it on GitHub
<#5597 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BIN7S2HAF22A57C7A6PA7VL4QLYAPAVCNFSM6AAAAACWLGS4MSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DANBYGY2DINZSGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@edk12564 @Ambika-Sony |
@adamsaghy Just rebased! |
edk12564
force-pushed
the
FINERACT-2501-in-use
branch
from
07ca9b6 to
8142693
Compare
|
@edk12564 Please rebase. |
…hod names - rebased locally
edk12564
force-pushed
the
FINERACT-2501-in-use
branch
from
8142693 to
1fd8ca3
Compare
edk12564
force-pushed
the
FINERACT-2501-in-use
branch
from
1fd8ca3 to
50d0806
Compare
Rebased again! I just finished working on the client related endpoints as well. Will work on Products next. |
4 participants
JIRA: FINERACT-2501
Description
According to 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.
To start, the currently used Feign methods must be refactored, and the endpoints must have operationIds added to them. This is because changes in other modules will cause these in use methods to change due to instability. This results in build failures.
Changes
The methodology for naming OperationIds is methodName + entityName + descriptionClause. For example, update() for SavingsProducts that updates using an externalId becomes updateSavingsProductsByExternalId.
More Changes
Certain method names were named improperly. In FinancialActivityAccountsApiResource, we have createGLAccount endpoints. However, this endpoint actually creates a mapping between a FinancialActivityAccount and a GLAccount that already exists. For this case, I updated the operationId to be more descriptive on the actual function.
Results
All affected tests in integration and Cucumber tests pass.
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.