Skip to content
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.

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

FINERACT-2110: Introduce type-safety API for commands (Note API) #3994

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Zeyad2003
Copy link
Contributor

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.

@vidakovic vidakovic self-requested a review July 26, 2024 06:04
@Zeyad2003 Zeyad2003 force-pushed the FINERACT-2021/refactor_Write_Methods branch 2 times, most recently from 1381b9e to 885a2d8 Compare July 26, 2024 06:23
private final Long commandId;
private final Long resourceId;
private final Long subresourceId;
private final Long groupId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these IDs necessary here? This is mimicking exactly the same implementation as the current production one... in other words: if you have a command that has IDs belonging to different entities then it's not really type safe.

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.
Now we depend on resourceId and subresourceId only for representing IDs

custom/v3/commands/provider/dependencies.gradle Outdated Show resolved Hide resolved

public interface TypedCommandProcessingService {

<T> CommandProcessingResult executeCommand(CommandTypedWrapper<T> wrapper);
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the type safety for the "CommandProcessingResult"?

import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

@RestController
Copy link
Contributor

Choose a reason for hiding this comment

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

You are mixing read and write here... read related stuff is one PR, write related stuff is a separate PR... this cannot be approved, because this code is duplicated in 2 PRs!

Copy link
Contributor

Choose a reason for hiding this comment

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

A cleaner approach would be to have 1 controller for read and a separate one for write...

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 can split the controller into 2 files, but the Commands' PR depends on Queries PR (same structure, share packages, and dependencies ...etc).
If the #3915 got merged then this one can be rebased with it seamlessly (no duplication).

Am I missing something?

@@ -44,6 +44,9 @@ fineract.tenant.read-only-password=${FINERACT_DEFAULT_TENANTDB_RO_PWD:}
fineract.tenant.read-only-parameters=${FINERACT_DEFAULT_TENANTDB_RO_CONN_PARAMS:}
fineract.tenant.read-only-name=${FINERACT_DEFAULT_TENANTDB_RO_NAME:}

# backwards compatibility for 'org.apache.fineract.infrastructure.security.filter.TenantAwareBasicAuthenticationFilter'
baseUrl=https://localhost:8443${FINERACT_SERVER_SERVLET_CONTEXT_PATH:/fineract-provider}
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to use a variable for host and port... those are available already with Spring Boot (just don't remember the names). Should be used here instead of hardcoded values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to use a variable for host and port... those are available already with Spring Boot (just don't remember the names). Should be used here instead of hardcoded values.

How to use spring built-in vars?
I tried this but didn't work:

# FINERACT-914
server.forward-headers-strategy=framework
server.port=${FINERACT_SERVER_PORT:8443}
server.address=${FINERACT_SERVER_ADDRESS:localhost}
server.servlet.context-path=${FINERACT_SERVER_SERVLET_CONTEXT_PATH:/fineract-provider}
server.compression.enabled=${FINERACT_SERVER_COMPRESSION_ENABLED:true}

# backwards compatibility for 'org.apache.fineract.infrastructure.security.filter.TenantAwareBasicAuthenticationFilter
baseUrl=https://${server.address}:${server.port}${server.servlet.context-path}

and the checks failed as following:

Run curl -f -k --retry 10 --retry-connrefused --connect-timeout 30 --retry-delay 30 https://localhost:8443/fineract-provider/actuator/health
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (35) error:0A000[12](https://github.com/Zeyad2003/fineract/actions/runs/10129914159/job/28010527803#step:8:13)6:SSL routines::unexpected eof while reading
Error: Process completed with exit code 35.

any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This approach has passed the checks:

server.forward-headers-strategy=framework
server.port=${FINERACT_SERVER_PORT:8443}
server.domain=${FINERACT_SERVER_DOMAIN:localhost}
server.servlet.context-path=${FINERACT_SERVER_SERVLET_CONTEXT_PATH:/fineract-provider}
server.compression.enabled=${FINERACT_SERVER_COMPRESSION_ENABLED:true}

# backwards compatibility for 'org.apache.fineract.infrastructure.security.filter.TenantAwareBasicAuthenticationFilter'
baseUrl=https://${server.domain}:${server.port}${server.servlet.context-path}

It appears that the server.address property causes some issues. The exact reason is ambiguous to me.

@Zeyad2003 Zeyad2003 changed the title Introduce type-safety API for commands FINERACT-2110: Introduce type-safety API for commands (Note API) Jul 27, 2024
@Zeyad2003 Zeyad2003 force-pushed the FINERACT-2021/refactor_Write_Methods branch 7 times, most recently from 026a952 to 00e499a Compare July 29, 2024 12:15
return new CommandWrapper(officeId, groupId, clientId, loanId, savingsId, actionName, entityName, entityId,
subentityId, href, json, transactionId, productId, null, creditBureauId, organisationCreditBureauId,
jobName, idempotencyKey);
return new CommandWrapper(null, null, null, null, null, actionName, entityName, entityId,
Copy link
Contributor

Choose a reason for hiding this comment

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

No... this whole function is written in a way that goes against all best practices of dependency injection; "ObjectMapper" should be re-used/injected (with all the global configurations)... here we create it with default configurations over and over again... but even without that: this function is acts like copy function... how many times do we use this? My bet is again: once... this programming style is obscuring more things than it saves or helps...

Copy link
Contributor Author

@Zeyad2003 Zeyad2003 Jul 29, 2024

Choose a reason for hiding this comment

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

It's just temporary. Not implemented there to remain.
Actually, I'm breaking some rules to get things to work first.
Then it can be improved or even removed.

@@ -57,7 +47,7 @@ public JsonCommand getJsonCommand() {
}

return new JsonCommand(null, json, null, null, entityName, resourceId,
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a "CommandWrapper" and a "JsonCommand"... why? Can't we just express what we need in one class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've thought about that, there's no major differences. (just action & entity names).
Thanks for mentioning it!

}

return resourceDetails.toBuilder().entityName(resourceNameForPermissions).build();
String resourceNameForPermissions = switch (type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Without even reading more details: very likely the wrong place for ANYTHING permission related! Note: we don't need to squeeze every concept into one class... we are going to run out of space if we create another class... remidner: this belongs to Spring Security (remember when we talked about "Access Desicion Voters"?).

@Zeyad2003 Zeyad2003 force-pushed the FINERACT-2021/refactor_Write_Methods branch 12 times, most recently from 0b7d73d to 654d70d Compare August 5, 2024 15:55
@Zeyad2003 Zeyad2003 force-pushed the FINERACT-2021/refactor_Write_Methods branch 3 times, most recently from 831d1e7 to e6c3504 Compare August 9, 2024 07:27
@Zeyad2003 Zeyad2003 force-pushed the FINERACT-2021/refactor_Write_Methods branch 2 times, most recently from 2514be6 to dd88e8c Compare August 27, 2024 20:39
@Zeyad2003 Zeyad2003 force-pushed the FINERACT-2021/refactor_Write_Methods branch 12 times, most recently from 24959a7 to 6d617fe Compare September 1, 2024 07:12
@Zeyad2003 Zeyad2003 force-pushed the FINERACT-2021/refactor_Write_Methods branch 2 times, most recently from df0a5a0 to 004e718 Compare September 1, 2024 23:24
@vidakovic
Copy link
Contributor

Please rebase into 1 commit.

<column name="result_status_code" type="INTEGER" remarks="Result status code"/>
<column name="created_date" type="TIMESTAMP WITH TIME ZONE" remarks="Creation date"/>
<column name="processed_at" type="TIMESTAMP WITH TIME ZONE" remarks="Processing timestamp"/>
<column name="request_body" type="TEXT" remarks="Request body"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

First: keep attributes simple... rename to body (we know that it's from a request... and there will never be another body). Alternative name payload.

Copy link
Contributor

@vidakovic vidakovic Sep 5, 2024

Choose a reason for hiding this comment

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

Also: type TEXT is not really the way to go... it should be in the database jsonb and Java side com.fasterxml.jackson.databind.JsonNode. That way we can keep the structure and the parsing is done automatically.

Check https://www.baeldung.com/spring-boot-jpa-storing-postgresql-jsonb section 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I checked it, but couldn't resolve the mapping because the ORM needs to know how to map the attribute to a jsonb column, and the way explaied in the tutorial uses hibernate (which doesn't work with eclipselink).

Copy link
Contributor

Choose a reason for hiding this comment

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

@vidakovic I dont think jsonb would work with mysql or mariadb... isnt it postgres specific?

I reckon we can differentiate json and jsonb based on which engine is used if 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.

@adamsaghy

You're right, jsonb is specific to PostgreSQL, but I think it's possible to configure the column as json for MySQL (and MariaDB) and jsonb for PostgreSQL.

The challenge I'm facing is how to map a jsonb attribute using EclipseLink.

</column>
<column name="command_processing_status" type="VARCHAR(255)" remarks="Command processing status"/>
<column name="command_result" type="TEXT" remarks="Command result"/>
<column name="result_status_code" type="INTEGER" remarks="Result status code"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's name this just status. Simpler and says everything.

<constraints nullable="false" unique="true"/>
</column>
<column name="command_processing_status" type="VARCHAR(255)" remarks="Command processing status"/>
<column name="command_result" type="TEXT" remarks="Command result"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's name this just result. Simpler and says everything.

Copy link
Contributor

@vidakovic vidakovic Sep 5, 2024

Choose a reason for hiding this comment

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

Also: type TEXT is not really the way to go... it should be in the database jsonb and Java side com.fasterxml.jackson.databind.JsonNode. That way we can keep the structure and the parsing is done automatically.

Check https://www.baeldung.com/spring-boot-jpa-storing-postgresql-jsonb section 3.

<column name="request_idempotency_key" type="VARCHAR(50)" remarks="Unique request idempotency key">
<constraints nullable="false" unique="true"/>
</column>
<column name="command_processing_status" type="VARCHAR(255)" remarks="Command processing status"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need yet another status... and in a different type? Let's do either integer or string or enum status...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because they should represent different things.

The integer => Represents the http status code.(200, 201, 403, 404 ...etc)

The enum => Represents the command execution status within the system. (PROCESSED, UNDER_PROCESSING, ERROR ...etc)

}

@Named("mapRequestBody")
default JsonNode mapRequestBody(Object body) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be done by a Spring Converter. See my previous comments above and the Baeldung link. If implemented correctly then this default function is not necessary (Spring Data JPA will automatically take care of it).

@EnableConfigurationProperties({ NoteProperties.class })
@ComponentScan("org.apache.fineract.v3.note")
@ConditionalOnProperty(value = "fineract.note.enabled", havingValue = "true")
public class NoteV3AutoConfiguration {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just NoteAutoConfiguration will do.

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 tried it but there's a conflict between it and the other configuration in the upstream code.

Here's the error logs:

fineract-fineract-1  | org.springframework.beans.factory.BeanDefinitionStoreException: Failed to process import candidates for configuration class [org.apache.fineract.ServerApplication$Configuration]: Annotation-specified bean name 'noteAutoConfiguration' for bean class [org.apache.fineract.portfolio.note.starter.NoteAutoConfiguration] conflicts with existing, non-compatible bean definition of same name and class [org.apache.fineract.v3.note.starter.NoteAutoConfiguration]

…ajor improvements with the request processing flow.

This commit squashes 2 commits (Read & Write) type-safe request

**Read Requests**

FINERACT-2021: refactor the note GET end-points to eliminate manual serialization with GSON.

- Implemented a new custom module for the note API.
- Used Spring Web-MVC annotations instead of JAX-RS.
- Dispensed the redundant query parameters like "fields, prettyPrint ..etc"
- Implemented a new `CommonWebConfiguration` for establishing the security configurations (not complete yet).
- Implemented configuration modules (core, starter, configuration) for configuring the note module.
- Implemented a new `docker-compose-api-v3.yml` configuration file.

**Write Requests**

FINERACT-2110: Add basic structure for the new mirror type-safe API (Commands Proof of Concept)

- Made a unique command (Request & Response) for each http method.
- Define a new way for locating the dedicated handlers.
- Add basic structure for the new mirror type-safe API (Commands).
- Add a new domain module for storing `Command` entity and add a repository for it.
- Re-Implement the logic within the Note Write Service.
- Implement a `ControllerAdvice` class for handling the exceptions that's thrown when working with note API.
Copy link

github-actions bot commented Oct 8, 2024

This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.

@github-actions github-actions bot added the stale label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants