Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Useless condition in parseQuery #454

Merged
merged 1 commit into from Dec 14, 2015
Merged

Conversation

@marschall
Copy link
Contributor

@marschall marschall commented Dec 13, 2015

AbstractJdbc2ResultSet#parseQuery contains the following code

if ( !tableFound )
{
}
else
{
}

However since that code is inside a loop with the following condition

while ( !tableFound && !tablesChecked && st.hasMoreTokens() )

We know that tableFound will always be false and therefore the true
branch of the if else will always be executed.

AbstractJdbc2ResultSet#parseQuery contains a condition that is always
true.
@vlsi
Copy link
Member

@vlsi vlsi commented Dec 13, 2015

@marschall , you seem to miss singleTable = !name.equalsIgnoreCase(","); assignment.

Can you please double check that?

Loading

@marschall
Copy link
Contributor Author

@marschall marschall commented Dec 13, 2015

I don't think I did, if we look at the original code we have

        while ( !tableFound && /* .. */ )
        {
            // ...
            if ( !tableFound ) // always true because we're in the loop
            {
                // always executed because !tableFound is always true
            }
            else
            {  // never executed because the condition in the if is always true
                tablesChecked = true;
                // if the very next token is , then there are multiple tables
                singleTable = !name.equalsIgnoreCase(",");
            }
        }

The else branch is never executed and the singleTable = !name.equalsIgnoreCase(","); assignment never happens. As a consequence singleTable is always true after #parseQuery() has been executed.

Loading

@vlsi
Copy link
Member

@vlsi vlsi commented Dec 13, 2015

I think the intention was to return "no updates possible if there are joins in query": https://github.com/marschall/pgjdbc/blob/useless-condition/org/postgresql/jdbc2/AbstractJdbc2ResultSet.java#L1636-L1640
So !tableFound && !tablesChecked should be corrected to (!tableFound || !tablesChecked).

Loading

@marschall
Copy link
Contributor Author

@marschall marschall commented Dec 13, 2015

@vlsi you mean the condition in the while? If so I can fix that (and would create a new PR if that is ok).

Loading

@vlsi
Copy link
Member

@vlsi vlsi commented Dec 13, 2015

@marschall ,
Frankly speaking, that parseQuery logic seems too fragile, so I'm fine with your current PR (I can't commit, so we still need to wait for someone powerful).
At least your PR does stress true logic behind parseQuery.

It could make sense to use ResultSetMetaData instead of parseQuery to check if there is just a single table though.

It would be nice to improve test for the case of "resultSet.updateRow() for multi-table select" as current org.postgresql.test.jdbc2.UpdateableResultTest#testUpdateable seem to fail to catch that bug.

Loading

@vlsi
Copy link
Member

@vlsi vlsi commented Dec 13, 2015

Regarding "condition in the while": I would prefer keep it as it is or rewrite the whole thing to ResultSetMetaData or something more robust than a StringTokenizer

Loading

@marschall
Copy link
Contributor Author

@marschall marschall commented Dec 14, 2015

Fine with me. I'll do nothing for now and wait for further guidance.

Loading

@davecramer
Copy link
Member

@davecramer davecramer commented Dec 14, 2015

@visi, so you are OK with this now or should I wait ?

Dave Cramer

On 14 December 2015 at 06:34, Philippe Marschall notifications@github.com
wrote:

Fine with me. I'll do nothing for now and wait for further guidance.


Reply to this email directly or view it on GitHub
#454 (comment).

Loading

@vlsi
Copy link
Member

@vlsi vlsi commented Dec 14, 2015

I'm OK.

Loading

davecramer added a commit that referenced this issue Dec 14, 2015
fix:Useless condition in parseQuery
@davecramer davecramer merged commit dcd1071 into pgjdbc:master Dec 14, 2015
1 check passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants