You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/07/12 20:32:28 UTC
[GitHub] [fineract] thesmallstar opened a new pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
thesmallstar opened a new pull request #1171:
URL: https://github.com/apache/fineract/pull/1171
There are 3 things to be reviewed:
1. Why is "o.hierarchy like" is added two times? - I could not find any particular reason:
-> The only reason that makes sense is we want it to be like X and also Y (which maybe be required).
2. I did not get what the following code was supposed to do.
```
if (StringUtils.isNotBlank(extraCriteria.toString())) {
extraCriteria.delete(0, 4);
}
```
Also this part
`and g.staff_id = ? `
was added twice, with nearly the same condition. I have removed one of them.
3. The same problem that was in #1123
that we take SQL template from user which can be any SQL query string, how do we prevent SQL injection in that case?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-660534457
Update: I could make it work, but the problem is it only supports few queries which are "like" "is" etc and does not support any operators like = >= etc. I am trying to add support of all operators.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar edited a comment on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-680301147
This search parameter (@vorburger take a look at this when you are free), you will see the best mix of all modules inside a file here, everyone is so much cross-linked, like centres use .forGroups function to init themselves, there are 7+ constructors each one of which should have been a different file.
I have my head spinning :P The build will pass B) ) (soon)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-657273721
I am looking into problem 3 currently. After which we can merge this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-680301147
This search parameter (@vorburger take a look at this when you are free), you will see the best mix of all modules inside a file here, everyone is so much cross-linked, like centres use .forGroups function to init themselves, there are 7+ constructors each one of which should have been a different file.
I have my head spinning :P The build will pass B) (soon)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-680278783
@vorburger For some reason I kept this pending, I thought somehow removing sqlsearch part affects this PR as well but it don't, since we are anyways not using the functionality for SQL search in groups.
If the build pass we should merge this, and we are halfway through the SQL Builder part.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] github-actions[bot] commented on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-678566564
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-660296444
@vorburger I have summarised what I found in my research:
Problem -> We are taking SQL from the user for example - a=b and c=d.
This is then appended directly (with current validations)
Example:
User input: a=b&c=d
output : Select abc from tables.. where (conditions) and a=b&c=d
Best Solution I could figure out:
To make this part of SQL builder we would need to
1. Split the user input on and so we get
A[0]=> a=b
B[0] => c=d
And then execute our addCriteria function on each of this input.
I am moving forward on this approach and open for suggestions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-660314611
@vorburger @awasum can you help me run the previous API? I have read the docs but haven't tried the API( just believed it would work as written in docs) but it turns out, I am getting a SQL error for all types of request I am making.
Have you ever tried this? If not I would reach out the mailing list.
I have tried:
https://demo.fineract.dev/fineract-provider/api/v1/clients?paged=true&sqlSearch=%22accountNo=000000003%22&tenantIdentifier=default
and
https://demo.fineract.dev/fineract-provider/api/v1/clients?paged=true&sqlSearch=accountNo%20like%20000000003&tenantIdentifier=default
can you make any of this work?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-680299522
> If the build pass we should merge this
it didn't :smiling_imp:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-678600900
@thesmallstar any updates on this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger merged pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #1171:
URL: https://github.com/apache/fineract/pull/1171
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar edited a comment on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-680301147
This search parameter (@vorburger take a look at this when you are free), you will see the best mix of all modules inside a file here, everything is so much cross-linked, like centres use .forGroups function to init themselves, there are 7+ constructors each one of which should have been a different file.
I have my head spinning :P The build will pass B) ) (soon)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on pull request #1171: FINERACT-854 Removed string concatenated SQL in GroupReadPlatform
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1171:
URL: https://github.com/apache/fineract/pull/1171#issuecomment-680309392
@vorburger I have also removed SQLsearch here (As planned)
#1123 can be merged after rebase (once this is merged)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org