FINERACT-2181: Moneyhelper multitenant configuration fix - #4861

budaidev · 2025-07-15T07:19:10Z

Description

There were possible rounding issue during the multitenant configuration, when different tenants use different rounding modes. This fix targets to fix that

Ignore if these details are present on the associated Apache Fineract JIRA ticket.

Checklist

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

Write the commit message as per https://github.com/apache/fineract/#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 for verifying the changes made.
Follow coding conventions at 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
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

galovics · 2025-07-15T19:05:21Z

+        // Fallback for cases where tenant context is not available
+        return "default";


I'd really not hardcode something like this. You can't be sure that there will be a default tenant in the system.

Throw an exception if we think this is an invalid case rather than hiding the problem.

adamsaghy · 2025-07-16T10:17:03Z

+    private static final ConcurrentHashMap<String, MathContext> mathContextCache = new ConcurrentHashMap<>();
    public static final int PRECISION = 19;

    private static ConfigurationDomainService staticConfigurationDomainService;


I was wondering whether we could remove the ConfigurationDomainService all together and remove @Component also.

This could be a utility class which is instantiated with values during the startup:

Not managed bean

No any injections

Value holder only

Once instance got started, for each tenants automatically we set the value

What do you think?

But if we setup the values on startup, the global configuration gets kinda meaningless, doesn't it? Its whole purpose is to be able to dynamically override values in fineract without a redeployment/restart.

rounding-mode is a trap door configuration, you are not allowed to change once it was set.

Agreed, I think a validation could be useful for that use case. Changing the rounding would cause troubles for previous calculation consistency

good, let's proceed with it then as Adam suggested. Although please put a comment there so anybody coming after us knows why it's only fetched once at startup.

adamsaghy · 2025-07-17T14:33:22Z

@budaidev

/home/runner/work/fineract/fineract/fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/api/InternalConfigurationsApiResource.java:34: error: cannot find symbol
import org.apache.fineract.organisation.monetary.service.MoneyHelperInitializationService;
                                                        ^
  symbol:   class MoneyHelperInitializationService
  location: package org.apache.fineract.organisation.monetary.service
/home/runner/work/fineract/fineract/fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/api/InternalConfigurationsApiResource.java:47: error: cannot find symbol
    private final MoneyHelperInitializationService moneyHelperInitializationService;
                  ^
  symbol:   class MoneyHelperInitializationService
  location: class InternalConfigurationsApiResource
/home/runner/work/fineract/fineract/fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/api/InternalConfigurationsApiResource.java:42: error: cannot find symbol
@RequiredArgsConstructor
^
  symbol:   class MoneyHelperInitializationService
  location: class InternalConfigurationsApiResource
3 errors
2 warnings

adamsaghy · 2025-07-21T09:16:50Z

-    private static RoundingMode roundingMode = null;
-    private static MathContext mathContext;
    public static final int PRECISION = 19;
+    private static final RoundingMode DEFAULT_ROUNDING_MODE = RoundingMode.HALF_EVEN;


I am not a big fan of hardcoded "default" values... I would prefer throwing error if it was not set, so it will be blocker if someone tries something nasty!

adamsaghy · 2025-07-21T09:18:14Z

+                return roundingModeProperty.getValue().intValue();
+            }
+        } catch (Exception e) {
+            log.warn("Failed to read rounding mode from configuration: {}", e.getMessage());


I am not a big fan of swallowing this error...

adamsaghy · 2025-07-21T09:19:27Z

+        }
+
+        // Default to HALF_UP if configuration is not available
+        int defaultRoundingMode = RoundingMode.HALF_UP.ordinal();


I dont think we should go with default, fallback value, but even if we would, it is inconsistent to use HALF-UP here and HALF-EVEN in the MoneyHelper... :/

If we are unable to fetch the details of it, then let it fails!

adamsaghy · 2025-07-21T09:27:15Z

+            }
+
+        } catch (Exception e) {
+            log.error("Critical error during MoneyHelper initialization for all tenants", e);


Dont swallow the issue...let it fails!

adamsaghy · 2025-07-21T09:27:33Z

+            }
+
+        } catch (Exception e) {
+            log.error("Critical error during MoneyHelper initialization for all tenants", e);


Dont swallow the issue...let it fails!

adamsaghy · 2025-07-21T09:27:44Z

+
+                } catch (Exception e) {
+                    failureCount++;
+                    log.error("Failed to initialize MoneyHelper for tenant '{}'", tenant.getTenantIdentifier(), e);


Dont swallow the issue...let it fails!

…o be a utility class

galovics requested changes Jul 15, 2025

View reviewed changes

budaidev force-pushed the FINERACT-2181/money-helper-tenant-refactor branch 2 times, most recently from 6cd7598 to a024c29 Compare July 16, 2025 08:17

adamsaghy marked this pull request as ready for review July 16, 2025 10:11

adamsaghy reviewed Jul 16, 2025

View reviewed changes

budaidev force-pushed the FINERACT-2181/money-helper-tenant-refactor branch from 91432ce to 9650d6e Compare July 17, 2025 09:33

budaidev force-pushed the FINERACT-2181/money-helper-tenant-refactor branch 4 times, most recently from ed4bcec to db17169 Compare July 18, 2025 19:43

budaidev requested review from adamsaghy and galovics July 20, 2025 19:29

budaidev force-pushed the FINERACT-2181/money-helper-tenant-refactor branch from db17169 to b7c3111 Compare July 20, 2025 19:31

adamsaghy reviewed Jul 21, 2025

View reviewed changes

FINERACT-2181: Moneyhelper multitenant configuration fix, rework it t…

dcd3cf6

…o be a utility class

adamsaghy force-pushed the FINERACT-2181/money-helper-tenant-refactor branch from b7c3111 to d42ffb7 Compare July 21, 2025 12:53

FINERACT-2181: Money helper exception handling

88596e2

adamsaghy force-pushed the FINERACT-2181/money-helper-tenant-refactor branch from d42ffb7 to 88596e2 Compare July 21, 2025 15:11

adamsaghy merged commit 5940955 into apache:develop Jul 21, 2025

adamsaghy deleted the FINERACT-2181/money-helper-tenant-refactor branch July 21, 2025 16:03

		// Fallback for cases where tenant context is not available
		return "default";

Uh oh!

Conversation

budaidev commented Jul 15, 2025

Description

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!

adamsaghy commented Jul 17, 2025

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