FINERACT-854 Removed string concatenated SQL from CenterReadPlatform #1123

thesmallstar · 2020-06-29T09:44:56Z

Refer: https://issues.apache.org/jira/browse/FINERACT-854 and #725 #723 for background.
The work for this part is completed, but SQLbuilder currently does not support the use of "limit" and "order by" query which I will be adding before this can be merged.

thesmallstar · 2020-06-29T09:49:25Z

...src/main/java/org/apache/fineract/portfolio/group/service/CenterReadPlatformServiceImpl.java

-                extraCriteria.append(" and (").append(sqlQueryCriteria).append(") ");
-                this.columnValidator.validateSqlInjection(schemaSl, sqlQueryCriteria);


Also removing this part is not correct as of NOW, the problem is we are talking SQL query as a query to give users more functionality and certainly that is not supported in SQL builder currently, we probably would need to parse things find relevant queries and then add those to extra criteria still thinking on it.
Will be looked after limit and order by are supported.

@vorburger WDYT on this?

@thesmallstar I'm not sure I fully understand what you mean here, but starting to think about adding parsing things sounds like the wrong direction - try not to have to do that (it will be a mess - trust me).

@thesmallstar sorry for the huge delay in getting back to you on this one. I've re-read it again now (took me a minute to get back into it). Are you suggesting that we merge this now? Because, unless I misunderstand, this would break the currently existing functionality for these sqlQueryCriteria, agreed? But they really are a problem, huh? I need to dig more into the code to understand where this is coming from..

We can't (shouldn't) merge this PR as is, because it will break support for sqlSearch. We need to EITHER support it here, OR (my preference) should just cleanly remove it all together - as (now) suggested in https://issues.apache.org/jira/browse/FINERACT-1095.

thesmallstar · 2020-06-29T13:15:55Z

Blocked by: https://issues.apache.org/jira/browse/FINERACT-1058

vorburger · 2020-07-01T09:37:54Z

CenterIntegrationTest > testListCenters() FAILED
    java.lang.AssertionError: 1 expectation failed.
    Expected status code <200> but was <500>.```

thesmallstar · 2020-07-01T18:16:32Z

Failed due to https://issues.apache.org/jira/browse/FINERACT-1016
Looking into it.

thesmallstar · 2020-07-01T18:59:50Z

@vorburger @awasum if you like this approach Before we merge this I will add the tests for the same, if not do you have any suggestions?

vorburger

I'm OK with something like this, but have several feedbacks:

I don't know if this actually works (PreparedStatement), so will have to trust you that you test that it really does.
The current implementation seems to assume that setLimit() & Co. would only ever be called after addCriteria() - right? That's... not a good idea. Imagine some dumb developer coming along in a few months, not knowing how you implemented this internally (and correctly so, they should not have to). So they do new SQLBuilder().addOrderBy().addCriteria(...).getSQLTemplate() - and that would create invalid SQL - agreed?
SQLBuidlerTest must be extended to cover these new methods.

I'll re-review this week after next (vacation).

vorburger · 2020-07-03T14:41:44Z

...act-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/SQLBuilder.java

+     * @param orderBy
+     *            The value that will be used as orderBy
+     */
+    public void addOrderBY(Object orderBy) {


lower-case y:

Suggested change

public void addOrderBY(Object orderBy) {

public void addOrderBy(Object orderBy) {

thesmallstar · 2020-07-04T12:54:03Z

I don't know if this actually works (PreparedStatement), so will have to trust you that you test that it really does.

-> Yes it works! I have tested it :)

The current implementation seems to assume that setLimit() & Co. would only ever be called after addCriteria() - right? That's... not a good idea. Imagine some dumb developer coming along in a few months, not knowing how you implemented this internally (and correctly so, they should not have to). So they do new SQLBuilder().addOrderBy().addCriteria(...).getSQLTemplate() - and that would create invalid SQL - agreed?

->Agreed I am making the change.

SQLBuidlerTest must be extended to cover these new methods.

->Extending, I was waiting for the approach to be marked correct :)

thesmallstar · 2020-07-04T13:46:46Z

failed due to connection error. Closing and opening this PR to retest.

thesmallstar · 2020-07-05T22:00:19Z

I am removing the part to add limit and orderby to SQL builder separate from this PR, so as to review this properly, and also we can merge this quickly then and not keep it blocked by the work and testing on that part.

vorburger · 2020-07-22T21:47:34Z

...src/main/java/org/apache/fineract/portfolio/group/service/CenterReadPlatformServiceImpl.java

-                extraCriteria.append(" and (").append(sqlQueryCriteria).append(") ");
-                this.columnValidator.validateSqlInjection(schemaSl, sqlQueryCriteria);


@thesmallstar sorry for the huge delay in getting back to you on this one. I've re-read it again now (took me a minute to get back into it). Are you suggesting that we merge this now? Because, unless I misunderstand, this would break the currently existing functionality for these sqlQueryCriteria, agreed? But they really are a problem, huh? I need to dig more into the code to understand where this is coming from..

github-actions · 2020-08-22T00:30:47Z

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

xurror · 2020-08-22T06:07:22Z

@thesmallstar any updates on this?

thesmallstar · 2020-08-25T21:34:39Z

This build will fail now, will pass only after rebase from #1171

thesmallstar · 2020-08-26T10:24:57Z

@vorburger review/merge this?
Removed SQLsearch
Fixed small error in groups
SQL concatenation remove ;)

vorburger

LGTM

thesmallstar commented Jun 29, 2020

View reviewed changes

thesmallstar force-pushed the sqlsec1 branch 2 times, most recently from 19d9137 to 4ab4d9d Compare July 1, 2020 16:52

thesmallstar force-pushed the sqlsec1 branch from 4ab4d9d to e6a24b5 Compare July 2, 2020 18:42

vorburger requested changes Jul 3, 2020

View reviewed changes

vorburger reviewed Jul 3, 2020

View reviewed changes

thesmallstar force-pushed the sqlsec1 branch from e6a24b5 to 9249e25 Compare July 4, 2020 12:55

thesmallstar closed this Jul 4, 2020

thesmallstar reopened this Jul 4, 2020

thesmallstar force-pushed the sqlsec1 branch from 9249e25 to 7a89391 Compare July 4, 2020 22:30

thesmallstar marked this pull request as draft July 5, 2020 21:05

thesmallstar force-pushed the sqlsec1 branch 2 times, most recently from f70ae3b to adc0701 Compare July 5, 2020 22:15

thesmallstar mentioned this pull request Jul 12, 2020

FINERACT-854 Removed string concatenated SQL in GroupReadPlatform #1171

Merged

thesmallstar force-pushed the sqlsec1 branch from adc0701 to 008dfe7 Compare July 12, 2020 20:40

thesmallstar changed the title ~~FINERACT-854 Removed string concatenated SQL from CenterReadPlatformS…~~ FINERACT-854 Removed string concatenated SQL from CenterReadPlatform Jul 12, 2020

thesmallstar marked this pull request as ready for review July 12, 2020 20:41

thesmallstar mentioned this pull request Jul 12, 2020

Fineract-1058 added support for limit,offset and orderBy in SQLbuilder #1156

Closed

vorburger requested changes Jul 22, 2020

View reviewed changes

thesmallstar mentioned this pull request Aug 17, 2020

FINERACT-1095 Added status parameter in Loans API #1251

Closed

github-actions bot added the stale label Aug 22, 2020

thesmallstar force-pushed the sqlsec1 branch 2 times, most recently from e9249ad to 67d4126 Compare August 25, 2020 21:34

FINERACT-854 Removed string concatenated SQL from CenterReadPlatform

01a71fb

thesmallstar force-pushed the sqlsec1 branch from 67d4126 to 01a71fb Compare August 26, 2020 09:36

vorburger approved these changes Aug 26, 2020

View reviewed changes

vorburger merged commit b22d4b6 into apache:develop Aug 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FINERACT-854 Removed string concatenated SQL from CenterReadPlatform #1123

FINERACT-854 Removed string concatenated SQL from CenterReadPlatform #1123

thesmallstar commented Jun 29, 2020

thesmallstar Jun 29, 2020

thesmallstar Jul 1, 2020

vorburger Jul 3, 2020

vorburger Jul 22, 2020

vorburger Jul 23, 2020

thesmallstar commented Jun 29, 2020

vorburger commented Jul 1, 2020 •

edited

Loading

thesmallstar commented Jul 1, 2020

thesmallstar commented Jul 1, 2020

vorburger left a comment

vorburger Jul 3, 2020

thesmallstar commented Jul 4, 2020

thesmallstar commented Jul 4, 2020

thesmallstar commented Jul 5, 2020

vorburger Jul 22, 2020

github-actions bot commented Aug 22, 2020

xurror commented Aug 22, 2020

thesmallstar commented Aug 25, 2020

thesmallstar commented Aug 26, 2020

vorburger left a comment

		extraCriteria.append(" and (").append(sqlQueryCriteria).append(") ");
		this.columnValidator.validateSqlInjection(schemaSl, sqlQueryCriteria);

	public void addOrderBY(Object orderBy) {
	public void addOrderBy(Object orderBy) {

FINERACT-854 Removed string concatenated SQL from CenterReadPlatform #1123

FINERACT-854 Removed string concatenated SQL from CenterReadPlatform #1123

Conversation

thesmallstar commented Jun 29, 2020

thesmallstar Jun 29, 2020

Choose a reason for hiding this comment

thesmallstar Jul 1, 2020

Choose a reason for hiding this comment

vorburger Jul 3, 2020

Choose a reason for hiding this comment

vorburger Jul 22, 2020

Choose a reason for hiding this comment

vorburger Jul 23, 2020

Choose a reason for hiding this comment

thesmallstar commented Jun 29, 2020

vorburger commented Jul 1, 2020 • edited Loading

thesmallstar commented Jul 1, 2020

thesmallstar commented Jul 1, 2020

vorburger left a comment

Choose a reason for hiding this comment

vorburger Jul 3, 2020

Choose a reason for hiding this comment

thesmallstar commented Jul 4, 2020

thesmallstar commented Jul 4, 2020

thesmallstar commented Jul 5, 2020

vorburger Jul 22, 2020

Choose a reason for hiding this comment

github-actions bot commented Aug 22, 2020

xurror commented Aug 22, 2020

thesmallstar commented Aug 25, 2020

thesmallstar commented Aug 26, 2020

vorburger left a comment

Choose a reason for hiding this comment

vorburger commented Jul 1, 2020 •

edited

Loading