FINERACT-2494: Add unit tests for ApiParameterHelper in fineract-core

[Ambika-Sony]

Description

This PR adds unit test coverage for the ApiParameterHelper utility class in the fineract-core module.

Changes:

Added ApiParameterHelperTest.java to test core methods like extractFieldsForResponseIfProvided.

Verified code formatting using ./gradlew spotlessApply.

Testing:

Successfully ran locally using: ./gradlew :fineract-core:test --tests "org.apache.fineract.infrastructure.core.api.ApiParameterHelperTest"

All tests passed (100% success rate).

Checklist

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

• [ x] Write the commit message as per our guidelines
• [ x] 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.
• [ x] Create/update unit or integration tests for verifying the changes made.
• [ x] 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
• [ x] 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.

[adamsaghy]

@Ambika-Sony Testing 1 method does not provide too much value. Would you consider to test the whole class?

[Ambika-Sony]

@Ambika-Sony Testing 1 method does not provide too much value. Would you consider to test the whole class?

I've Implemented pagination (limit, offset), sorting (orderBy, sortOrder), and association helpers in ApiParameterHelper.

Added 10 unit tests in ApiParameterHelperTest, covering standard scenarios, null/empty inputs, and defensive checks for whitespace handling.

Verified the build locally with ./gradlew :fineract-core:test --tests ApiParameterHelperTest (all 10 passed).

Looking forward to your feedback.

[Ambika-Sony]

@adamsaghy I noticed the Cucumber E2E tests are failing in the CI, but since my changes are strictly limited to the ApiParameterHelper utility and its unit tests (2 files), these failures seem unrelated to my code. I saw your recent email about the E2E framework transition—perhaps these are known issues?

[Aman-Mittal]

Please fix the header it should be exactly same as from other files

[Aman-Mittal]

This will trigger errorProne String[] split = commaSeparatedParameters.split(","); fix this

[Aman-Mittal]

/home/runner/work/fineract/fineract/fineract-validation/src/main/java/org/apache/fineract/validation/constraints/EnumValueValidator.java:33: warning: [StringCaseLocaleUsage] Specify a Locale when calling String#to{Lower,Upper}Case. (Note: there are multiple suggested fixes; the third may be most appropriate if you're dealing with ASCII Strings.)
acceptedValues = Arrays.stream(annotation.enumClass().getEnumConstants()).map(e -> e.name().toLowerCase())
^
(see https://errorprone.info/bugpattern/StringCaseLocaleUsage)
Did you mean 'acceptedValues = Arrays.stream(annotation.enumClass().getEnumConstants()).map(e -> e.name().toLowerCase(Locale.ROOT))' or 'acceptedValues = Arrays.stream(annotation.enumClass().getEnumConstants()).map(e -> e.name().toLowerCase(Locale.getDefault()))' or 'acceptedValues = Arrays.stream(annotation.enumClass().getEnumConstants()).map(e -> Ascii.toLowerCase(e.name()))'?
/home/runner/work/fineract/fineract/fineract-validation/src/main/java/org/apache/fineract/validation/constraints/EnumValueValidator.java:39: warning: [StringCaseLocaleUsage] Specify a Locale when calling String#to{Lower,Upper}Case. (Note: there are multiple suggested fixes; the third may be most appropriate if you're dealing with ASCII Strings.)
return value != null && acceptedValues.contains(value.toLowerCase());
^
(see https://errorprone.info/bugpattern/StringCaseLocaleUsage)
Did you mean 'return value != null && acceptedValues.contains(value.toLowerCase(Locale.ROOT));' or 'return value != null && acceptedValues.contains(value.toLowerCase(Locale.getDefault()));' or 'return value != null && acceptedValues.contains(Ascii.toLowerCase(value));'?
3 warnings
Unable to resolve POM for org.eclipse.platform:org.eclipse.swt:3.124.100

[Aman-Mittal]

@Ambika-Sony try running ./gradlew compileJava for errorProne for more info.

[Ambika-Sony]

@Ambika-Sony try running ./gradlew compileJava for errorProne for more info.

Hi @Aman-Mittal, I've updated ApiParameterHelper.java to resolve the ErrorProne warnings.I replaced all split() regex calls with StringUtils.split() and manual trimming,refined the excludeAssociationsForResponseIfProvided logic to be more robust and Cleaned up imports and verified the build locally with ./gradlew compileJava.
Please let me know if any further changes are needed!

[Ambika-Sony]

Hi @Aman-Mittal, @adamsaghy — All checks are now passing (37/37). I have resolved the ErrorProne issues and verified the logic with the new unit tests. Ready for your final review when you have a moment!

[Aman-Mittal]

This seems duplication

[Aman-Mittal]

The current test coverage is a good start and covers the basic happy paths and invalid number parsing. However, I think we can strengthen this further by adding a few important edge cases to better protect API behavior.

Some scenarios that are currently not covered:

Multiple query parameters with the same key

e.g. ?fields=id,name&fields=description

Same for associations

Ensures proper flattening across multi-valued params.

Empty string values

?fields=

?associations=

Prevents accidental inclusion of empty entries.

Duplicate values

fields=id,name,id

Verifies deduplication behavior since the return type is Set.

Partial pagination parameters

Only offset present

Only limit present

Ensures default behavior is explicitly verified.

Negative pagination values

offset=-1, limit=-10

Clarifies whether negatives are allowed or should fail.

Overflow cases

Very large numeric values exceeding Integer range.

Important defensive test for parsing logic.

Adding these would make the test suite more robust and better document the intended contract of ApiParameterHelper, especially since these helpers are reused across many endpoints.

[Ambika-Sony]

The current test coverage is a good start and covers the basic happy paths and invalid number parsing. However, I think we can strengthen this further by adding a few important edge cases to better protect API behavior.

Some scenarios that are currently not covered:

Multiple query parameters with the same key

e.g. ?fields=id,name&fields=description

Same for associations

Ensures proper flattening across multi-valued params.

Empty string values

?fields=

?associations=

Prevents accidental inclusion of empty entries.

Duplicate values

fields=id,name,id

Verifies deduplication behavior since the return type is Set.

Partial pagination parameters

Only offset present

Only limit present

Ensures default behavior is explicitly verified.

Negative pagination values

offset=-1, limit=-10

Clarifies whether negatives are allowed or should fail.

Overflow cases

Very large numeric values exceeding Integer range.

Important defensive test for parsing logic.

Adding these would make the test suite more robust and better document the intended contract of ApiParameterHelper, especially since these helpers are reused across many endpoints.

Hi @Aman-Mittal,
I’ve updated the PR to address your feedback. The parameter extraction logic has been refactored to use a private helper to remove duplication, and it now properly handles multi-valued parameters. I’ve also expanded the test suite to cover deduplication, blank values, negative pagination, and overflow cases.

Please let me know if anything further should be adjusted.

[Ambika-Sony]

Hi, I've addressed all the edge cases and refactored the logic as discussed. I noticed the workflows are currently awaiting approval to run. Please let me know if you'd like me to adjust anything else once the builds finish.

…

On Thu, Feb 26, 2026 at 9:04 PM Aman-Mittal ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In fineract-core/src/main/java/org/apache/fineract/infrastructure/core/api/ApiParameterHelper.java <#5510 (comment)>: > } } return fields; } public static Set<String> extractAssociationsForResponseIfProvided(final MultivaluedMap<String, String> queryParams) { - Set<String> fields = new HashSet<>(); - String commaSeparatedParameters = ""; - if (queryParams.getFirst("associations") != null) { - commaSeparatedParameters = queryParams.getFirst("associations"); - if (StringUtils.isNotBlank(commaSeparatedParameters)) { - fields = new HashSet<>(Arrays.asList(commaSeparatedParameters.split("\\s*,\\s*"))); // NOSONAR + Objects.requireNonNull(queryParams, "queryParams map cannot be null"); + Set<String> associations = new HashSet<>(); + String commaSeparatedParameters = queryParams.getFirst("associations"); + + if (StringUtils.isNotBlank(commaSeparatedParameters)) { This seems duplication ------------------------------ In fineract-core/src/test/java/org/apache/fineract/infrastructure/core/api/ApiParameterHelperTest.java <#5510 (comment)>: > + */ +package org.apache.fineract.infrastructure.core.api; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import jakarta.ws.rs.core.MultivaluedHashMap; +import jakarta.ws.rs.core.MultivaluedMap; +import java.util.Set; +import org.junit.jupiter.api.Test; + +class ApiParameterHelperTest { + + @test The current test coverage is a good start and covers the basic happy paths and invalid number parsing. However, I think we can strengthen this further by adding a few important edge cases to better protect API behavior. Some scenarios that are currently not covered: Multiple query parameters with the same key e.g. ?fields=id,name&fields=description Same for associations Ensures proper flattening across multi-valued params. Empty string values ?fields= ?associations= Prevents accidental inclusion of empty entries. Duplicate values fields=id,name,id Verifies deduplication behavior since the return type is Set. Partial pagination parameters Only offset present Only limit present Ensures default behavior is explicitly verified. Negative pagination values offset=-1, limit=-10 Clarifies whether negatives are allowed or should fail. Overflow cases Very large numeric values exceeding Integer range. Important defensive test for parsing logic. Adding these would make the test suite more robust and better document the intended contract of ApiParameterHelper, especially since these helpers are reused across many endpoints. — Reply to this email directly, view it on GitHub <#5510 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BIN7S2E4RKJOU4A4TQESRVT4N4HB7AVCNFSM6AAAAACVQUWMQCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTQNRRG42DQOBZG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[adamsaghy]

@Ambika-Sony Please rebase this PR

[Ambika-Sony]

@Ambika-Sony Please rebase this PR

I've rebased against the latest upstream and cleared out that conflict in the integration tests. I also made sure to keep the history clean with a single verified commit. Ready for another look.