parameter parsing fails (regression) #296

Closed
kobruleht opened this Issue Jul 26, 2014 · 28 comments

Comments

Projects
None yet
6 participants
@kobruleht

Newer npgsql versions does not parse :p parameters properly.
In 2.2.0-beta1 code below produces error

syntax error at or near ":"

shown in comment. npgsql sends :p6 to server without parsing it.

        using (var connection = new NpgsqlConnection(ConnectionString()))
        {
            connection.Open();
            DbCommand command = (DbCommand)connection.CreateCommand();
           command.CommandText = @"
 SELECT 1
 WHERE ''= 'type(''m.response'')#''O''%' AND :p6
            ";

           for (int i = 0; i < 7; i++)
           {
               IDbDataParameter param = command.CreateParameter();
               param.ParameterName = "p" + i.ToString();
               var value = DBNull.Value;
               command.Parameters.Add(param);
           }
           // syntax error at or near ":"
            command.ExecuteReader();
         }

    static string ConnectionString()
    {
        NpgsqlConnectionStringBuilder csb = new NpgsqlConnectionStringBuilder()
        {
            SearchPath = "public",
            Host = "localhost",
            Database = "postgres",
            UserName = "postgres",
            Password = "secret"
        };
        return csb.ConnectionString;
    }

@roji roji added this to the 2.2 milestone Jul 26, 2014

@roji roji added the bug label Jul 26, 2014

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jul 26, 2014

Member

Should be resolved for 2.2...

Member

roji commented Jul 26, 2014

Should be resolved for 2.2...

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jul 26, 2014

Member

@kobruleht, any way you could send SQL to create the schema for this query, to help with reproducing?

Member

roji commented Jul 26, 2014

@kobruleht, any way you could send SQL to create the schema for this query, to help with reproducing?

@kobruleht

This comment has been minimized.

Show comment
Hide comment
@kobruleht

kobruleht Jul 26, 2014

This occurs with any schema. No need to create schema for reproducing.

Error is reported from Postgres server during parsing :p6 and before tables are accessed. I verified it by specifying standard postgres database as database name.
I updated issue and added ConnectionString() method.

This occurs with any schema. No need to create schema for reproducing.

Error is reported from Postgres server during parsing :p6 and before tables are accessed. I verified it by specifying standard postgres database as database name.
I updated issue and added ConnectionString() method.

@kobruleht

This comment has been minimized.

Show comment
Hide comment
@kobruleht

kobruleht Jul 26, 2014

@roji I updated testcase so it does not reference any tables.

@roji I updated testcase so it does not reference any tables.

roji added a commit to roji/Npgsql that referenced this issue Jul 26, 2014

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jul 26, 2014

Member

Thanks for reporting @kobruleht, I can indeed reproduce this.

@glenebob, I think this is in your area of expertise... I've made a commit with a repro test: roji@b1a0378. Running it clearly shows that the string :p6 gets sent to the backend instead of getting replaced in AppendCommandReplacingParameterValues. Do you think you can take a look?

Member

roji commented Jul 26, 2014

Thanks for reporting @kobruleht, I can indeed reproduce this.

@glenebob, I think this is in your area of expertise... I've made a commit with a repro test: roji@b1a0378. Running it clearly shows that the string :p6 gets sent to the backend instead of getting replaced in AppendCommandReplacingParameterValues. Do you think you can take a look?

@kobruleht

This comment has been minimized.

Show comment
Hide comment
@kobruleht

kobruleht Jul 26, 2014

@roji my updated testcase contains 2 lines of sql to reproduce the issue. You can replace big sql in your commit with those 2 lines. It looke like AppendCommandReplacingParameterValues does not work properly if sql string contains '' which represent single quote.
In earlier versions of npgsql it worked OK.

@roji my updated testcase contains 2 lines of sql to reproduce the issue. You can replace big sql in your commit with those 2 lines. It looke like AppendCommandReplacingParameterValues does not work properly if sql string contains '' which represent single quote.
In earlier versions of npgsql it worked OK.

@franciscojunior

This comment has been minimized.

Show comment
Hide comment
@franciscojunior

franciscojunior Jul 29, 2014

Member

Hi, @glenebob ! Were you able to have a look at this issue? It is a pretty important regression.
Thanks in advance!

Member

franciscojunior commented Jul 29, 2014

Hi, @glenebob ! Were you able to have a look at this issue? It is a pretty important regression.
Thanks in advance!

@glenebob

This comment has been minimized.

Show comment
Hide comment
@glenebob

glenebob Jul 29, 2014

Contributor

Ah, no, I missed it (was out on vacation all last week). I'll try to look
at it in the next couple days.

-Glen

On Tue, Jul 29, 2014 at 9:55 AM, Francisco Figueiredo Jr. <
notifications@github.com> wrote:

Hi, @glenebob https://github.com/glenebob ! Were you able to have a
look at this issue? It is a pretty important regression.
Thanks in advance!


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

Contributor

glenebob commented Jul 29, 2014

Ah, no, I missed it (was out on vacation all last week). I'll try to look
at it in the next couple days.

-Glen

On Tue, Jul 29, 2014 at 9:55 AM, Francisco Figueiredo Jr. <
notifications@github.com> wrote:

Hi, @glenebob https://github.com/glenebob ! Were you able to have a
look at this issue? It is a pretty important regression.
Thanks in advance!


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

@franciscojunior

This comment has been minimized.

Show comment
Hide comment
@franciscojunior

franciscojunior Jul 29, 2014

Member

Ah, no, I missed it (was out on vacation all last week). I'll try to look at it in the next couple days.

Thank you very much, Glen!

Member

franciscojunior commented Jul 29, 2014

Ah, no, I missed it (was out on vacation all last week). I'll try to look at it in the next couple days.

Thank you very much, Glen!

@franciscojunior

This comment has been minimized.

Show comment
Hide comment
@franciscojunior

franciscojunior Aug 1, 2014

Member

I think this problem was also reported in #240 .

Member

franciscojunior commented Aug 1, 2014

I think this problem was also reported in #240 .

@glenebob

This comment has been minimized.

Show comment
Hide comment
@glenebob

glenebob Aug 2, 2014

Contributor

@roji, I took your repro test and simplified the heck out of it :) I should have this fixed shortly.

-Glen

Contributor

glenebob commented Aug 2, 2014

@roji, I took your repro test and simplified the heck out of it :) I should have this fixed shortly.

-Glen

@glenebob

This comment has been minimized.

Show comment
Hide comment
@glenebob

glenebob Aug 2, 2014

Contributor

@franciscojunior, yep, #240 looks like the same issue. We'll find out in a bit.

-Glen

Contributor

glenebob commented Aug 2, 2014

@franciscojunior, yep, #240 looks like the same issue. We'll find out in a bit.

-Glen

@glenebob

This comment has been minimized.

Show comment
Hide comment
@glenebob

glenebob Aug 2, 2014

Contributor

Check out PR #307 ;)

Contributor

glenebob commented Aug 2, 2014

Check out PR #307 ;)

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Aug 2, 2014

Member

Great @glenebob, it's awesome to have this out of the way.

One small comment: note that @kobruleht submitted a much shorter repro than the huge SQL he first submitted (see the description above) - it's better to replace my unit test with it.

I'll also try to quickly review tomorrow!

Member

roji commented Aug 2, 2014

Great @glenebob, it's awesome to have this out of the way.

One small comment: note that @kobruleht submitted a much shorter repro than the huge SQL he first submitted (see the description above) - it's better to replace my unit test with it.

I'll also try to quickly review tomorrow!

@glenebob

This comment has been minimized.

Show comment
Hide comment
@glenebob

glenebob Aug 2, 2014

Contributor

Good point @roji, and I missed @kobruleht https://github.com/kobruleht's
simplified query the first time through. Check out the latest commit; I
added @kobruleht https://github.com/kobruleht's query (simplified a bit
further), and also @alenkacz https://github.com/alenkacz's query from bug
#240. So, we run three queries in the test now.

-Glen

On Sat, Aug 2, 2014 at 2:18 PM, Shay Rojansky notifications@github.com
wrote:

Great @glenebob https://github.com/glenebob, it's awesome to have this
out of the way.

One small comment: note that @kobruleht https://github.com/kobruleht
submitted a much shorter repro than the huge SQL he first submitted (see
the description above) - it's better to replace my unit test with it.


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

Contributor

glenebob commented Aug 2, 2014

Good point @roji, and I missed @kobruleht https://github.com/kobruleht's
simplified query the first time through. Check out the latest commit; I
added @kobruleht https://github.com/kobruleht's query (simplified a bit
further), and also @alenkacz https://github.com/alenkacz's query from bug
#240. So, we run three queries in the test now.

-Glen

On Sat, Aug 2, 2014 at 2:18 PM, Shay Rojansky notifications@github.com
wrote:

Great @glenebob https://github.com/glenebob, it's awesome to have this
out of the way.

One small comment: note that @kobruleht https://github.com/kobruleht
submitted a much shorter repro than the huge SQL he first submitted (see
the description above) - it's better to replace my unit test with it.


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

glenebob added a commit that referenced this issue Aug 4, 2014

Merge pull request #307 from glenebob/bug_296
Fix bugs #240 and  #296, parameter substituion troubles
@glenebob

This comment has been minimized.

Show comment
Hide comment
@glenebob

glenebob Aug 4, 2014

Contributor

PR #307 merged. Please have a look.

Contributor

glenebob commented Aug 4, 2014

PR #307 merged. Please have a look.

roji added a commit that referenced this issue Aug 4, 2014

Repro unit test for #296
(cherry picked from commit b1a0378)

Backport #307 from master

glenebob added a commit that referenced this issue Aug 4, 2014

Simplfied repro unit test bugs #240 and #296
This is about as simple as I can make it.

(cherry picked from commit 0d13218)

Backport #307 from master

glenebob added a commit that referenced this issue Aug 4, 2014

Fix for bugs #240 and #296
Properly handle escaped single quote character ('') in a quoted section during query parameter substitution.

(cherry picked from commit 253b6f9)

Backport #307 from master

glenebob added a commit that referenced this issue Aug 4, 2014

Bugs #249 and #296 Unit Test Improvements
Use close approximations to the repro queries supplied for these bugs, in addition to the simple one devised for diagnosis.

(cherry picked from commit d45f620)

Backport #307 from master
@kobruleht

This comment has been minimized.

Show comment
Hide comment
@kobruleht

kobruleht Aug 13, 2014

Ca we add empty string

select ''

to unit test. It also stopped parameter parsing.

Ca we add empty string

select ''

to unit test. It also stopped parameter parsing.

@franciscojunior

This comment has been minimized.

Show comment
Hide comment
@franciscojunior

franciscojunior Aug 17, 2014

Member

This is fixed in Npgsql 2.2.0-rc1. Please, give it a try and let us know if you find any other problem.

Member

franciscojunior commented Aug 17, 2014

This is fixed in Npgsql 2.2.0-rc1. Please, give it a try and let us know if you find any other problem.

@franciscojunior

This comment has been minimized.

Show comment
Hide comment
@franciscojunior

franciscojunior Aug 17, 2014

Member

@kobruleht , can you confirm if the empty string is still an issue? It was supposed to be fixed by now. I'm closing this issue now. If you still find problems with it, let us know.

Member

franciscojunior commented Aug 17, 2014

@kobruleht , can you confirm if the empty string is still an issue? It was supposed to be fixed by now. I'm closing this issue now. If you still find problems with it, let us know.

@FransBouma

This comment has been minimized.

Show comment
Hide comment
@FransBouma

FransBouma Oct 23, 2014

Contributor

Still fails in 2.2.1 with an output parameter.

proc:
CREATE OR REPLACE FUNCTION pr_getcustomeramount(IN pcountry character varying, OUT pcustomercount integer)
RETURNS integer AS
$BODY$
BEGIN
SELECT COUNT(*) INTO pCustomerCount FROM Customers WHERE Country = pCountry;
END;
$BODY$
LANGUAGE plpgsql VOLATILE
COST 100;
ALTER FUNCTION pr_getcustomeramount(character varying)
OWNER TO postgres;

(on northwind DB on postgres).

Call: "Northwind"."public"."pr_getcustomeramount"(:pcountry, :pcustomercount)"

Fails with the same error at ':'. See for more info:
http://www.llblgen.com/tinyforum/GotoMessage.aspx?MessageID=130522&ThreadID=22680

I'll see if I have time later today whether I can repro it in a unit test with the latest trunk code of npgsql. I can reproduce it on the released 2.2.1 assembly (.net 4.0 build)

Contributor

FransBouma commented Oct 23, 2014

Still fails in 2.2.1 with an output parameter.

proc:
CREATE OR REPLACE FUNCTION pr_getcustomeramount(IN pcountry character varying, OUT pcustomercount integer)
RETURNS integer AS
$BODY$
BEGIN
SELECT COUNT(*) INTO pCustomerCount FROM Customers WHERE Country = pCountry;
END;
$BODY$
LANGUAGE plpgsql VOLATILE
COST 100;
ALTER FUNCTION pr_getcustomeramount(character varying)
OWNER TO postgres;

(on northwind DB on postgres).

Call: "Northwind"."public"."pr_getcustomeramount"(:pcountry, :pcustomercount)"

Fails with the same error at ':'. See for more info:
http://www.llblgen.com/tinyforum/GotoMessage.aspx?MessageID=130522&ThreadID=22680

I'll see if I have time later today whether I can repro it in a unit test with the latest trunk code of npgsql. I can reproduce it on the released 2.2.1 assembly (.net 4.0 build)

@Emill

This comment has been minimized.

Show comment
Hide comment
@Emill

Emill Oct 23, 2014

Member

It would be good with a unit test that shows the problem. I don't know how llblgen calls Npgsql. For example, is command type set to stored procedure or text.

Member

Emill commented Oct 23, 2014

It would be good with a unit test that shows the problem. I don't know how llblgen calls Npgsql. For example, is command type set to stored procedure or text.

@FransBouma

This comment has been minimized.

Show comment
Hide comment
@FransBouma

FransBouma Oct 23, 2014

Contributor

Sorry, should have given that info. It creates a DbCommand using the factory and sets the commandtype to StoredProcedure. Input parameters only calls work fine, output parameters don't. I'll write a test which uses native commands to illustrate the problem.

Contributor

FransBouma commented Oct 23, 2014

Sorry, should have given that info. It creates a DbCommand using the factory and sets the commandtype to StoredProcedure. Input parameters only calls work fine, output parameters don't. I'll write a test which uses native commands to illustrate the problem.

@franciscojunior

This comment has been minimized.

Show comment
Hide comment
@franciscojunior

franciscojunior Oct 23, 2014

Member

Hi, @FransBouma !

When working with parameter in a NpgsqlCommand whose commandtype is StoredProcedure, you don't need to specify them in the CommandText. You just need to add the parameters as usual in the Parameters collection and let Npgsql take care of them for you.

In the case of output parameters, Postgresql return them as a resultset after the function is executed. Them, Npgsql will map them to any output parameter specified in your Parameters collection. If you add the output parameter marker in the commandtext, Npgsql will ignore it when calling the function (because it is output parameter) and then you get the error.
In the case of inputparameters, Npgsql will use them when calling the function. That's why you don't get errors when using input parameters only.
In order to be consistent and don't have this problem again, I'd suggest you to not add any parameter markers at all in your commandtext.

Check here a test we have about output parameter in our testsuite: (you can find a lot of other output parameters usage in our testsuite)

[Test]
        public void TestBug1006158OutputParameters()
        {
            const string createFunction =
                @"CREATE OR REPLACE FUNCTION more_params(OUT a integer, OUT b boolean) AS
            $BODY$DECLARE
                BEGIN
                    a := 3;
                    b := true;
                END;$BODY$
              LANGUAGE 'plpgsql' VOLATILE;";

            var command = new NpgsqlCommand(createFunction, Conn);
            command.ExecuteNonQuery();

            command = new NpgsqlCommand("more_params", Conn);
            command.CommandType = CommandType.StoredProcedure;

            command.Parameters.Add(new NpgsqlParameter("a", DbType.Int32));
            command.Parameters[0].Direction = ParameterDirection.Output;
            command.Parameters.Add(new NpgsqlParameter("b", DbType.Boolean));
            command.Parameters[1].Direction = ParameterDirection.Output;

            var result = command.ExecuteScalar();

            Assert.AreEqual(3, command.Parameters[0].Value);
            Assert.AreEqual(true, command.Parameters[1].Value);
        }

Note that only the name is specified in the commandtext and output parameters are added to the Parameters collection and have their value populated correctly.

I hope it helps.

Member

franciscojunior commented Oct 23, 2014

Hi, @FransBouma !

When working with parameter in a NpgsqlCommand whose commandtype is StoredProcedure, you don't need to specify them in the CommandText. You just need to add the parameters as usual in the Parameters collection and let Npgsql take care of them for you.

In the case of output parameters, Postgresql return them as a resultset after the function is executed. Them, Npgsql will map them to any output parameter specified in your Parameters collection. If you add the output parameter marker in the commandtext, Npgsql will ignore it when calling the function (because it is output parameter) and then you get the error.
In the case of inputparameters, Npgsql will use them when calling the function. That's why you don't get errors when using input parameters only.
In order to be consistent and don't have this problem again, I'd suggest you to not add any parameter markers at all in your commandtext.

Check here a test we have about output parameter in our testsuite: (you can find a lot of other output parameters usage in our testsuite)

[Test]
        public void TestBug1006158OutputParameters()
        {
            const string createFunction =
                @"CREATE OR REPLACE FUNCTION more_params(OUT a integer, OUT b boolean) AS
            $BODY$DECLARE
                BEGIN
                    a := 3;
                    b := true;
                END;$BODY$
              LANGUAGE 'plpgsql' VOLATILE;";

            var command = new NpgsqlCommand(createFunction, Conn);
            command.ExecuteNonQuery();

            command = new NpgsqlCommand("more_params", Conn);
            command.CommandType = CommandType.StoredProcedure;

            command.Parameters.Add(new NpgsqlParameter("a", DbType.Int32));
            command.Parameters[0].Direction = ParameterDirection.Output;
            command.Parameters.Add(new NpgsqlParameter("b", DbType.Boolean));
            command.Parameters[1].Direction = ParameterDirection.Output;

            var result = command.ExecuteScalar();

            Assert.AreEqual(3, command.Parameters[0].Value);
            Assert.AreEqual(true, command.Parameters[1].Value);
        }

Note that only the name is specified in the commandtext and output parameters are added to the Parameters collection and have their value populated correctly.

I hope it helps.

@FransBouma

This comment has been minimized.

Show comment
Hide comment
@FransBouma

FransBouma Oct 23, 2014

Contributor

Ok that helps :) I'll strip out generating the output proc names and leave the input proc names in, as I do recall they had to be there at some point in old(er) versions of npgsql? (removing all parameters from the header then would break code for ppl who still use older npgsql versions, but I don't have exact info on this)

(edit) omitting the output parameters works indeed. (on 2.2.1)

Contributor

FransBouma commented Oct 23, 2014

Ok that helps :) I'll strip out generating the output proc names and leave the input proc names in, as I do recall they had to be there at some point in old(er) versions of npgsql? (removing all parameters from the header then would break code for ppl who still use older npgsql versions, but I don't have exact info on this)

(edit) omitting the output parameters works indeed. (on 2.2.1)

@Emill

This comment has been minimized.

Show comment
Hide comment
@Emill

Emill Oct 23, 2014

Member

Well, according to the documentation at http://msdn.microsoft.com/en-us/library/system.data.common.dbcommand.commandtype(v=vs.110).aspx, it states that the CommandText should simply contain the name of the stored procedure...

Member

Emill commented Oct 23, 2014

Well, according to the documentation at http://msdn.microsoft.com/en-us/library/system.data.common.dbcommand.commandtype(v=vs.110).aspx, it states that the CommandText should simply contain the name of the stored procedure...

@franciscojunior

This comment has been minimized.

Show comment
Hide comment
@franciscojunior

franciscojunior Oct 23, 2014

Member

Ok that helps :) I'll strip out generating the output proc names and leave the input proc names in, as I do recall they had to be there at some point in old(er) versions of npgsql? (removing all parameters from the header then would break code for ppl who still use older npgsql versions, but I don't have exact info on this)

Yeah. If I recall correctly, only very very old Npgsql versions would need the parameter markers when calling functions, so you are right to have them there. I think that for 2.0.x and above you can omit them without fear.

(edit) omitting the output parameters works indeed. (on 2.2.1)

Excellent!

Please, let us know if you find any other problem, Frans!

I'm a big fan of your work and I'm glad to know Npgsql is being helpful to bring support of your tool to Postgresql!

Member

franciscojunior commented Oct 23, 2014

Ok that helps :) I'll strip out generating the output proc names and leave the input proc names in, as I do recall they had to be there at some point in old(er) versions of npgsql? (removing all parameters from the header then would break code for ppl who still use older npgsql versions, but I don't have exact info on this)

Yeah. If I recall correctly, only very very old Npgsql versions would need the parameter markers when calling functions, so you are right to have them there. I think that for 2.0.x and above you can omit them without fear.

(edit) omitting the output parameters works indeed. (on 2.2.1)

Excellent!

Please, let us know if you find any other problem, Frans!

I'm a big fan of your work and I'm glad to know Npgsql is being helpful to bring support of your tool to Postgresql!

@FransBouma

This comment has been minimized.

Show comment
Hide comment
@FransBouma

FransBouma Oct 23, 2014

Contributor

:) Thanks Francisco! Yes stripping everything off (the brackets and all parameter markers) works fine, which makes it easy to fix this. If v2.x.y supports this, it's fine by me, older versions are too old anyway. So consider this solved! :)

Contributor

FransBouma commented Oct 23, 2014

:) Thanks Francisco! Yes stripping everything off (the brackets and all parameter markers) works fine, which makes it easy to fix this. If v2.x.y supports this, it's fine by me, older versions are too old anyway. So consider this solved! :)

@franciscojunior

This comment has been minimized.

Show comment
Hide comment
@franciscojunior

franciscojunior Oct 23, 2014

Member

:) Thanks Francisco! Yes stripping everything off (the brackets and all parameter markers) works fine, which makes it easy to fix this. If v2.x.y supports this, it's fine by me, older versions are too old anyway. So consider this solved! :)

Awesome! I'm glad to hear that. :)

Member

franciscojunior commented Oct 23, 2014

:) Thanks Francisco! Yes stripping everything off (the brackets and all parameter markers) works fine, which makes it easy to fix this. If v2.x.y supports this, it's fine by me, older versions are too old anyway. So consider this solved! :)

Awesome! I'm glad to hear that. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment