Re: bug in json_to_record with arrays

Tested on 9.4b3, 9.4rc1, 9.5devel

select * from json_to_record('
{"id":1,"val":"josh","valry":["potter","chef","programmer"]}') as r(id
int, val text, valry text[]);

ERROR: missing dimension value

With some experimentation, I can't find any way to convert a JSON array
to an array field using json_to_record or json_to_recordset. I know
this worked back in January, though.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 11/26/2014 11:54 AM, Josh Berkus wrote:
> Tested on 9.4b3, 9.4rc1, 9.5devel
>
> select * from json_to_record('
> {"id":1,"val":"josh","valry":["potter","chef","programmer"]}') as r(id
> int, val text, valry text[]);
>
> ERROR: missing dimension value
>
> With some experimentation, I can't find any way to convert a JSON array
> to an array field using json_to_record or json_to_recordset. I know
> this worked back in January, though.

Lemme take that back, it didn't work. Just checked an old devel snapshot.

Looks like this is not intended to work, so the only bug is that we need
a less confusing error message.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> On 11/26/2014 11:54 AM, Josh Berkus wrote:
>> Tested on 9.4b3, 9.4rc1, 9.5devel
>>
>> select * from json_to_record('
>> {"id":1,"val":"josh","valry":["potter","chef","programmer"]}') as r(id
>> int, val text, valry text[]);
>>
>> ERROR: missing dimension value
>>
>> With some experimentation, I can't find any way to convert a JSON array
>> to an array field using json_to_record or json_to_recordset. I know
>> this worked back in January, though.

> Lemme take that back, it didn't work. Just checked an old devel snapshot.

> Looks like this is not intended to work, so the only bug is that we need
> a less confusing error message.

What's happening is that this string:

["potter","chef","programmer"]

is getting passed to array_in, which of course does not like it because
that's not the I/O syntax for Postgres arrays. Arguably,
populate_record_worker should be smart enough to convert somehow, but
it isn't today. Looks to me like it wouldn't succeed for the comparable
case of converting a sub-object to a Postgres composite type, either.
I'm satisfied with regarding those cases as missing features to be
added later.

As far as your request for a better error message is concerned, I'm a
bit inclined to lay the blame on array_in rather than the JSON code.
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?

regards, tom lane

On 11/26/2014 03:48 PM, Tom Lane wrote:

> Arguably,
> populate_record_worker should be smart enough to convert somehow, but
> it isn't today. Looks to me like it wouldn't succeed for the comparable
> case of converting a sub-object to a Postgres composite type, either.
> I'm satisfied with regarding those cases as missing features to be
> added later.

Right. Before commit a749a23d7af4dba9b3468076ec561d2cbf69af09 it didn't
try by default to treat the value as text, but instead errored out if
the value was an array or object, with an appropriate message. Now we
always try to treat it as text and pass that to the array or composite
input functions, who now get the responsibility of telling us what's wrong.

It might be possible to process such values recursively, but it would be
far from trivial.

>
> As far as your request for a better error message is concerned, I'm a
> bit inclined to lay the blame on array_in rather than the JSON code.
> 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?
>
>

+1

cheers

andrew

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> As far as your request for a better error message is concerned, I'm a
> bit inclined to lay the blame on array_in rather than the JSON code.
> Wouldn't it be better if it said
>
> ERROR: invalid input syntax for array: "["potter","chef","programmer"]"
> DETAIL: Dimension value is missing.

Sounds pretty reasonable to me, but I would just caution that we should
check if that's considered 'leakproof' or not (or, if it is, if it'd
ever possibly leak data it shouldn't or if it would only ever return
information provided by the user).

Otherwise, someone might be able to convince the planner to push it down
below a security qual and expose data from rows which shouldn't be
visible.

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> As far as your request for a better error message is concerned, I'm a
>> bit inclined to lay the blame on array_in rather than the JSON code.
>> Wouldn't it be better if it said
>>
>> ERROR: invalid input syntax for array: "["potter","chef","programmer"]"
>> DETAIL: Dimension value is missing.

> Sounds pretty reasonable to me, but I would just caution that we should
> check if that's considered 'leakproof' or not (or, if it is, if it'd
> ever possibly leak data it shouldn't or if it would only ever return
> information provided by the user).

array_in could only be regarded as leakproof if every element-type input
function it could ever call is also leakproof. Which ain't the case,
so I sure hope it's not marked that way. (Likewise record_in, range_in,
etc.)

regards, tom lane

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.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)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

On 11/28/2014 12:55 PM, Tom Lane wrote:
> * 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?

If we don't fix an error text in an RC, what would we fix in an RC?
That's as minor as things get.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> On 11/28/2014 12:55 PM, Tom Lane wrote:
>> * 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?

> If we don't fix an error text in an RC, what would we fix in an RC?
> That's as minor as things get.

Code-wise, yeah, but it would put some pressure on the translators.

What did you think of the new error texts in themselves?

regards, tom lane

On 12/01/2014 12:28 PM, Josh Berkus wrote:
>
> On 11/28/2014 12:55 PM, Tom Lane wrote:
>> * 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?
>
> If we don't fix an error text in an RC, what would we fix in an RC?
> That's as minor as things get.

I think the idea is that you only fix *major* things in an RC? That
said, I agree that we should fix something so minor.

JD

>

--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, @cmdpromptinc
"If we send our children to Caesar for their education, we should
not be surprised when they come back as Romans."

On 2014-12-01 15:30:06 -0500, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
> > On 11/28/2014 12:55 PM, Tom Lane wrote:
> >> * 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?
>
> > If we don't fix an error text in an RC, what would we fix in an RC?
> > That's as minor as things get.
>
> Code-wise, yeah, but it would put some pressure on the translators.

I think a untranslated, but on the point, error message is better than a
translated confusing one.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12/01/2014 12:30 PM, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> On 11/28/2014 12:55 PM, Tom Lane wrote:
>>> * 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?
>
>> If we don't fix an error text in an RC, what would we fix in an RC?
>> That's as minor as things get.
>
> Code-wise, yeah, but it would put some pressure on the translators.
>
> What did you think of the new error texts in themselves?

I would prefer "invalid input syntax" to "malformed array literal", but
I'll take anything we can backpatch.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> On 12/01/2014 12:30 PM, Tom Lane wrote:
>> Code-wise, yeah, but it would put some pressure on the translators.
>>
>> What did you think of the new error texts in themselves?

> I would prefer "invalid input syntax" to "malformed array literal", but
> I'll take anything we can backpatch.

I think if we're changing them at all, it's about the same cost
no matter what we change them to exactly.

I'd be good with going to "invalid input syntax" if we also change
record_in to match. Anyone have a feeling pro or con about that?

regards, tom lane

On Mon, Dec 1, 2014 at 3:01 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> On 12/01/2014 12:30 PM, Tom Lane wrote:
>>> Code-wise, yeah, but it would put some pressure on the translators.
>>>
>>> What did you think of the new error texts in themselves?
>
>> I would prefer "invalid input syntax" to "malformed array literal", but
>> I'll take anything we can backpatch.
>
> I think if we're changing them at all, it's about the same cost
> no matter what we change them to exactly.
>
> I'd be good with going to "invalid input syntax" if we also change
> record_in to match. Anyone have a feeling pro or con about that?

I don't know. "malformed array literal" is a mighty big clue that you
have a bad "postgres format" array literal and will be well supported
by googling -- this problem isn't new.

merlin

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> On Mon, Dec 1, 2014 at 3:01 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'd be good with going to "invalid input syntax" if we also change
>> record_in to match. Anyone have a feeling pro or con about that?

> I don't know. "malformed array literal" is a mighty big clue that you
> have a bad "postgres format" array literal and will be well supported
> by googling -- this problem isn't new.

Good point about googling, and after all we are already using "malformed
array literal" for a subset of these cases. Let's stick with that
wording.

regards, tom lane