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 2021/01/31 16:24:48 UTC
[GitHub] [fineract] BLasan opened a new pull request #1598: Removed unwanted result-set previous method
BLasan opened a new pull request #1598:
URL: https://github.com/apache/fineract/pull/1598
## Description
See [FINERACT-1089](https://issues.apache.org/jira/browse/FINERACT-1089)
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [x] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [x] 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.
- [x] 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/api-docs/apiLive.htm with details of any API changes
- [x] 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.
----------------------------------------------------------------
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] BLasan edited a comment on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan edited a comment on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-773227196
> Hi @BLasan - no, the ResultSet initially doesn't point at the first row. But this is not a question about the initial position: the same ResultSet is being iterated by both the calling function and the called function. The calling function expects the ResultSet to be returned in such state that by calling rs.next() it gets the next record to be processed. If you remove rs.previous() from the called function, that will no longer be the case and one record gets skipped. You can try it out by stepping through the code with a debugger and seeing what happens (as long as your JDBC driver supports rs.previous())
Hello @ptuomola , could you please mention the calling function of the extractData with regarding this issue? I was unable to find it as I'm new to the community. Could you please help me in finding 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] awasum commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-774410511
@BLasan . This might be more complex than you think. Given that you are new to the community, Would you like to look at our JIRA board and pick beginner issues?
JIRA Dashboard: https://issues.apache.org/jira/secure/Dashboard.jspa?selectPageId=12335824
Beginner friendly issues: https://issues.apache.org/jira/browse/FINERACT-1012?filter=12349022
----------------------------------------------------------------
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] ptuomola commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-820329942
Yes - so in all the cases where rs.previous() is currently called, the
output should be the same but without calling rs.previous()
On Thu, Apr 15, 2021, 18:50 Benura Abeywardena ***@***.***>
wrote:
> If you look at the code, I think the call to rs.previous() only happens in
> case of having a specific combination of interest rate slabs / interest
> rates that are read from the database.
>
> Yuh, so the goal of this issue is to replace that with an appropriate one
> without affecting the functionality right?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/fineract/pull/1598#issuecomment-820328895>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AASJVCWC3BAMOC5X53ITQULTI3AHTANCNFSM4W3PY63Q>
> .
>
--
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-812864590
@ptuomola Would you mind adding the reproducing steps? Isn't it in the interest charts tab in FD creation?
--
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-788540295
> @BLasan do you see any chance that this PR gets done - let's say - in the next couple of days? Just asking, because of the upcoming release (important security fix). No worries, we'll have immediately another release on the heels of this one. Just to know for triaging the Jira issues.
Shall I give an update tonight sir ? Need to reproduce this and check again
----------------------------------------------------------------
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] ptuomola commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-771684590
Hi @BLasan - no, the ResultSet initially doesn't point at the first row. But this is not a question about the initial position: the same ResultSet is being iterated by both the calling function and the called function. The calling function expects the ResultSet to be returned in such state that by calling rs.next() it gets the next record to be processed. If you remove rs.previous() from the called function, that will no longer be the case and one record gets skipped. You can try it out by stepping through the code with a debugger and seeing what happens (as long as your JDBC driver supports rs.previous())
----------------------------------------------------------------
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 #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-841747267
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] ptuomola commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-820316288
If you look at the code, I think the call to rs.previous() only happens in case of having a specific combination of interest rate slabs / interest rates that are read from the database.
--
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-773227196
@ptuomola Hi sir!! Could you please add the steps to reproduce this error. I'm
> Hi @BLasan - no, the ResultSet initially doesn't point at the first row. But this is not a question about the initial position: the same ResultSet is being iterated by both the calling function and the called function. The calling function expects the ResultSet to be returned in such state that by calling rs.next() it gets the next record to be processed. If you remove rs.previous() from the called function, that will no longer be the case and one record gets skipped. You can try it out by stepping through the code with a debugger and seeing what happens (as long as your JDBC driver supports rs.previous())
Hello @ptuomola , could you please mention the calling function of the extractData with regarding this issue? I was unable to find it as I'm new to the community. Could you please help me in finding 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] vidakovic commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-788419560
@BLasan do you see any chance that this PR gets done - let's say - in the next couple of days? Just asking, because of the upcoming release (important security fix). No worries, we'll have immediately another release on the heels of this one. Just to know for triaging the Jira issues.
----------------------------------------------------------------
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-773227196
@ptuomola Hi sir!! Could you please add the steps to reproduce this error. I'm
> Hi @BLasan - no, the ResultSet initially doesn't point at the first row. But this is not a question about the initial position: the same ResultSet is being iterated by both the calling function and the called function. The calling function expects the ResultSet to be returned in such state that by calling rs.next() it gets the next record to be processed. If you remove rs.previous() from the called function, that will no longer be the case and one record gets skipped. You can try it out by stepping through the code with a debugger and seeing what happens (as long as your JDBC driver supports rs.previous())
Hello @ptuomola , could you please mention the calling function of the extractData with regarding this issue? I was unable to find it as I'm new to the community. Could you please help me in finding 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] BLasan edited a comment on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan edited a comment on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-773227196
> Hi @BLasan - no, the ResultSet initially doesn't point at the first row. But this is not a question about the initial position: the same ResultSet is being iterated by both the calling function and the called function. The calling function expects the ResultSet to be returned in such state that by calling rs.next() it gets the next record to be processed. If you remove rs.previous() from the called function, that will no longer be the case and one record gets skipped. You can try it out by stepping through the code with a debugger and seeing what happens (as long as your JDBC driver supports rs.previous())
Hello @ptuomola , could you please mention the calling function of the extractData with regarding this issue? I was unable to find it as I'm new to the community. Could you please help me in finding 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] github-actions[bot] commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-812246204
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-820314767
> @BLasan Sorry, I don't have steps to reproduce. You should be able to work them out by looking at the code? But that's what I would need to do to work out how to reproduce.
>
> This was originally reported by @vorburger as he saw it in the logs - if it's still happening on fineract.dev, he may be able to point to the account where this is happening to reproduce.
For this currently Fineract has used `rs.previous()` in iterating the result set. I think that's what @vorburger has reported. But there were no error getting back from the server when visit the interest chart tab (As far as I could remember this was related to a GET method)
--
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-771681068
----------------------------------------------------------------
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] ptuomola commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-771684590
Hi @BLasan - no, the ResultSet initially doesn't point at the first row. But this is not a question about the initial position: the same ResultSet is being iterated by both the calling function and the called function. The calling function expects the ResultSet to be returned in such state that by calling rs.next() it gets the next record to be processed. If you remove rs.previous() from the called function, that will no longer be the case and one record gets skipped. You can try it out by stepping through the code with a debugger and seeing what happens (as long as your JDBC driver supports rs.previous())
----------------------------------------------------------------
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-812842895
@ptuomola @vorburger any idea?
--
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-770410302
@vorburger @ptuomola Please review
----------------------------------------------------------------
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] BLasan edited a comment on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan edited a comment on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-812864590
@ptuomola Would you mind adding the reproducing steps? Isn't it in the interest rate charts tab in FD creation?
https://github.com/openMF/community-app/pull/3362
--
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] ptuomola commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-812862072
> > > @ptuomola @vorburger any idea?
> >
> >
> > Hello @BLasan - any idea about what?
>
> Didn't get any issue in getting interest charts. Does this issue still occur?
As far as I can see, the issue still exists. Not all JDBC drivers support resultset.previous(). It would make sense to refactor the logic so that previous() is not required.
--
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] ptuomola commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-820308082
@BLasan Sorry, I don't have steps to reproduce. You should be able to work them out by looking at the code? But that's what I would need to do to work out how to reproduce.
This was originally reported by @vorburger as he saw it in the logs - if it's still happening on fineract.dev, he may be able to point to the account where this is happening to reproduce.
--
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-820328895
> If you look at the code, I think the call to rs.previous() only happens in case of having a specific combination of interest rate slabs / interest rates that are read from the database.
Yuh, so the goal of this issue is to replace that with an appropriate one without affecting the functionality right?
--
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-812861588
> > @ptuomola @vorburger any idea?
>
> Hello @BLasan - any idea about what?
Didn't get any issue in getting interest charts. Does this issue still occur?
--
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-771681068
> Hi @BLasan - thanks for attempting this, but resolving this issue is a bit more complicated than just removing the offending lines as the calls to rs.previous() are there for a reason:
>
> Currently the called function takes the ResultSet and extracts data until it finds a row that doesn't belong with the others. Then it reverses the cursor by 1 row with rs.previous(). That way when the control returns to the calling function, the cursor is set so that the calling function gets the first row of the next group by calling rs.next().
>
> If we just remove the rs.previous(), then calling rs.next() in the calling function will give us the _second_ row of the next group, as we have already consumed the first row within the called function.
>
> So one way to solve this would be to "flatten" the functions so that all work with the ResultSet is in same function - so that there is no need to pass rs.previous(). But to ensure that the "flattening" doesn't change the behaviour, it would be good to first have a test case which checks the functionality. Then when we refactor the function, we can ensure we haven't broken the functionality.
>
> Hope this helps...
Hi. Please refer this. As far as know, initially it does not point to the first row of the selected data. This will give a clear idea
https://www.tutorialspoint.com/java-resultset-next-method-with-example
----------------------------------------------------------------
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] BLasan edited a comment on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan edited a comment on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-812864590
@ptuomola Would you mind adding the reproducing steps? Isn't it in the interest rate charts tab in FD creation?
--
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-774069115
@vorburger Sir, would you mind adding the steps to reproduce this issue?
----------------------------------------------------------------
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 #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-841747267
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] BLasan commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
BLasan commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-771713179
> Hi @BLasan - no, the ResultSet initially doesn't point at the first row. But this is not a question about the initial position: the same ResultSet is being iterated by both the calling function and the called function. The calling function expects the ResultSet to be returned in such state that by calling rs.next() it gets the next record to be processed. If you remove rs.previous() from the called function, that will no longer be the case and one record gets skipped. You can try it out by stepping through the code with a debugger and seeing what happens (as long as your JDBC driver supports rs.previous())
Oh!! I didn't notice that. Thanks for the guidance. I'll take a proper look again. :)
----------------------------------------------------------------
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] ptuomola commented on pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1598:
URL: https://github.com/apache/fineract/pull/1598#issuecomment-812844380
> @ptuomola @vorburger any idea?
Hello @BLasan - any idea about what?
--
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] closed pull request #1598: Removed unwanted result-set previous method
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #1598:
URL: https://github.com/apache/fineract/pull/1598
--
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