Re: bug in json_to_record with arrays - Mailing list pgsql-hackers
| From | Tom Lane |
|---|---|
| Subject | Re: bug in json_to_record with arrays |
| Date | |
| Msg-id | 15621.1417208131@sss.pgh.pa.us Whole thread Raw |
| In response to | Re: bug in json_to_record with arrays (Josh Berkus <josh@agliodbs.com>) |
| List | pgsql-hackers |
Josh Berkus <josh@agliodbs.com> writes:
> On 11/26/2014 12:48 PM, Tom Lane wrote:
>> Wouldn't it be better if it said
>>
>> ERROR: invalid input syntax for array: "["potter","chef","programmer"]"
>> DETAIL: Dimension value is missing.
>>
>> which is comparable to what you'd get out of most other input functions
>> that were feeling indigestion?
> Yes, it most definitely would be better.
Here's a draft patch to improve array_in's error reports. With this
patch, what you'd get for this example is
# select * from json_to_record('
{"id":1,"val":"josh","valry":["potter","chef","programmer"]}') as r(id
int, val text, valry text[]);
ERROR: malformed array literal: "["potter","chef","programmer"]"
DETAIL: "[" must introduce explicitly-specified array dimensions.
Some comments:
* Probably the main objection that could be leveled against this approach
is that it's reasonable to expect that array literal strings could be
pretty long, maybe longer than is reasonable to print all of in a primary
error message. However, the same could be said about record literal
strings; yet I've not heard any complaints about the fact that record_in
just prints the whole input string when it's unhappy. So that's what this
does too.
* The phrasing "malformed array literal" matches some already-existing
error texts, as well as record_in which uses "malformed record literal".
I wonder though if we shouldn't change all of these to "invalid input
syntax for array" (resp. "record"), which seems more like our usual
phraseology in other datatype input functions. OTOH, that would make
the primary error message even longer.
* I changed every one of the ERRCODE_INVALID_TEXT_REPRESENTATION cases
to "malformed array literal" with an errdetail. I did not touch some
existing ereport's with different SQLSTATEs, notably "upper bound cannot
be less than lower bound". Anyone have a different opinion about whether
that case needs to print the string contents?
* Some of the errdetails don't really seem all that helpful (the ones
triggered by the existing regression test cases are examples). Can anyone
think of better wording? Should we just leave those out?
* This would only really address Josh's complaint if we were to back-patch
it into 9.4, but I'm hesitant to change error texts post-RC1. Thoughts?
regards, tom lane
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 743351b..933c6b0 100644
*** a/src/backend/utils/adt/arrayfuncs.c
--- b/src/backend/utils/adt/arrayfuncs.c
*************** array_in(PG_FUNCTION_ARGS)
*** 247,257 ****
errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
ndim + 1, MAXDIM)));
! for (q = p; isdigit((unsigned char) *q) || (*q == '-') || (*q == '+'); q++);
if (q == p) /* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("missing dimension value")));
if (*q == ':')
{
--- 247,259 ----
errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
ndim + 1, MAXDIM)));
! for (q = p; isdigit((unsigned char) *q) || (*q == '-') || (*q == '+'); q++)
! /* skip */ ;
if (q == p) /* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", string),
! errdetail("\"[\" must introduce explicitly-specified array dimensions.")));
if (*q == ':')
{
*************** array_in(PG_FUNCTION_ARGS)
*** 259,269 ****
*q = '\0';
lBound[ndim] = atoi(p);
p = q + 1;
! for (q = p; isdigit((unsigned char) *q) || (*q == '-') || (*q == '+'); q++);
if (q == p) /* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("missing dimension value")));
}
else
{
--- 261,273 ----
*q = '\0';
lBound[ndim] = atoi(p);
p = q + 1;
! for (q = p; isdigit((unsigned char) *q) || (*q == '-') || (*q == '+'); q++)
! /* skip */ ;
if (q == p) /* no digits? */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", string),
! errdetail("Missing array dimension value.")));
}
else
{
*************** array_in(PG_FUNCTION_ARGS)
*** 273,279 ****
if (*q != ']')
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("missing \"]\" in array dimensions")));
*q = '\0';
ub = atoi(p);
--- 277,285 ----
if (*q != ']')
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", string),
! errdetail("Missing \"%s\" after array dimensions.",
! "]")));
*q = '\0';
ub = atoi(p);
*************** array_in(PG_FUNCTION_ARGS)
*** 293,299 ****
if (*p != '{')
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("array value must start with \"{\" or dimension information")));
ndim = ArrayCount(p, dim, typdelim);
for (i = 0; i < ndim; i++)
lBound[i] = 1;
--- 299,306 ----
if (*p != '{')
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", string),
! errdetail("Array value must start with \"{\" or dimension information.")));
ndim = ArrayCount(p, dim, typdelim);
for (i = 0; i < ndim; i++)
lBound[i] = 1;
*************** array_in(PG_FUNCTION_ARGS)
*** 307,313 ****
if (strncmp(p, ASSGN, strlen(ASSGN)) != 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("missing assignment operator")));
p += strlen(ASSGN);
while (array_isspace(*p))
p++;
--- 314,322 ----
if (strncmp(p, ASSGN, strlen(ASSGN)) != 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", string),
! errdetail("Missing \"%s\" after array dimensions.",
! ASSGN)));
p += strlen(ASSGN);
while (array_isspace(*p))
p++;
*************** array_in(PG_FUNCTION_ARGS)
*** 319,336 ****
if (*p != '{')
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("array value must start with \"{\" or dimension information")));
ndim_braces = ArrayCount(p, dim_braces, typdelim);
if (ndim_braces != ndim)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("array dimensions incompatible with array literal")));
for (i = 0; i < ndim; ++i)
{
if (dim[i] != dim_braces[i])
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("array dimensions incompatible with array literal")));
}
}
--- 328,348 ----
if (*p != '{')
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", string),
! errdetail("Array contents must start with \"{\".")));
ndim_braces = ArrayCount(p, dim_braces, typdelim);
if (ndim_braces != ndim)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", string),
! errdetail("Specified array dimensions do not match array contents.")));
for (i = 0; i < ndim; ++i)
{
if (dim[i] != dim_braces[i])
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", string),
! errdetail("Specified array dimensions do not match array contents.")));
}
}
*************** ArrayCount(const char *str, int *dim, ch
*** 460,466 ****
/* Signal a premature end of the string */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str)));
break;
case '\\':
--- 472,479 ----
/* Signal a premature end of the string */
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str),
! errdetail("Unexpected end of input.")));
break;
case '\\':
*************** ArrayCount(const char *str, int *dim, ch
*** 475,481 ****
parse_state != ARRAY_ELEM_DELIMITED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str)));
if (parse_state != ARRAY_QUOTED_ELEM_STARTED)
parse_state = ARRAY_ELEM_STARTED;
/* skip the escaped character */
--- 488,496 ----
parse_state != ARRAY_ELEM_DELIMITED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str),
! errdetail("Unexpected \"%c\" character.",
! '\\')));
if (parse_state != ARRAY_QUOTED_ELEM_STARTED)
parse_state = ARRAY_ELEM_STARTED;
/* skip the escaped character */
*************** ArrayCount(const char *str, int *dim, ch
*** 484,490 ****
else
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str)));
break;
case '\"':
--- 499,506 ----
else
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str),
! errdetail("Unexpected end of input.")));
break;
case '\"':
*************** ArrayCount(const char *str, int *dim, ch
*** 498,504 ****
parse_state != ARRAY_ELEM_DELIMITED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str)));
in_quotes = !in_quotes;
if (in_quotes)
parse_state = ARRAY_QUOTED_ELEM_STARTED;
--- 514,521 ----
parse_state != ARRAY_ELEM_DELIMITED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str),
! errdetail("Unexpected array element.")));
in_quotes = !in_quotes;
if (in_quotes)
parse_state = ARRAY_QUOTED_ELEM_STARTED;
*************** ArrayCount(const char *str, int *dim, ch
*** 518,524 ****
parse_state != ARRAY_LEVEL_DELIMITED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str)));
parse_state = ARRAY_LEVEL_STARTED;
if (nest_level >= MAXDIM)
ereport(ERROR,
--- 535,543 ----
parse_state != ARRAY_LEVEL_DELIMITED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str),
! errdetail("Unexpected \"%c\" character.",
! '{')));
parse_state = ARRAY_LEVEL_STARTED;
if (nest_level >= MAXDIM)
ereport(ERROR,
*************** ArrayCount(const char *str, int *dim, ch
*** 546,566 ****
!(nest_level == 1 && parse_state == ARRAY_LEVEL_STARTED))
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str)));
parse_state = ARRAY_LEVEL_COMPLETED;
if (nest_level == 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str)));
nest_level--;
if (nelems_last[nest_level] != 0 &&
nelems[nest_level] != nelems_last[nest_level])
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("multidimensional arrays must have "
! "array expressions with matching "
! "dimensions")));
nelems_last[nest_level] = nelems[nest_level];
nelems[nest_level] = 1;
if (nest_level == 0)
--- 565,589 ----
!(nest_level == 1 && parse_state == ARRAY_LEVEL_STARTED))
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str),
! errdetail("Unexpected \"%c\" character.",
! '}')));
parse_state = ARRAY_LEVEL_COMPLETED;
if (nest_level == 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str),
! errdetail("Unmatched \"%c\" character.", '}')));
nest_level--;
if (nelems_last[nest_level] != 0 &&
nelems[nest_level] != nelems_last[nest_level])
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str),
! errdetail("Multidimensional arrays must have "
! "sub-arrays with matching "
! "dimensions.")));
nelems_last[nest_level] = nelems[nest_level];
nelems[nest_level] = 1;
if (nest_level == 0)
*************** ArrayCount(const char *str, int *dim, ch
*** 591,597 ****
parse_state != ARRAY_LEVEL_COMPLETED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str)));
if (parse_state == ARRAY_LEVEL_COMPLETED)
parse_state = ARRAY_LEVEL_DELIMITED;
else
--- 614,622 ----
parse_state != ARRAY_LEVEL_COMPLETED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str),
! errdetail("Unexpected \"%c\" character.",
! typdelim)));
if (parse_state == ARRAY_LEVEL_COMPLETED)
parse_state = ARRAY_LEVEL_DELIMITED;
else
*************** ArrayCount(const char *str, int *dim, ch
*** 612,618 ****
parse_state != ARRAY_ELEM_DELIMITED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str)));
parse_state = ARRAY_ELEM_STARTED;
}
}
--- 637,644 ----
parse_state != ARRAY_ELEM_DELIMITED)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str),
! errdetail("Unexpected array element.")));
parse_state = ARRAY_ELEM_STARTED;
}
}
*************** ArrayCount(const char *str, int *dim, ch
*** 631,637 ****
if (!array_isspace(*ptr++))
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str)));
}
/* special case for an empty array */
--- 657,664 ----
if (!array_isspace(*ptr++))
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
! errmsg("malformed array literal: \"%s\"", str),
! errdetail("Junk after closing right brace.")));
}
/* special case for an empty array */
*************** ReadArrayStr(char *arrayStr,
*** 718,724 ****
* character.
*
* The error checking in this routine is mostly pro-forma, since we expect
! * that ArrayCount() already validated the string.
*/
srcptr = arrayStr;
while (!eoArray)
--- 745,752 ----
* character.
*
* The error checking in this routine is mostly pro-forma, since we expect
! * that ArrayCount() already validated the string. So we don't bother
! * with errdetail messages.
*/
srcptr = arrayStr;
while (!eoArray)
diff --git a/src/test/regress/expected/arrays.out b/src/test/regress/expected/arrays.out
index cb606af..d33c9b9 100644
*** a/src/test/regress/expected/arrays.out
--- b/src/test/regress/expected/arrays.out
*************** select '{{1,{2}},{2,3}}'::text[];
*** 1065,1090 ****
--- 1065,1096 ----
ERROR: malformed array literal: "{{1,{2}},{2,3}}"
LINE 1: select '{{1,{2}},{2,3}}'::text[];
^
+ DETAIL: Unexpected "{" character.
select '{{},{}}'::text[];
ERROR: malformed array literal: "{{},{}}"
LINE 1: select '{{},{}}'::text[];
^
+ DETAIL: Unexpected "}" character.
select E'{{1,2},\\{2,3}}'::text[];
ERROR: malformed array literal: "{{1,2},\{2,3}}"
LINE 1: select E'{{1,2},\\{2,3}}'::text[];
^
+ DETAIL: Unexpected "\" character.
select '{{"1 2" x},{3}}'::text[];
ERROR: malformed array literal: "{{"1 2" x},{3}}"
LINE 1: select '{{"1 2" x},{3}}'::text[];
^
+ DETAIL: Unexpected array element.
select '{}}'::text[];
ERROR: malformed array literal: "{}}"
LINE 1: select '{}}'::text[];
^
+ DETAIL: Junk after closing right brace.
select '{ }}'::text[];
ERROR: malformed array literal: "{ }}"
LINE 1: select '{ }}'::text[];
^
+ DETAIL: Junk after closing right brace.
select array[];
ERROR: cannot determine type of empty array
LINE 1: select array[];
pgsql-hackers by date: