FINERACT-2181: Refactor ResultsetColumnHeaderData using Lombok and modern java new features #4307

a7med3del1973 · 2025-02-06T05:10:01Z

Description

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.

oluexpert99 · 2025-02-06T10:09:00Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

+        return ResultsetColumnHeaderData.builder()
+                .columnName(columnName)
+                .columnType(JdbcJavaType.getByTypeName(dialect, adjustColumnType(columnType), true))
+                .columnLength(null)


why set null? we could omit this in the builder rather than set null

Same goes for primitive boolean types. It is nice to explicitly set, but in the other hand it is unnecessary

adamsaghy · 2025-02-06T10:09:39Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

@@ -6,9 +6,9 @@
 * to you under the Apache License, Version 2.0 (the
 * "License"); you may not use this file except in compliance
 * with the License. You may obtain a copy of the License at
- *
+ * <p>


Please do not change the licence header

oluexpert99 · 2025-02-06T10:11:43Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

-        }
+        if (columnType.isTextType()) return DisplayType.TEXT;
+        if (columnType.isStringType()) return DisplayType.STRING;
+        if (columnType.isAnyIntegerType()) return DisplayType.INTEGER;


a switch or a map will be better in this case

@oluexpert99
Thanks for your response .. i noted your comments then refactored the code according to yours and waiting for feedback

adamsaghy · 2025-02-06T10:11:57Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

-        return columnValues != null && !columnValues.isEmpty();
-    }
-
-    public boolean isColumnValueAllowed(final String match) {


This is used in the SeachUtil class... i am not sure why you removed it....

adamsaghy · 2025-02-06T10:12:09Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

-        return false;
-    }
-
-    public boolean isColumnCodeAllowed(final Integer match) {


Same here...

adamsaghy · 2025-02-06T10:13:06Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

-        return this.columnName;
-    }
-
-    public boolean isNamed(final String columnName) {


This methid is used in many classes.

@a7med3del1973 could you please review the comment?

adamsaghy

Kindly review the comments!

a7med3del1973 · 2025-02-09T07:00:20Z

Kindly review the comments!

@adamsaghy
Thanks for your review .. your comments are noted then refactored the code now waiting your feedback

adamsaghy · 2025-02-09T11:11:27Z

@a7med3del1973 please squash your commits!

adamsaghy · 2025-02-09T11:11:43Z

@a7med3del1973 please squash your commits!

a7med3del1973 · 2025-02-10T11:27:39Z

@adamsaghy squash done

a7med3del1973 · 2025-02-10T13:29:26Z

@adamsaghy
@oluexpert99
if your think something should be modified please let me know.

adamsaghy · 2025-02-11T18:35:49Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

 public final class ResultsetColumnHeaderData implements Serializable {

    private final String columnName;
    private JdbcJavaType columnType;
    private final Long columnLength;
    private final DisplayType columnDisplayType;
+    @Getter(AccessLevel.NONE)


We dont need to restricted getter for these fields...

adamsaghy · 2025-02-11T18:36:30Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

-        return null;
+        Map<Predicate<JdbcJavaType>, DisplayType> typeMap = new LinkedHashMap<>();
+
+        typeMap.put(JdbcJavaType::isTextType, DisplayType.TEXT);


Using switch-case would be better...

galovics · 2025-02-12T14:37:35Z

@a7med3del1973 what's the purpose here? are you familiar with Java and have an IDE set up properly? I really feel like you're just starting out hence the "used in SearchUtil" comments in the codebase. Also, the PR builds are failing, probably due to formatting issues (haven't checked).

a7med3del1973 · 2025-02-12T16:01:39Z

@galovics Thanks for your review
Added this comments just for clearity. Your comment noted and working on it.

adamsaghy · 2025-02-24T09:52:14Z

@a7med3del1973 Kindly fix the failing checks:

> Task :fineract-core:modernizer
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:225:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:226:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:227:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:228:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:229:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:230:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:231:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:232:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:233:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:234:9: 'if' construct must use '{}'s. [NeedBraces]

Execution failed for task ':fineract-core:checkstyleMain'.
> A failure occurred while executing org.gradle.api.plugins.quality.internal.CheckstyleAction
   > Checkstyle rule violations were found. See the report at: file:///home/runner/work/fineract/fineract/fineract-core/build/reports/checkstyle/main.html
     Checkstyle files with violations: 1
     Checkstyle violations by severity: [error:10]

adamsaghy · 2025-02-27T09:26:21Z

@a7med3del1973 Please make sure the PR title and commit message are following the required naming conventions: "FINERACT-XXXX: short description"

adamsaghy · 2025-03-03T16:02:43Z

@a7med3del1973 FINERACT-2081 was a story to cover minor changes in 1.11. Since this version of release process started, we cannot use this story anymore. Kindly use the https://issues.apache.org/jira/browse/FINERACT-2181 from now on for minor changes which will be part of 1.12.

a7med3del1973 · 2025-03-03T16:50:50Z

@a7med3del1973 FINERACT-2081 was a story to cover minor changes in 1.11. Since this version of release process started, we cannot use this story anymore. Kindly use the https://issues.apache.org/jira/browse/FINERACT-2181 from now on for minor changes which will be part of 1.12.

Changes done.

adamsaghy · 2025-03-05T22:28:31Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

@@ -239,40 +206,58 @@ private DisplayType calcDisplayType() {
    }

    public static DisplayType calcColumnDisplayType(JdbcJavaType columnType) {
+        return switch (getJdbcType(columnType)) {


I see no any value of this changes....
If you want to change the IF conditions to switch fine... but why to map JdbcJavaType to String and from String to DisplayType?

Map JdbcJavaType to DisplayType with mapper if you want...

I see no any value of this changes.... If you want to change the IF conditions to switch fine... but why to map JdbcJavaType to String and from String to DisplayType?

Map JdbcJavaType to DisplayType with mapper if you want...

Got it your opinion
Thinked my change will make it easy for add new property
above i already used mapper you told me switch case will be better.

adamsaghy

See my review...

…n Java new features

oluexpert99 reviewed Feb 6, 2025

View reviewed changes

adamsaghy reviewed Feb 6, 2025

View reviewed changes

oluexpert99 reviewed Feb 6, 2025

View reviewed changes

adamsaghy reviewed Feb 6, 2025

View reviewed changes

adamsaghy requested changes Feb 6, 2025

View reviewed changes

a7med3del1973 requested a review from oluexpert99 February 9, 2025 06:56

a7med3del1973 requested a review from adamsaghy February 9, 2025 07:00

a7med3del1973 force-pushed the lombok-integration branch from b68b45b to e384bfa Compare February 10, 2025 11:17

a7med3del1973 force-pushed the lombok-integration branch 3 times, most recently from 2b77e08 to 41b4859 Compare February 10, 2025 13:25

adamsaghy reviewed Feb 11, 2025

View reviewed changes

a7med3del1973 force-pushed the lombok-integration branch from 41b4859 to c78a15c Compare February 12, 2025 10:42

a7med3del1973 requested a review from adamsaghy February 12, 2025 10:43

a7med3del1973 force-pushed the lombok-integration branch from c78a15c to 616da39 Compare February 19, 2025 19:22

a7med3del1973 force-pushed the lombok-integration branch from 616da39 to d529545 Compare February 26, 2025 01:43

a7med3del1973 changed the title ~~Refactor ResultsetColumnHeaderData to Use Lombok Annotations for Boilerplate Reduction~~ FINERACT-2081: Refactor ResultsetColumnHeaderData to Use Lombok Annotations for Boilerplate Reduction Mar 3, 2025

a7med3del1973 changed the title ~~FINERACT-2081: Refactor ResultsetColumnHeaderData to Use Lombok Annotations for Boilerplate Reduction~~ FINERACT-2081: Refactor ResultsetColumnHeaderData using Lombok and modern java new features Mar 3, 2025

a7med3del1973 force-pushed the lombok-integration branch from d529545 to 0646edd Compare March 3, 2025 04:18

a7med3del1973 force-pushed the lombok-integration branch from 0646edd to 5995e49 Compare March 3, 2025 16:49

a7med3del1973 changed the title ~~FINERACT-2081: Refactor ResultsetColumnHeaderData using Lombok and modern java new features~~ FINERACT-2181: Refactor ResultsetColumnHeaderData using Lombok and modern java new features Mar 3, 2025

a7med3del1973 force-pushed the lombok-integration branch from 5995e49 to 9813a07 Compare March 4, 2025 15:39

adamsaghy reviewed Mar 5, 2025

View reviewed changes

adamsaghy requested changes Mar 5, 2025

View reviewed changes

FINERACT-2181: Refactor ResultColumnHeaderData using Lombok and moder…

609b152

…n Java new features

a7med3del1973 force-pushed the lombok-integration branch from 9813a07 to 609b152 Compare March 6, 2025 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FINERACT-2181: Refactor ResultsetColumnHeaderData using Lombok and modern java new features #4307

FINERACT-2181: Refactor ResultsetColumnHeaderData using Lombok and modern java new features #4307

a7med3del1973 commented Feb 6, 2025 •

edited

Loading

oluexpert99 Feb 6, 2025

adamsaghy Feb 6, 2025

adamsaghy Feb 6, 2025

oluexpert99 Feb 6, 2025

a7med3del1973 Feb 9, 2025

adamsaghy Feb 6, 2025

adamsaghy Feb 6, 2025

adamsaghy Feb 6, 2025

IOhacker Mar 3, 2025

adamsaghy left a comment

a7med3del1973 commented Feb 9, 2025

adamsaghy commented Feb 9, 2025

adamsaghy commented Feb 9, 2025

a7med3del1973 commented Feb 10, 2025 •

edited

Loading

a7med3del1973 commented Feb 10, 2025

adamsaghy Feb 11, 2025

adamsaghy Feb 11, 2025

galovics commented Feb 12, 2025

a7med3del1973 commented Feb 12, 2025

adamsaghy commented Feb 24, 2025

adamsaghy commented Feb 27, 2025

adamsaghy commented Mar 3, 2025

a7med3del1973 commented Mar 3, 2025

adamsaghy Mar 5, 2025

a7med3del1973 Mar 6, 2025 •

edited

Loading

adamsaghy left a comment

FINERACT-2181: Refactor ResultsetColumnHeaderData using Lombok and modern java new features #4307

Are you sure you want to change the base?

FINERACT-2181: Refactor ResultsetColumnHeaderData using Lombok and modern java new features #4307

Conversation

a7med3del1973 commented Feb 6, 2025 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsaghy left a comment

Choose a reason for hiding this comment

a7med3del1973 commented Feb 9, 2025

adamsaghy commented Feb 9, 2025

adamsaghy commented Feb 9, 2025

a7med3del1973 commented Feb 10, 2025 • edited Loading

a7med3del1973 commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galovics commented Feb 12, 2025

a7med3del1973 commented Feb 12, 2025

adamsaghy commented Feb 24, 2025

adamsaghy commented Feb 27, 2025

adamsaghy commented Mar 3, 2025

a7med3del1973 commented Mar 3, 2025

Choose a reason for hiding this comment

a7med3del1973 Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

adamsaghy left a comment

Choose a reason for hiding this comment

a7med3del1973 commented Feb 6, 2025 •

edited

Loading

a7med3del1973 commented Feb 10, 2025 •

edited

Loading

a7med3del1973 Mar 6, 2025 •

edited

Loading