Fineract-1910 data table query advanced #3321
Conversation
|
Review only, no tests yet, to be added - tests added |
|
Squash your commits pls and format the PR title properly (FINERACT-1234 Something) |
|
@marta-jankovics please change back the javadoc/comments where you've only changed formatting. It's polluting the PR and makes it hard to review. Also, do we need a 4k line change in this specific PR, just to extend the datatable functionality? Let's make the changes as small as possible unless we have a good reason to change it in its core. Until that, it's way too big to review. |
marta-jankovics
changed the title
|
Sorry I did not know that merge the PRs can not be done with squash and merge here. I will then squash the commits (most of them are merge). About the formatting: I did not change anything, spotless did. I reverted 300+ changes after spotless run but I do not check it again. Would be nice to create a check-style which does not touch the existing code (and does not ruin the formatted enums, does not add empty line after each method start, allows 200 chars line length...) |
| return escape(databaseTypeResolver.databaseType(), arg); | ||
| } | ||
|
|
||
| public static String escape(DatabaseType dialect, String arg) { |
There was a problem hiding this comment.
Why do we need to pass the dialect here but not everywhere else?
There was a problem hiding this comment.
because it is static
There was a problem hiding this comment.
This class is a spring bean, let's not make static methods on it but use it as a proper component.
There was a problem hiding this comment.
I do not agree with your opinion.
There was a problem hiding this comment.
I'd also vote to remove the "static"... we have a lot of "utility" classes that have static methods and are just creating problems down the road (example: just have a look at how a tenant's timezone is resolved). Globally there is also no added value in terms of saving mem or similar (by not instantiating a full object); effectively "@component"s are singletons... and in that form they are composable... with static they are not... there are plenty of examples where this leads to bloated static functions (more than 500 lines of code). The fix is very easy, the class is already instantiated as a singleton, so there's added value by using the static keyword.
There was a problem hiding this comment.
... and on top of that: if we don't declare static then we don't have to pass the dialect parameter... and if we don't need to pass that parameter then we don't need to declare a new function which makes this class more explicit and leaves less to guess for the dev.
There was a problem hiding this comment.
The dialect parameter might not be used for the currently supported dialects but will be needed for the next supported one, that is why this class has a reference to the dialect.
You can not instantiate a component, only from a component (mainly) so if not static then a parameter of the SqlGenerator component for every method in the call chain. That would be another solution to this, right.
| @@ -189,4 +207,49 @@ public String castJson(String sql) { | |||
| throw new IllegalStateException("Database type is not supported for casting to json " + databaseTypeResolver.databaseType()); | |||
| } | |||
| } | |||
|
|
|||
| public static String alias(@NotNull String field, String alias) { | |||
There was a problem hiding this comment.
How are these methods below specific to the database type?
There was a problem hiding this comment.
Just looking at all these functions in general (not only Marta's): this is getting more and more complex... a fight we cannot win.... and in the end we have compatibility for 2 database systems. A lot of work (also future maintenance). Why not hit the breaks here and use e. g. JOOQ. Slowly, but steadily we are recreating it here with less features and less supported databases. For another day to decide, but I think it will be an important improvement.... and all this code can go away.
| return formatValue(databaseTypeResolver.databaseType(), columnType, value); | ||
| } | ||
|
|
||
| public static String formatValue(DatabaseType dialect, JdbcJavaType columnType, String value) { |
There was a problem hiding this comment.
How is this specific again to a dialect?
There was a problem hiding this comment.
Do not understand this question, of course formatting a value is dialect dependent.
There was a problem hiding this comment.
I dont see that, the parameter is not even used.
There was a problem hiding this comment.
Same arguments here, see my previous comments "escape" function.
| @@ -58,10 +58,10 @@ public DatabaseType databaseType() { | |||
| } | |||
|
|
|||
| public boolean isPostgreSQL() { | |||
| return DatabaseType.POSTGRESQL.equals(currentDatabaseType.get()); | |||
| return DatabaseType.POSTGRESQL == currentDatabaseType.get(); | |||
There was a problem hiding this comment.
Why was this necessary? It's not an issue, I'm just curious.
There was a problem hiding this comment.
I would dump these functions too (see my previous comment on DatabaseType). What are we saving here? Again, let's say we add support for 10 more databases...
There was a problem hiding this comment.
I guess my point is: you can iterate over all values of an enum or use it for example in a switch statement... those "is..." functions you can only use in (ugly) if-then-else-if statements.... I think this style has a tendency to bloat code...
There was a problem hiding this comment.
These isX functions are not new and sometimes really useful, when you do not want to know which enum(s) belong to a category.
For identity please see my previous comment.
| } | ||
|
|
||
| public void validateValues(String... values) { | ||
| if (values == null ? paramCount != 0 : (paramCount < 0 ? values.length < -paramCount : values.length != paramCount)) { |
There was a problem hiding this comment.
This condition is hard to understand unfortunately, can't we do it differently?
| * if(loan.isSyncDisbursementWithMeeting()){ // validate actual disbursement date against meeting date | ||
| * CalendarInstance calendarInstance = this.calendarInstanceRepository.findCalendarInstaneByLoanId | ||
| * (loan.getId(), CalendarEntityType.LOANS.getValue()); this.loanEventApiJsonValidator | ||
| * if(loan.isSyncDisbursementWithMeeting()){ // validate actual disbursement date against meeting date CalendarInstance calendarInstance = |
There was a problem hiding this comment.
Not needed. Please revert.
| @@ -48,8 +48,7 @@ | |||
| import org.apache.fineract.portfolio.loanaccount.domain.transactionprocessor.impl.InterestPrincipalPenaltyFeesOrderLoanRepaymentScheduleTransactionProcessor; | |||
|
|
|||
| /** | |||
| * Abstract implementation of {@link LoanRepaymentScheduleTransactionProcessor} which is more convenient for concrete | |||
| * implementations to extend. | |||
| * Abstract implementation of {@link LoanRepaymentScheduleTransactionProcessor} which is more convenient for concrete implementations to extend. | |||
There was a problem hiding this comment.
Not needed. Please revert.
| PagedRequest<AdvancedQueryRequest> queryRequest, @Context final UriInfo uriInfo) { | ||
| final org.springframework.data.domain.Page<JsonObject> result = transactionsSearchServiceImpl.queryAdvanced(savingsId, | ||
| queryRequest); | ||
| return this.toApiJsonSerializer.serializePretty(ApiParameterHelper.prettyPrint(uriInfo.getQueryParameters()), result); |
There was a problem hiding this comment.
As I said for the datatables API, you can go with Page<...> since this is a new API.
There was a problem hiding this comment.
I do not know where you said this already, but this is a v1 API, and I learned that only v2 APIs can go with Object response. Additionally the response here is dynamic.
There was a problem hiding this comment.
The rule is
for V2 it is mandatory to be object response
for V1 new API it is okay to be object response
| @@ -43,7 +43,7 @@ public static class Filters { | |||
| @Data | |||
| public static class RangeFilter<T> { | |||
|
|
|||
| private RangeOperator operator; | |||
| private SqlOperator operator; | |||
There was a problem hiding this comment.
Now this I don't like. We're exposing "SqlOperators" to higher layers (i.e.API) which shouldn't happen.
There was a problem hiding this comment.
I like this. Why can't sql operators be exposed to higher levels??? (Actually it was exposed before but as simple string or RangeOparator)
I was thinking on even using graphQL, but this was not complex enough for that.
| import org.apache.fineract.portfolio.search.data.FilterData; | ||
| import org.springframework.jdbc.support.rowset.SqlRowSet; | ||
|
|
||
| public class SearchUtil { |
There was a problem hiding this comment.
Woah, this class is really really complex and impossible to review. Can we separate pieces out according to responsibilities?
There was a problem hiding this comment.
What do you mean? A util class with 10 static methods, all search related is not complex.
...provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/EntityTables.java
Outdated
Show resolved
Hide resolved
.../org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java
Outdated
Show resolved
Hide resolved
marta-jankovics
force-pushed
the
FINERACT-1910_Data_Table_query_Advanced_marta
branch
3 times, most recently
from
780304c
to
b233f34
Compare
marta-jankovics
force-pushed
the
FINERACT-1910_Data_Table_query_Advanced_marta
branch
from
b233f34
to
68aee23
Compare
There was a problem hiding this comment.
@vidakovic Could you please review org.apache.fineract.infrastructure.security.utils.SQLInjectionValidator
SQL_PATTERN extended by these characters: {,},[,],+
We should support json value in dynamic insert statements.
Please confirm that it is ok from security perspective.
Tried to use your solution from
https://github.com/apache/fineract/blame/1.8.4/fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/SQLInjectionValidator.java
and introduced INJECTION_PATTERNS as black patterns, but ReadWriteNonCoreDataServiceImplTest failed, as "1; DROP TABLE m_loan; SELECT" in a select query was not detected as injection.
Added a TODO FINERACT-1870 so you will add the final solution.
Thank you
marta-jankovics
force-pushed
the
FINERACT-1910_Data_Table_query_Advanced_marta
branch
from
68aee23
to
9d2f3fe
Compare
| POSTGRESQL, // | ||
| ; | ||
|
|
||
| public boolean isMySql() { |
There was a problem hiding this comment.
Is there any added value of having these "convenience functions"? I think they hide more than they actually help... what happens if we add support for 10 more databases? We have other examples in the code base where these things quickly went out of hand and people started adding all kind of functionality to enums... whereas these files should be really simple and readable. My 2 cents.
Is a simple MYSQL.equals(dbType) so much worse (for me it's more readable... imagine static imports of those enums)?
BTW: we should really use "equals" instead "==".
There was a problem hiding this comment.
This enum is not new.
For enums the == comparison is better because
- enums are singleton classes and it never happens that the same enum is not identical with itself
- implementation of equals for enums is actually ==
- == is null safe
- == shows what it is, does not give the impression as if it could be not t
In my opinion enum is better than static string because - it can have its own functionality
- type safe and value safe
- singleton and identical
- ability to use as list type
- serialised by name (not by value)
marta-jankovics
force-pushed
the
FINERACT-1910_Data_Table_query_Advanced_marta
branch
6 times, most recently
from
8b64d4c
to
7d9c901
Compare
marta-jankovics
force-pushed
the
FINERACT-1910_Data_Table_query_Advanced_marta
branch
from
7d9c901
to
6cde7ad
Compare
marta-jankovics
force-pushed
the
FINERACT-1910_Data_Table_query_Advanced_marta
branch
from
6cde7ad
to
cbdb729
Compare
marta-jankovics
force-pushed
the
FINERACT-1910_Data_Table_query_Advanced_marta
branch
7 times, most recently
from
e3861ee
to
570e380
Compare
marta-jankovics
changed the title
| @@ -86,4 +89,106 @@ public static LocalDate getBusinessLocalDate() { | |||
| public static long getDifferenceInDays(final LocalDate localDateBefore, final LocalDate localDateAfter) { | |||
| return DAYS.between(localDateBefore, localDateAfter); | |||
| } | |||
|
|
|||
| public static int compareToBusinessDate(LocalDate date) { | |||
| return compare(date, getBusinessLocalDate()); | ||
| } | ||
|
|
||
| public static boolean isEqualBusinessDate(LocalDate date) { |
| return isEqual(date, getBusinessLocalDate()); | ||
| } | ||
|
|
||
| public static boolean isBeforeBusinessDate(LocalDate date) { |
| return isBefore(date, getBusinessLocalDate()); | ||
| } | ||
|
|
||
| public static boolean isAfterBusinessDate(LocalDate date) { |
| return isAfter(date, getBusinessLocalDate()); | ||
| } | ||
|
|
||
| public static int compare(LocalDate first, LocalDate second) { |
There was a problem hiding this comment.
unused, also it is wrong: if we are comparing two null LocalDate it will says the first is earlier which is not true.
| @@ -1940,8 +2047,8 @@ private String validateDatatableRegistered(String datatable) { | |||
|
|
|||
| private boolean isRegisteredDataTable(final String datatable) { | |||
| final String sql = "SELECT COUNT(application_table_name) FROM " + TABLE_REGISTERED_TABLE + " WHERE registered_table_name = ?"; | |||
| final int count = jdbcTemplate.queryForObject(sql, Integer.class, datatable); | |||
| return count > 0; | |||
| final Integer count = jdbcTemplate.queryForObject(sql, Integer.class, datatable); | |||
There was a problem hiding this comment.
count function can never return null. using int is more than fine and no need for null check
There was a problem hiding this comment.
for me it was a checkstyle warning, if it can not return null, it is fine, but lets be safe
| @@ -1890,7 +1997,7 @@ public boolean isDatatableAttachedToEntityDatatableCheck(final String datatableN | |||
| + " JOIN m_entity_datatable_check edc ON edc.x_registered_table_name = xrt.registered_table_name" | |||
| + " WHERE edc.x_registered_table_name = '" + datatableName + "'"; | |||
| final Long count = this.jdbcTemplate.queryForObject(sql, Long.class); | |||
| return count > 0; | |||
| return count != null && count > 0; | |||
There was a problem hiding this comment.
count function can never return null. using int is more than fine and no need for null check
There was a problem hiding this comment.
for me it was a checkstyle warning, if it can not return null, it is fine, but lets be safe
| PagedRequest<AdvancedQueryRequest> queryRequest, @Context final UriInfo uriInfo) { | ||
| final org.springframework.data.domain.Page<JsonObject> result = transactionsSearchServiceImpl.queryAdvanced(savingsId, | ||
| queryRequest); | ||
| return this.toApiJsonSerializer.serializePretty(ApiParameterHelper.prettyPrint(uriInfo.getQueryParameters()), result); |
There was a problem hiding this comment.
The rule is
for V2 it is mandatory to be object response
for V1 new API it is okay to be object response
| public static List<String> validateToJdbcColumnNames(List<String> columns, Map<String, ResultsetColumnHeaderData> headersByName, | ||
| boolean allowEmpty) { | ||
| List<ResultsetColumnHeaderData> columnHeaders = validateToJdbcColumns(columns, headersByName, allowEmpty); | ||
| return columnHeaders.stream().map(e -> e == null ? null : e.getColumnName()).toList(); |
There was a problem hiding this comment.
in what situation can the columnHeaders element be null?
There was a problem hiding this comment.
columnHeaders can not be null, but the elements in it could be (in case there are null values in the columns list and allowEmpty is true)
| assertNull(((List) ((Map) ((List) items.get("data")).get(1)).get("row")).get(8)); | ||
| assertNull(((List) ((Map) ((List) items.get("data")).get(1)).get("row")).get(9)); | ||
| List headers = (List) items.get("columnHeaders"); | ||
| List values0 = (List) ((Map) ((List) items.get("data")).get(0)).get("row"); |
There was a problem hiding this comment.
Can you please rename this to values?
There was a problem hiding this comment.
no, because its the first table row values, and there are also values from the second table row, see line 525
There was a problem hiding this comment.
I see. FirstRowValues, secondRowValues then?
marta-jankovics
force-pushed
the
FINERACT-1910_Data_Table_query_Advanced_marta
branch
3 times, most recently
from
92d0f31
to
e59b580
Compare
marta-jankovics
force-pushed
the
FINERACT-1910_Data_Table_query_Advanced_marta
branch
from
e59b580
to
2b561dd
Compare
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.