Skip to content

FINERACT-2217: Replace 500 by meaningful error when request requires c… #4948

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

FINERACT-2217: Replace 500 by meaningful error when request requires c… #4948

wants to merge 1 commit into from

Conversation

javamak
Copy link
Contributor

@javamak javamak commented Aug 12, 2025 •
edited
Loading

Description

Fix to return a proper error message when S3 is not configured and a report request requires upload to S3.

(https://issues.apache.org/jira/browse/FINERACT-2217).

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.

@javamak javamak changed the title Draft: FINERACT-821: Replace 500 by meaningful error when request requires c… FINERACT-821: Replace 500 by meaningful error when request requires c… Aug 12, 2025
@javamak javamak changed the title FINERACT-821: Replace 500 by meaningful error when request requires c… FINERACT-2217: Replace 500 by meaningful error when request requires c… Aug 12, 2025
@adamsaghy
Copy link
Contributor

@javamak Please squash your commits.

adamsaghy
adamsaghy reviewed Aug 13, 2025
View reviewed changes
...org/apache/fineract/infrastructure/dataqueries/service/DatatableReportingProcessService.java Outdated
@@ -59,7 +60,8 @@ public Response processRequest(String reportName, MultivaluedMap<String, String>
final String parameterTypeValue = ApiParameterHelper.parameterType(queryParams) ? "parameter" : "report";
final Map<String, String> reportParams = getReportParams(queryParams);
ResponseHolder response = findReportExportService(exportMode) //
.orElseThrow(() -> new IllegalArgumentException("Unsupported export target: " + exportMode)) //
.orElseThrow(() -> new PlatformInternalServerException("error.msg.report.export.mode.unavailable",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please return with GeneralPlatformDomainRuleException instead and dont forget to attach test cases which ensures, the correct behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception changed to GeneralPlatformDomainRuleException and test cases added. I was not sure if this is the correct exception as it is converted to 403 which in my understanding used for Auth.

adamsaghy
adamsaghy requested changes Aug 13, 2025
View reviewed changes
Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review my comments!

@javamak
Copy link
Contributor Author

javamak commented Aug 14, 2025

Review comments fixed. Please check

@adamsaghy
Copy link
Contributor 

FAILURE: Build failed with an exception.
gradle/actions: Writing build results to /home/runner/work/_temp/.gradle-actions/build-results/__run-1755198045168.json


* What went wrong:
Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.
Execution failed for task ':rat'.

> A failure occurred while executing org.nosphere.apache.rat.RatWork
You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
   > Apache Rat audit failure - 1 unapproved license

adamsaghy
adamsaghy reviewed Aug 15, 2025
View reviewed changes
...apache/fineract/infrastructure/dataqueries/service/DatatableReportingProcessServiceTest.java
@@ -0,0 +1,61 @@
package org.apache.fineract.infrastructure.dataqueries.service;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Licence header is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@javamak javamak force-pushed the FINERACT-2217 branch 2 times, most recently from a9a51d7 to b4cc96b Compare August 19, 2025 17:51
@javamak javamak requested a review from adamsaghy August 19, 2025 17:54
@javamak
Copy link
Contributor Author

javamak commented Aug 25, 2025

@adamsaghy , Could you please review this and let me know if anything requires fixing.

adamsaghy
adamsaghy reviewed Aug 28, 2025
View reviewed changes
...apache/fineract/infrastructure/dataqueries/service/DatatableReportingProcessServiceTest.java Outdated
queryParams.put("R_officeId", List.of("2"));
queryParams.put("exportS3", List.of("true"));

assertThrows(GeneralPlatformDomainRuleException.class,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the content of GeneralPlatformDomainRuleException and whether it was thrown with this code:
error.msg.report.export.mode.unavailable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertions added to validate the exception content.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamsaghy adamsaghy adamsaghy requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@javamak @adamsaghy