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

Replace question marks only in PreparedStatements #427

Merged
merged 3 commits into from Nov 25, 2015

Conversation

Projects
None yet
3 participants
@laurenz
Contributor

laurenz commented Nov 18, 2015

In this thread Thomas Kellerer uttered his surprise that questions marks are replaced with positional parameters in a java.sql.Statement.

I think that is a bug, an I came up with this patch to fix it.

My only concern is that this is a behaviour change that might break existing code...

Laurenz Albe
Replace question marks only in PreparedStatements
Question marks in the query text should be replaced with positional
parameters ($1, $2, ...) only in java.sql.PreparedStatement.
In a java.sql.Statement they should be left alone.
@davecramer

This comment has been minimized.

Member

davecramer commented Nov 18, 2015

Thanks!, any chance you can add some test cases? I also suspect this only fixes this particular problem. What happens if there is an extra single ? with parameters ?

@vlsi

This comment has been minimized.

Member

vlsi commented Nov 18, 2015

@laurenz , please use spaces, not tabs for indent.

This might be a questionable change:

  1. We need to identify what was the behavior of previous driver.
  2. With this change it would be impossible to copy-paste a query between prepared and non-prepared statements. This might be the source of surprises like "prepared statements do not work".
@davecramer

This comment has been minimized.

Member

davecramer commented Nov 18, 2015

also looks like it fails CompositeQueryParseTest

@davecramer

This comment has been minimized.

Member

davecramer commented Nov 18, 2015

The easiest thing might be to document the use of ?? instead of trying to code around it. This is a hack we were forced into. I'm not convinced I want to fix this corner case

@vlsi

This comment has been minimized.

Member

vlsi commented Nov 18, 2015

This is a hack we were forced into. I'm not convinced I want to fix this corner case

+1

We might even do better job by handling PSQLException: ERROR: syntax error at or near "$1" kind of errors and adding pgjdbc notice like "if you want to use question-mark operator, use ??" help message right into the exception.

@davecramer

This comment has been minimized.

Member

davecramer commented Nov 18, 2015

On 18 November 2015 at 06:30, Vladimir Sitnikov notifications@github.com
wrote:

This is a hack we were forced into. I'm not convinced I want to fix this
corner case

+1

We might even do better job by handling PSQLException: ERROR: syntax
error at or near "$1" kind of errors and adding pgjdbc notice like "if
you want to use question-mark operator, use ??" help message right into the
exception.

+1

Dave Cramer

@laurenz

This comment has been minimized.

Contributor

laurenz commented Nov 18, 2015

Well, CompositeQueryParseTest fails because they specifically test the ?? escape.

I think the argument that it might make copy and paste between java.sql.Statement and java.sql.PreparedStatement more difficult is a weak one, but I certainly understand that the behaviour change might cause problems.

My patch does not modify the behaviour for java.sql.PreparedStatements, so having a ? operator there still requires escaping it.

I don't know if java.sql.Statement.execute() can be called a corner case, though.

BTW, could the documentation build on the web site be updated? I see the ?? escape documented in the source, but not on the web site. That might also help with the problem.

If there is a consensus against it, I'll retract the patch.

@davecramer

This comment has been minimized.

Member

davecramer commented Nov 18, 2015

I'll try to update the docs ASAP.

The corner case I refer to is someone putting a single ? in a select
statement without parameters
Dave

Dave Cramer

On 18 November 2015 at 07:07, Laurenz Albe notifications@github.com wrote:

Well, CompositeQueryParseTest fails because they specifically test the ??
escape.

I think the argument that it might make copy and paste between
java.sql.Statement and java.sql.PreparedStatement more difficult is a
weak one, but I certainly understand that the behaviour change might cause
problems.

My patch does not modify the behaviour for java.sql.PreparedStatements,
so having a ? operator there still requires escaping it.

I don't know if java.sql.Statement.execute() can be called a corner case,
though.

BTW, could the documentation build on the web site be updated? I see the
?? escape documented in the source, but not on the web site. That might
also help with the problem.

If there is a consensus against it, I'll retract the patch.


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

@vlsi

This comment has been minimized.

Member

vlsi commented Nov 19, 2015

I don't know if java.sql.Statement.execute() can be called a corner case, though

The question is: what was pgjdbc behavior as of January 2015?
Did Statement require escaping? If it did, then there are probably end-users who use that.
If not, it might be a bug introduced during recent preparedstatement-parser fixes, so it might be fixed.

Laurenz Albe
Allow both single and double question marks in simple statements
This is an improvement over the previous commit:
Now you can either use a single question mark in a java.sql.Statement
which will be interpreted as such, or you can use escaped (doubled)
question marks.
This way code that used to work will continue to work, while people
who do not expect that question marks need to be escaped in simple
statements will not be surprised.

This commit also fixes the regression tests and adds a new one.
@laurenz

This comment has been minimized.

Contributor

laurenz commented Nov 20, 2015

After sleeping on it, I came up with this approach:

  • For simple statements, allow both single and double question marks.

This preserves the old behaviour while avoiding to surprise users who don't expect that a question mark needs to be escaped in a simple statement.

I have also fixed the test cases and added a new one.

@vlsi

This comment has been minimized.

Member

vlsi commented Nov 20, 2015

allow both single and double question marks.

Are you sure there are no operators that include double question marks?

nativeSql.append(NativeQuery.bindName(bindIndex));
}
else
nativeSql.append('?');

This comment has been minimized.

@vlsi

vlsi Nov 20, 2015

Member

Please put "shortest branch" first, please always use braces (it seems to be the project style)

@vlsi

This comment has been minimized.

Member

vlsi commented Nov 20, 2015

I've did a quick check with

select * from pg_operator where oprname like '%??%'

and it looks like there are no built-in operators with double question marks.
That is quite a smart move.

@laurenz , can you please add test that ?? is converted back to ? when using simple statement?

Laurenz Albe
Prettify code and add another regression test
Make the code adhere to project style guidlines.
The new tests verifies that double question marks will work with
simple statements.
@laurenz

This comment has been minimized.

Contributor

laurenz commented Nov 20, 2015

Thanks for the feedback; I improved the code and added another regression test.

@davecramer

This comment has been minimized.

Member

davecramer commented Nov 20, 2015

I'm a little concerned that we now have two different ways to do this. One
for prepared statements and another for statements
On Nov 20, 2015 6:26 AM, "Laurenz Albe" notifications@github.com wrote:

Thanks for the feedback; I improved the code and added another regression
test.


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

@laurenz

This comment has been minimized.

Contributor

laurenz commented Nov 20, 2015

I'm a little concerned that we now have two different ways to do this. One for prepared statements and another for statements

I don't say that it is very pretty, but my impression is that the ?? escape is slightly hacky and nonstandard itself.

My change is driven by the desire to follow the principle of least astonishment.

I admit that it feels a little like DWIM, but I can't think of a case where it could go wrong.

@davecramer

This comment has been minimized.

Member

davecramer commented Nov 20, 2015

My thought is that someone switching between statements and prepared
statements would be surprised by this behaviour
On Nov 20, 2015 6:39 AM, "Laurenz Albe" notifications@github.com wrote:

I'm a little concerned that we now have two different ways to do this. One
for prepared statements and another for statements

I don't say that it is very pretty, but my impression is that the ??
escape is slightly hacky and nonstandard itself.

My change is driven by the desire to follow the principle of least
astonishment.

I admit that it feels a little like DWIM
https://en.wikipedia.org/wiki/DWIM, but I can't think of a case where
it could go wrong.


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

@laurenz

This comment has been minimized.

Contributor

laurenz commented Nov 20, 2015

I have seen a user who was surprised by the necessity to escape question marks.

Now what cases could cause surprise with the new behaviour:

  1. Somebody used a ? operator in a java.sql.Statement and is now surprised that that is replaced with $n in a java.sql.PreparedStatement.
    But everybody who uses a java.sql.PreparedStatement would expect ? to be a parameter, right?
  2. Somebody uses a parameter in a java.sql.PreparedStatement and is now surprised that it is taken literally in a java.sql.Statement.
    It is hard to imagine that somebody woud expect parameters to work in that case, and anyhow the error message would be more meaningful than it is today.
@vlsi

This comment has been minimized.

Member

vlsi commented Nov 20, 2015

I'm inclined to +1 for the change

@davecramer

This comment has been minimized.

Member

davecramer commented Nov 22, 2015

Here is where the documentation for the site lives
https://github.com/pgjdbc/www

Wondering if we can get corresponding docs ?

Dave Cramer

On 20 November 2015 at 10:13, Vladimir Sitnikov notifications@github.com
wrote:

I'm inclined to +1 for the change


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

@laurenz

This comment has been minimized.

Contributor

laurenz commented Nov 23, 2015

I'd be happy to contribute documentation, but I need help:

  • Is there any documentation how to build the documentation?
    I tried ant doc in pgjdbc/pgjdbc but it complained about missing stuff.
  • In the project you just gave me there are lots of md files. What are they?
  • Which of the two projects has the proper documentation source?
    Or are they generated from each other?
@davecramer

This comment has been minimized.

Member

davecramer commented Nov 23, 2015

They are markdown files and github will almost display them.

They use jekyll to build them http://jekyllrb.com/docs/configuration/

Thanks!

Dave Cramer

On 23 November 2015 at 03:12, Laurenz Albe notifications@github.com wrote:

I'd be happy to contribute documentation, but I need help:

  • Is there any documentation how to build the documentation?
    I tried ant doc in pgjdbc/pgjdbc but it complained about missing stuff.
  • In the project you just gave me there are lots of md files. What are
    they?
  • Which of the two projects has the proper documentation source?
    Or are they generated from each other?


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

@laurenz

This comment has been minimized.

Contributor

laurenz commented Nov 23, 2015

Thanks, but my question remains:

  • What do I do to change the doc? Update the md files in documentation/head?

In the file doc/pgjdbc.xml in the pgjdbc project, I find ?-contained operator escapes documented, but not in the www project.Can you explain that?

@davecramer

This comment has been minimized.

Member

davecramer commented Nov 23, 2015

Yes, in documention/head.

As for the docs in /docs. I need to delete that file

Dave Cramer

On 23 November 2015 at 06:58, Laurenz Albe notifications@github.com wrote:

Thanks, but my question remains:

  • What do I do to change the doc? Update the md files in
    documentation/head?

In the doc/pgjdbc.xml in the pgjdbc project, I find ?-contained operator
escapes
documented, but not in the www project.Can you explain that?


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

@laurenz

This comment has been minimized.

Contributor

laurenz commented Nov 23, 2015

I cannot install jekyll on my RHEL 6 machine.
It requires a recent ruby, and that would mean building ruby from source.
Isn't there a simpler way to update the documentation?

@davecramer

This comment has been minimized.

Member

davecramer commented Nov 23, 2015

On 23 November 2015 at 10:59, Laurenz Albe notifications@github.com wrote:

I cannot install jekyll on my RHEL 6 machine.
It requires a recent ruby, and that would mean building ruby from source.
Isn't there a simpler way to update the documentation?

Not that I am aware of ?


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

@laurenz

This comment has been minimized.

Contributor

laurenz commented Nov 24, 2015

Ok, my best effort is here: pgjdbc/www#16

@laurenz

This comment has been minimized.

Contributor

laurenz commented Nov 24, 2015

I guess you'll want to merge this request since you merged the documentation for it :^)

davecramer added a commit that referenced this pull request Nov 25, 2015

Merge pull request #427 from laurenz/master
Replace question marks only in PreparedStatements

@davecramer davecramer merged commit c48dff9 into pgjdbc:master Nov 25, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment