Re: [BUGS] hstore parser incorrectly handles malformed input
On Thu, Apr 26, 2012 at 9:12 PM, Tom Lane wrote: > I wrote: >> Ryan Kelly writes: >>> In my mind, all of these should have been rejected as erroneous input. >>> To that end, I have attached a patch which causes all of these inputs >>> to be rejected as invalid. > >> Hm, I would have expected all three of these to result in "a" having >> an empty-string value. I see nothing in the hstore documentation >> suggesting that I must write a=>"" or some such to get an empty value, > > Attached is an alternative patch that fixes it that way. > > Does anybody else have an opinion as to which of these solutions is > more preferable? And should we regard this as a back-patchable bug > fix, or a definition change suitable only for HEAD? I vote for not back-patching, regardless of exactly what we decide to do here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] hstore parser incorrectly handles malformed input
On Sat, Apr 28, 2012 at 17:20, Ryan Kelly wrote:
> I don't think any language supports the empty key edge case in this way.
> The only language I know of that will let you get away with it is Perl,
> and in that case you get undef, not the empty string, and it's a warning
> if you have warnings on:
>
> $ use warnings;
> $ my $h = { 1 => };
> Odd number of elements in anonymous hash
Well... kind of but not really, perl (as usual) is a bit funky here.
Try it with more than one key. What ends up happening is the 2nd key
is used as the first keys value... Surprise! You also don't get the
warning, it sees 1=>2,(void) or something.
$ perl -E 'my $h = {1=>,2=>}; say $h->{1};'
2
Anyway, +1 for what tom proposed.
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] hstore parser incorrectly handles malformed input
On Fri, Apr 27, 2012 at 10:22:11AM -0400, Tom Lane wrote:
> Ryan Kelly writes:
> > As long as we make it consistent on both sides of the '=>' (and document
> > it, too), then I don't really care either way. Currently you have to use
> > quotes to get an empty key, so I thought it natural to that you should
> > have to quote to get an empty value.
>
> > I've attached a modified version of Tom's patch which also allows empty
> > keys.
>
> Hm ... I don't agree that keys and values are interchangeable, and
> I don't see that empty keys are a good thing (whereas empty values are
> clearly a reasonable edge case). So I think this is going a bit far;
> it seems to me it'd be giving up a lot of syntax-error detection
> capability in return for some not-actually-helpful symmetry.
I don't think any language supports the empty key edge case in this way.
The only language I know of that will let you get away with it is Perl,
and in that case you get undef, not the empty string, and it's a warning
if you have warnings on:
$ use warnings;
$ my $h = { 1 => };
Odd number of elements in anonymous hash
$HASH1 = { 1 => undef };
In fact, it's akin to doing:
[db]> select hstore('{foo}'::text[]);
ERROR: array must have even number of elements
Or maybe even this:
[db]> select hstore('{foo,}'::text[]);
ERROR: malformed array literal: "{foo,}"
Python:
>>> h = {1 : }
SyntaxError: invalid syntax
Ruby:
>> h = {1 => }
SyntaxError: compile error
C#:
csharp> var h = new Dictionary(){ { "1", } };
{interactive}(1,50): error CS1525: Unexpected symbol `}'
csharp> var h = new Dictionary(){ { "1" } };
{interactive}(1,44): error CS1501: No overload for method `Add' takes `1'
arguments
JavaScript:
js> a = { 1: }
typein:1: SyntaxError: syntax error:
PHP:
$ echo ' ); ?>' | php
PHP Parse error: syntax error, unexpected ')'
> On the other hand, I seldom use hstore so I'm probably not the best
> person to be judging the appropriateness of these options. Any other
> votes out there?
As long as we're voting, I vote for my original patch. I think requiring
something on the RHS of => to get a value is a sane and rather common thing.
>
> regards, tom lane
-Ryan Kelly
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] hstore parser incorrectly handles malformed input
Ryan Kelly writes: > As long as we make it consistent on both sides of the '=>' (and document > it, too), then I don't really care either way. Currently you have to use > quotes to get an empty key, so I thought it natural to that you should > have to quote to get an empty value. > I've attached a modified version of Tom's patch which also allows empty > keys. Hm ... I don't agree that keys and values are interchangeable, and I don't see that empty keys are a good thing (whereas empty values are clearly a reasonable edge case). So I think this is going a bit far; it seems to me it'd be giving up a lot of syntax-error detection capability in return for some not-actually-helpful symmetry. On the other hand, I seldom use hstore so I'm probably not the best person to be judging the appropriateness of these options. Any other votes out there? regards, tom lane -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] hstore parser incorrectly handles malformed input
On Fri, Apr 27, 2012 at 11:27:03AM +0200, Vik Reykja wrote:
> On Fri, Apr 27, 2012 at 03:12, Tom Lane wrote:
>
> > Does anybody else have an opinion as to which of these solutions is
> > more preferable?
> >
>
> I think all unquoted whitespace should be ignored, so I prefer your
> solution. (note: I haven't actually tested it, I'm going off these emails)
As long as we make it consistent on both sides of the '=>' (and document
it, too), then I don't really care either way. Currently you have to use
quotes to get an empty key, so I thought it natural to that you should
have to quote to get an empty value.
I've attached a modified version of Tom's patch which also allows empty
keys.
>
>
> > And should we regard this as a back-patchable bug
> > fix, or a definition change suitable only for HEAD?
> >
>
> Since this is removing a syntax error and not creating one, I'd say it
> should be safe to backpatch.
-Ryan Kelly
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index 0eb48cf..f03dcdc 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -74,7 +74,15 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
}
else if (*(state->ptr) == '=' && !ignoreeq)
{
-elog(ERROR, "Syntax error near '%c' at postion %d", *(state->ptr), (int4) (state->ptr - state->begin));
+/* Empty key is perfectly OK */
+state->ptr--;
+return true;
+ }
+ else if (*(state->ptr) == ',' && ignoreeq)
+ {
+/* Empty value is perfectly OK */
+state->ptr--;
+return true;
}
else if (*(state->ptr) == '\\')
{
@@ -191,7 +199,7 @@ parse_hstore(HSParser *state)
if (st == WKEY)
{
if (!get_val(state, false, &escaped))
-return;
+return; /* end of string */
if (state->pcur >= state->plen)
{
state->plen *= 2;
@@ -236,7 +244,10 @@ parse_hstore(HSParser *state)
else if (st == WVAL)
{
if (!get_val(state, true, &escaped))
-elog(ERROR, "Unexpected end of string");
+ {
+/* end of string, treat as empty value */
+state->ptr--;
+ }
state->pairs[state->pcur].val = state->word;
state->pairs[state->pcur].vallen = hstoreCheckValLen(state->cur - state->word);
state->pairs[state->pcur].isnull = false;
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] hstore parser incorrectly handles malformed input
On Fri, Apr 27, 2012 at 03:12, Tom Lane wrote: > Does anybody else have an opinion as to which of these solutions is > more preferable? > I think all unquoted whitespace should be ignored, so I prefer your solution. (note: I haven't actually tested it, I'm going off these emails) > And should we regard this as a back-patchable bug > fix, or a definition change suitable only for HEAD? > Since this is removing a syntax error and not creating one, I'd say it should be safe to backpatch.
Re: [BUGS] hstore parser incorrectly handles malformed input
I wrote:
> Ryan Kelly writes:
>> In my mind, all of these should have been rejected as erroneous input.
>> To that end, I have attached a patch which causes all of these inputs
>> to be rejected as invalid.
> Hm, I would have expected all three of these to result in "a" having
> an empty-string value. I see nothing in the hstore documentation
> suggesting that I must write a=>"" or some such to get an empty value,
Attached is an alternative patch that fixes it that way.
Does anybody else have an opinion as to which of these solutions is
more preferable? And should we regard this as a back-patchable bug
fix, or a definition change suitable only for HEAD?
regards, tom lane
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index dde6c4b376ea2a82ed7d3cc0ef040e52f4df393a..08eafa18f1600d21e35eb4c3298215318dfebd9d 100644
*** a/contrib/hstore/hstore_io.c
--- b/contrib/hstore/hstore_io.c
*** get_val(HSParser *state, bool ignoreeq,
*** 74,81
--- 74,88
}
else if (*(state->ptr) == '=' && !ignoreeq)
{
+ /* Empty key is a syntax error */
elog(ERROR, "Syntax error near '%c' at position %d", *(state->ptr), (int4) (state->ptr - state->begin));
}
+ else if (*(state->ptr) == ',' && ignoreeq)
+ {
+ /* Empty value is perfectly OK */
+ state->ptr--;
+ return true;
+ }
else if (*(state->ptr) == '\\')
{
st = GV_WAITESCIN;
*** parse_hstore(HSParser *state)
*** 191,197
if (st == WKEY)
{
if (!get_val(state, false, &escaped))
! return;
if (state->pcur >= state->plen)
{
state->plen *= 2;
--- 198,204
if (st == WKEY)
{
if (!get_val(state, false, &escaped))
! return; /* end of string */
if (state->pcur >= state->plen)
{
state->plen *= 2;
*** parse_hstore(HSParser *state)
*** 236,242
else if (st == WVAL)
{
if (!get_val(state, true, &escaped))
! elog(ERROR, "Unexpected end of string");
state->pairs[state->pcur].val = state->word;
state->pairs[state->pcur].vallen = hstoreCheckValLen(state->cur - state->word);
state->pairs[state->pcur].isnull = false;
--- 243,252
else if (st == WVAL)
{
if (!get_val(state, true, &escaped))
! {
! /* end of string, treat as empty value */
! state->ptr--;
! }
state->pairs[state->pcur].val = state->word;
state->pairs[state->pcur].vallen = hstoreCheckValLen(state->cur - state->word);
state->pairs[state->pcur].isnull = false;
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] hstore parser incorrectly handles malformed input
Ryan Kelly writes: > It seems that the hstore parser has some odd behavior in the the > handling of certain malformed input constructions: > [db]> select 'a=>,b=>1'::hstore; > hstore > -- > "a"=>",b=>1" > [db]> select 'a=> ,b=>1'::hstore; > hstore > -- > "a"=>",b=>1" > [db]> select 'a=>, b=>1'::hstore; > ERROR: Syntax error near 'b' at position 5 > LINE 2: select 'a=>, b=>1'::hstore; > In my mind, all of these should have been rejected as erroneous input. > To that end, I have attached a patch which causes all of these inputs > to be rejected as invalid. Hm, I would have expected all three of these to result in "a" having an empty-string value. I see nothing in the hstore documentation suggesting that I must write a=>"" or some such to get an empty value, and it also says "whitespace between pairs or around the => sign is ignored". So what is your reasoning for calling this malformed input? (We're in agreement that the current behavior is wrong, though.) regards, tom lane -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
[BUGS] hstore parser incorrectly handles malformed input
It seems that the hstore parser has some odd behavior in the the
handling of certain malformed input constructions:
[db]> select 'a=>,b=>1'::hstore;
hstore
--
"a"=>",b=>1"
[db]> select 'a=> ,b=>1'::hstore;
hstore
--
"a"=>",b=>1"
[db]> select 'a=>, b=>1'::hstore;
ERROR: Syntax error near 'b' at position 5
LINE 2: select 'a=>, b=>1'::hstore;
In my mind, all of these should have been rejected as erroneous input.
To that end, I have attached a patch which causes all of these inputs
to be rejected as invalid.
-Ryan Kelly
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index 0eb48cf..13d6c22 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -76,6 +76,10 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
{
elog(ERROR, "Syntax error near '%c' at postion %d", *(state->ptr), (int4) (state->ptr - state->begin));
}
+ else if (*(state->ptr) == ',')
+ {
+elog(ERROR, "Syntax error near '%c' at postion %d", *(state->ptr), (int4) (state->ptr - state->begin));
+ }
else if (*(state->ptr) == '\\')
{
st = GV_WAITESCIN;
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
