Skip to content

Conversation

budaidev
Copy link
Contributor

@budaidev budaidev commented Oct 6, 2025

Description

Describe the changes made and why they were made.

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.

@budaidev budaidev force-pushed the FINERACT-2380/create-feign-client branch from a1ab808 to 06c3fc3 Compare October 7, 2025 12:52
@budaidev budaidev marked this pull request as ready for review October 7, 2025 13:01

doFirst {
delete fileTree(tempDir) {
include "src/main/java/org/apache/fineract/client/auth/OAuthOkHttpClient.java"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

into targetDir
filter { line ->
line
.replaceAll("import org\\.joda\\.time\\.\\*;", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, we don't need to support java 8 anyway

Comment on lines 149 to 159
java {
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
}

tasks.withType(JavaCompile).configureEach {
options.encoding = 'UTF-8'
options.compilerArgs << '-parameters'
options.errorprone {
excludedPaths = '.*/build/generated/java/src/main/java/.*'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

* Swagger YAML generation from the JAX RS and OpenAPI annotation to have the correct notation for binary document files
* and images, then this could be removed again.
*/
public interface DocumentsApiFixed {
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed duplicate

* in DefaultApi (see <a href="https://issues.apache.org/jira/browse/FINERACT-1222">FINERACT-1222</a>), but fixed for
* bugs in the code generation (see <a href="https://issues.apache.org/jira/browse/FINERACT-1227">FINERACT-1227</a>).
*/
public interface ImagesApi {
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed duplicate

import java.util.Map;
import org.apache.fineract.client.models.RunReportsResponse;

public interface RunReportsApi {
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed duplicate

*/
@Slf4j
@Getter
public class CallFailedRuntimeException extends RuntimeException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed duplicate

/**
* Extension methods for Feign calls. This class is recommended to be statically imported.
*/
public final class FeignCalls {
Copy link
Contributor

@adamsaghy adamsaghy Oct 8, 2025

Choose a reason for hiding this comment

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

Duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed duplicate

* {@link DocumentsApiFixed#updateDocument(String, Long, Long, File, String, String)} and
* {@link ImagesApi#create(String, Long, File)} and {@link ImagesApi#update(String, Long, File)}.
*/
public final class Parts {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, removed it

private static final long serialVersionUID = 141481953116476081L;

@Column(name = CREATED_BY_DB_FIELD, updatable = false, nullable = false)
@Setter(onMethod_ = @Override)
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to remove these?

Comment on lines +92 to +101
String waitForFineractTimeoutSeconds = project.findProperty('waitForFineractTimeoutSeconds') ?: '600'
String waitForFineractCommand = "timeout ${waitForFineractTimeoutSeconds} bash -c 'until curl -k -s --fail \"https://localhost:8443/fineract-provider/actuator/health\" > /dev/null; do sleep 5; echo \"Waiting for Fineract startup...\"; done'"

tasks.register('waitForFineract') {
doLast {
exec {
commandLine 'bash', '-lc', waitForFineractCommand
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For integration tests we need to wait for the cargo instance to be start, if it is not included it will send the request before the service started. The Retrofit based client worked because it kept retrying

Copy link
Contributor

Choose a reason for hiding this comment

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

This would not work if someone tries to run it on windows, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have something like this instead?

import java.net.HttpURLConnection
import java.net.URL

tasks.register('waitForFineract') {
    doLast {
        int timeoutSeconds = (project.findProperty('waitForFineractTimeoutSeconds') ?: '600') as int
        int waited = 0
        int interval = 5

        URL url = new URL("https://localhost:8443/fineract-provider/actuator/health")
        println "Waiting for Fineract startup (timeout: ${timeoutSeconds}s)..."

        while (waited < timeoutSeconds) {
            try {
                HttpURLConnection connection = (HttpURLConnection) url.openConnection()
                connection.setConnectTimeout(2000)
                connection.setReadTimeout(2000)
                connection.setRequestMethod("GET")
                int responseCode = connection.getResponseCode()
                if (responseCode == 200) {
                    println "Fineract is up!"
                    return
                }
            } catch (Exception ignored) { }
            sleep(interval * 1000)
            waited += interval
            println "Still waiting..."
        }

        throw new GradleException("Fineract did not start within ${timeoutSeconds} seconds")
    }
}

// Retrofit client for existing tests
testImplementation project(path: ':fineract-client', configuration: 'runtimeElements')

// Feign client - exclude services to avoid conflicts with Retrofit client
Copy link
Contributor

Choose a reason for hiding this comment

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

if you exclude the generated services, how can any of the integration tests using this client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I clarified the comment we don't actually excluded it

@budaidev budaidev force-pushed the FINERACT-2380/create-feign-client branch 7 times, most recently from 1dfd973 to 860fce4 Compare October 10, 2025 13:37
@budaidev budaidev force-pushed the FINERACT-2380/create-feign-client branch from 860fce4 to 5aa67c8 Compare October 13, 2025 16:13
@budaidev budaidev requested a review from adamsaghy October 15, 2025 13:03
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.

LGTM

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants