FINERACT-2293: update client address by nidhiii128 · Pull Request #6085 · apache/fineract

nidhiii128 · 2026-07-03T17:23:26Z

JIRA: https://issues.apache.org/jira/browse/FINERACT-2293

Migrates the update client address endpoint (PUT /v1/client/{clientId}/addresses)
from the legacy JsonCommand path to the new typed fineract-command framework.

Scope (PR 2 of 2 for FINERACT-2293)

AddressMapper extended with @MappingTarget partial-update method
(@BeanMapping(nullValuePropertyMappingStrategy = IGNORE) for clean null-skip)
AddressDomainService.update(AddressUpdateRequest) typed method
New AddressUpdateRequest / AddressUpdateResponse POJOs with Jakarta Validation
New AddressUpdateCommand and AddressUpdateCommandHandler
Controller routes PUT through CommandDispatcher, returns AddressUpdateResponse
AntMatcher added in SecurityConfig: PUT /api//client//addresses (UPDATE_ADDRESS)
Resilience4j retry config for commandAddressUpdate
Validation message bundle entries added
Legacy updateClientAddress(Long, JsonCommand) removed from
AddressWritePlatformService
Legacy CommandWrapperBuilder.updateClientAddress(Long) removed
Legacy UpdateClientAddressCommandHandler deleted

Depends on #6049 (PR 1 - create client address). This PR is currently based
on that branch. Once #6049 merge in develop, I will rebase this on develop and
force-push; that should reduce the diff to only the update-specific changes.

Pattern follows the merged template-module reference (#5851) and PR 1 (#6049).

Expected CI failure: run-api-backward-compatibility

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Write the commit message as per our guidelines
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 for verifying the changes made.
Follow our 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". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.

Your assigned reviewer(s) will follow our guidelines for code reviews.

vidakovic · 2026-07-07T15:50:43Z

    }

-    public CommandWrapperBuilder addClientAddress(final long clientId, final long addressTypeId) {
+    public CommandWrapperBuilder addClientAddress(final Long clientId, final Long addressTypeId) {


Why is this still here? CommandWrapperBuilder belongs to the legacy command processing... could you please explain why this is still relevant when we move those client commands to the new command processing?

vidakovic · 2026-07-07T15:54:41Z

                    .hasAnyAuthority(ALL_FUNCTIONS, ALL_FUNCTIONS_WRITE, "UPDATE_HOOK")
                    .requestMatchers(API_MATCHER.matcher(HttpMethod.DELETE, "/api/*/hooks/*"))
                    .hasAnyAuthority(ALL_FUNCTIONS, ALL_FUNCTIONS_WRITE, "DELETE_HOOK")
+


Seems like a minor thing, but try to avoid inserting blocks of code like this in the middle of previous changes... like this authorization configuration list in SecurityConfig. Maybe it makes more sense for you to put the client definitions at this position in the file, but for Git this means that you are potentially creating conflict situations that are hard to resolve, especially when other devs are working in the same area (and they are).

Pro tip: just go to the last entry of those authorization definitions and insert your (client) related definitions after the last entry. This is much easier to resolve and a lot nicer to your fellow developers.

vidakovic · 2026-07-07T15:57:06Z

+@ConditionalOnMissingBean(value = AddressDomainService.class, ignored = AddressDomainServiceImpl.class)
+public class AddressDomainServiceImpl implements AddressDomainService {
+
+    private final PlatformSecurityContext context;


Why do we still need this? The complete REST API is authentication protected... it makes pretty much no sense to double validate if a user is authenticated at this point. Please remove.

vidakovic · 2026-07-07T15:58:02Z

+    @Override
+    @Transactional
+    public AddressCreateResponse create(final AddressCreateRequest request) {
+        context.authenticatedUser();


See above comment about checking user authentication in business logic services. Keep in mind that removing this makes unit testing a ton easier.

vidakovic · 2026-07-07T15:58:27Z

+    @Override
+    @Transactional
+    public AddressUpdateResponse update(final AddressUpdateRequest request) {
+        context.authenticatedUser();


Remove all similar entries. See above comments.

vidakovic · 2026-07-07T16:01:00Z

-    private final ClientRepositoryWrapper clientRepositoryWrapper;
    private final AddressRepository addressRepository;
-    private final ClientAddressRepositoryWrapper clientAddressRepositoryWrapper;
    private final AddressCommandFromApiJsonDeserializer fromApiJsonDeserializer;


We said no manual JSON de-/serialization? Why would we need this one here? Please make sure that we don't use this and find a solution that works out of the box with Jackson... and I don't mean that you use ObjectMapper or similar here... just plain Java POJOs, nothing else.

vidakovic · 2026-07-07T16:02:42Z

-    }
-
    @Override
    public CommandProcessingResult addNewClientAddress(final Client client, final JsonCommand command) {


CommandProcessingResult belongs to the legacy command processing. Why is this still here when we migrate things to new command processing? In other words: a proper XXXResponse or XXXData POJO would be expected here.

vidakovic · 2026-07-07T16:05:26Z

+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class AddressCreateCommandHandlerTest {


Not sure if this adds a lot of value here (pretty much everything is a mock...), but let's leave it. I didn't define any specific unit testing strategy for the new command processing, because our main tests are the integration tests... and they cover the command handlers too.

vidakovic · 2026-07-07T16:05:46Z

+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class AddressUpdateCommandHandlerTest {


See previous comment on tests.

vidakovic · 2026-07-07T16:07:32Z

@@ -265,14 +264,14 @@ public static PostClientsResponse createClient(final RequestSpecification reques
    // Example: org.apache.fineract.integrationtests.common.loans.LoanTransactionHelper.disburseLoan(java.lang.Long,
    // org.apache.fineract.client.models.PostLoansLoanIdRequest)
    @Deprecated(forRemoval = true)
-    public static PostClientClientIdAddressesResponse createClientAddress(final RequestSpecification requestSpec,
+    public static HashMap<String, Object> createClientAddress(final RequestSpecification requestSpec,


Why do we suddenly lose the type information here? This looks like a regression and indicates that OpenAPI doesn't have enough information available to determine the proper Java type... which then falls back on HashMap. We should fix this.

vidakovic · 2026-07-07T16:08:01Z

                .stateProvinceId(Long.valueOf(stateId)).isActive(addressIsActive);
-        PostClientClientIdAddressesResponse response = ClientHelper.createClientAddress(requestSpec, responseSpec, clientId.longValue(),
-                addressTypeId, request);
+        HashMap<String, Object> response = ClientHelper.createClientAddress(requestSpec, responseSpec, clientId.longValue(), addressTypeId,


See my previous comment on OpenAPI type information.

nidhiii128 added 2 commits June 30, 2026 12:37

FINERACT-2293: New command processing - create client address

d064384

FINERACT-2293: New command processing - update client address

d94c4d0

Aman-Mittal assigned vidakovic Jul 3, 2026

Aman-Mittal added the java Pull requests that update Java code label Jul 3, 2026

vidakovic reviewed Jul 7, 2026

View reviewed changes

vidakovic mentioned this pull request Jul 7, 2026

FINERACT-2293: New command processing - create client address #6049

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

FINERACT-2293: update client address#6085

FINERACT-2293: update client address#6085
nidhiii128 wants to merge 2 commits into
apache:developfrom
nidhiii128:FINERACT-2293-update-client-address

nidhiii128 commented Jul 3, 2026

Uh oh!

vidakovic Jul 7, 2026

Uh oh!

vidakovic Jul 7, 2026

Uh oh!

vidakovic Jul 7, 2026

Uh oh!

vidakovic Jul 7, 2026

Uh oh!

vidakovic Jul 7, 2026

Uh oh!

vidakovic Jul 7, 2026

Uh oh!

vidakovic Jul 7, 2026

Uh oh!

vidakovic Jul 7, 2026

Uh oh!

vidakovic Jul 7, 2026

Uh oh!

vidakovic Jul 7, 2026

Uh oh!

vidakovic Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

nidhiii128 commented Jul 3, 2026

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants