mt_jsonb_fix_null_parent is not creating object structure properly

[marcin-brzozowski]

Hi, I've noticed that calling the mt_jsonb_fix_null_parent SQL function as follows does not work properly:

Test with PostgreSQL version: PostgreSQL 14.17

Actual output:

SELECT mt_jsonb_fix_null_parent('{"a": null}'::jsonb, ARRAY['a', 'b', 'x']);

  result   
-----------
 {"a": {}}

Expected output:

SELECT mt_jsonb_fix_null_parent('{"a": null}'::jsonb, ARRAY['a', 'b', 'x']);

      result      
------------------
 {"a": {"b": {}}}

When looking at the source code, it seems to me that the if condition checking for NULL is at fault;

IF retval #> dst_path_segment = 'null'::jsonb THEN

CREATE OR REPLACE FUNCTION mt_jsonb_fix_null_parent(jsonb, text[])
    RETURNS jsonb
    LANGUAGE plpgsql
AS $function$
DECLARE
retval ALIAS FOR $1;
    dst_path ALIAS FOR $2;
    dst_path_segment text[] = ARRAY[]::text[];
    dst_path_array_length integer;
    i integer = 1;
BEGIN
    dst_path_array_length = array_length(dst_path, 1);
    WHILE i <=(dst_path_array_length - 1)
    LOOP
        dst_path_segment = dst_path_segment || ARRAY[dst_path[i]];
        IF retval #> dst_path_segment = 'null'::jsonb THEN
            retval = jsonb_set(retval, dst_path_segment, '{}'::jsonb, TRUE);
        END IF;
        i = i + 1;
    END LOOP;

    RETURN retval;
END;
$function$;

I tried changing this condition to the following, which seems to resolve the issue:

IF retval #> dst_path_segment IS NULL THEN.

Looking forward to your feedback, I'm more than happy to do a PR with a fix for this, but would need some guidance whether this change might back-fire in some edge cases.

I've created a simple SQLFiddle with both versions of the functions (current and "fixed" showing difference in output): https://sqlfiddle.com/postgresql/online-compiler?id=55c69de0-082f-46ab-86da-54a0c4ad97d5

[jakobt]

Hi @marcin-brzozowski

My guess would be that you need both so something like:

IF retval #> dst_path_segment IS NULL OR retval #> dst_path_segment = 'null':jsonb THEN

The reason being that we probably need to support two cases:

1. Where the element doesn't exist
2. Where the element exists but is null valued

And given this small example:

select 1 where '{"a":null}'::jsonb #> '{a}' = 'null'::jsonb
UNION
select 2 WHERE '{}'::jsonb #> '{a}' = 'null'::jsonb
UNION
select 3 WHERE '{"a":null}'::jsonb #> '{a}' IS null
UNION
select 4 WHERE '{}'::jsonb #> '{a}' IS null

Returning
1
4

Then you need both to support both cases which i would think is needed (i haven't dived deep into the context where this is used just based on naming)

[mysticmind]

@marcin-brzozowski I am the author of these mt_jsonb* and the patching functionality. I will take a look and get back.

[mysticmind]

@marcin-brzozowski The function is working as expected, it fixes the higher level parent being null in a nested path. My blog post is incorrectly stating the output to be {"a": {"b": {"x": {}}} and this is correct {"a": {}}. From a patching stand point, after this parent is fixed, the further Marten patching logic will actually insert or update or copy etc accordingly to the operation in. patching.

Edit: There is nested call to this function within the patching logic is called for each path segment within the Marten patching logic, it will fix the parent if it is null.

[mysticmind]

As @jakobt indicated, we can amend this function to have both IF retval #> dst_path_segment IS NULL OR retval #> dst_path_segment = 'null':jsonb THEN.

[mysticmind]

@marcin-brzozowski If you want to send us a PR, please go ahead and do so with the change as IF retval #> dst_path_segment IS NULL OR retval #> dst_path_segment = 'null'::jsonb THEN.

Edit: I rechecked, we can go ahead with this change.

[marcin-brzozowski]

@marcin-brzozowski The function is working as expected, it fixes the higher level parent being null in a nested path. My blog post is incorrectly stating the output to be {"a": {"b": {"x": {}}} and this is correct {"a": {}}. From a patching stand point, after this parent is fixed, the further Marten patching logic will actually insert or update or copy etc accordingly to the operation in. patching.

Edit: There is nested call to this function within the patching logic is called for each path segment within the Marten patching logic, it will fix the parent if it is null.

I had the impression that the mt_jsonb_fix_null_parent was supposed to create all the parent keys along the path. I mean with the current approach if you do something like this:

(pseudocode)
var docWithFixedParent = SELECT mt_jsonb_fix_null_parent('{"a": null}'::jsonb, ARRAY['a', 'b', 'x']);

And then try to set the key

SELECT mt_jsonb_patch(
    docWithFixedParent::jsonb,
    '[
        {
            "type": "set",
            "path": "a.b.x",
            "value": "new_value"
        },
    ]'::jsonb
) AS updated_data;

Then currently, the mt_jsonb_patch would not work, as mt_jsonb_fix_null_parent would only create default {} object for the a key, and b would still be missing.

@mysticmind Changing this If statement will result in the full hierarchy of the path elements be created if missing, except for the last one (x in the above example). Won't this blow up the logic you mentioned in the current Patching implementation?

[mysticmind]

Won't this blow up the logic you mentioned in the current Patching implementation?

I have checked it, the change would work fine since the key is not null any more and will skip through. Actually the change is the right thing.

[marcin-brzozowski]

Ok, cool. Later today I will try to prepare a PR with this change. Thank you so much guys for the feedback!

[mysticmind]

@marcin-brzozowski I deleted a comment which I added so ignore it if you received an email on it.

[mysticmind]

Fixed via #3725