Re: json_query conditional wrapper bug

On 2024-09-04 We 4:10 PM, Andrew Dunstan wrote:
>
> On 2024-09-04 We 6:16 AM, Peter Eisentraut wrote:
>> On 28.08.24 11:21, Peter Eisentraut wrote:
>>> These are ok:
>>>
>>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' without 
>>> wrapper);
>>>   json_query
>>> ------------
>>>   42
>>>
>>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with 
>>> unconditional wrapper);
>>>   json_query
>>> ------------
>>>   [42]
>>>
>>> But this appears to be wrong:
>>>
>>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with 
>>> conditional wrapper);
>>>   json_query
>>> ------------
>>>   [42]
>>>
>>> This should return an unwrapped 42.
>>
>> If I make the code change illustrated in the attached patch, then I 
>> get the correct result here.  And various regression test results 
>> change, which, to me, all look more correct after this patch.  I 
>> don't know what the code I removed was supposed to accomplish, but it 
>> seems to be wrong somehow.  In the current implementation, the WITH 
>> CONDITIONAL WRAPPER clause doesn't appear to work correctly in any 
>> case I could identify.
>
>
> Agree the code definitely looks wrong. If anything the test should 
> probably be reversed:
>
>         wrap = count > 1  || !(
>             IsAJsonbScalar(singleton) ||
>             (singleton->type == jbvBinary &&
> JsonContainerIsScalar(singleton->val.binary.data)));
>
> i.e. in the count = 1 case wrap unless it's a scalar or a binary 
> wrapping a scalar. The code could do with a comment about the logic.
>
> I know we're very close to release but we should fix this as it's a 
> new feature.


I thought about this again.

I don't know what the spec says, but the Oracle docs say:

    Specify |WITH| |CONDITIONAL| |WRAPPER| to include the array wrapper
    only if the path expression matches a single scalar value or
    multiple values of any type. If the path expression matches a single
    JSON object or JSON array, then the array wrapper is omitted.

So I now think the code that's there now is actually correct, and what 
you say appears wrong is also correct.



cheers


andrew


--
Andrew Dunstan
EDB:https://www.enterprisedb.com