Search Postgresql Archives

Re: jsonb_set() strictness considered harmful to data

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/21/19 9:28 AM, Andrew Dunstan wrote:
> On 10/21/19 2:07 AM, Tomas Vondra wrote:
>> On Sun, Oct 20, 2019 at 06:51:05PM -0400, Andrew Dunstan wrote:
>>>> I think the general premise of this thread is that the application
>>>> developer does not realize that may be necessary, because it's a bit
>>>> surprising behavior, particularly when having more experience with
>>>> other
>>>> databases that behave differently. It's also pretty easy to not notice
>>>> this issue for a long time, resulting in significant data loss.
>>>>
>>>> Let's say you're used to the MSSQL or MySQL behavior, you migrate your
>>>> application to PostgreSQL or whatever - how do you find out about this
>>>> behavior? Users are likely to visit
>>>>
>>>>    https://www.postgresql.org/docs/12/functions-json.html
>>>>
>>>> but that says nothing about how jsonb_set works with NULL values :-(
>>>
>>>
>>> We should certainly fix that. I accept some responsibility for the
>>> omission.
>>>
>> +1
>>
>>
>
> So let's add something to the JSON funcs page  like this:
>
>
> Note: All the above functions except for json_build_object,
> json_build_array, json_to_recordset, json_populate_record, and
> json_populate_recordset and their jsonb equivalents are strict
> functions. That is, if any argument is NULL the function result will be
> NULL and the function won't even be called. Particular care should
> therefore be taken to avoid passing NULL arguments to those functions
> unless a NULL result is expected. This is particularly true of the
> jsonb_set and jsonb_insert functions.
>
>
>
> (We do have a heck of a lot of Note: sections on that page)
>
>


For release 13+, I have given some more thought to what should be done.
I think the bar for altering the behaviour of a function should be
rather higher than we have in the present case, and the longer the
function has been sanctioned by time the higher the bar should be.
However, I think there is a case to be made for providing a non-strict
jsonb_set type function. To advance th4e discussion, attached is a POC
patch that does that. This can also be done as an extension, meaning
that users of back branches could deploy it immediately. I've tested
this against release 12, but I think it could go probably all the way
back to 9.5. The new function is named jsonb_ set_lax, but I'm open to
bikeshedding.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 9fe4a4794a..bbe5bbf3ae 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1232,6 +1232,15 @@ LANGUAGE INTERNAL
 STRICT IMMUTABLE PARALLEL SAFE
 AS 'jsonb_set';
 
+CREATE OR REPLACE FUNCTION
+  jsonb_set_lax(jsonb_in jsonb, path text[] , replacement jsonb,
+            create_if_missing boolean DEFAULT true,
+            null_value_treatment text DEFAULT 'use_json_null')
+RETURNS jsonb
+LANGUAGE INTERNAL
+CALLED ON NULL INPUT IMMUTABLE PARALLEL SAFE
+AS 'jsonb_set_lax';
+
 CREATE OR REPLACE FUNCTION
   parse_ident(str text, strict boolean DEFAULT true)
 RETURNS text[]
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 3553a304b8..c9ce8e53e9 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -4395,6 +4395,68 @@ jsonb_set(PG_FUNCTION_ARGS)
 }
 
 
+/*
+ * SQL function jsonb_set_lax(jsonb, text[], jsonb, boolean, text)
+ */
+Datum
+jsonb_set_lax(PG_FUNCTION_ARGS)
+{
+	/* Jsonb	   *in = PG_GETARG_JSONB_P(0); */
+	/* ArrayType  *path = PG_GETARG_ARRAYTYPE_P(1); */
+	/* Jsonb	  *newval = PG_GETARG_JSONB_P(2); */
+	/* bool		create = PG_GETARG_BOOL(3); */
+	text       *handle_null;
+	char       *handle_val;
+
+	if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(3))
+		PG_RETURN_NULL();
+
+	/* could happen if they pass in an explicit NULL */
+	if (PG_ARGISNULL(4))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("need delete_key, return_target, use_json_null, or raise_exception")));
+
+	if (! PG_ARGISNULL(2))
+		return jsonb_set(fcinfo);
+
+	handle_null = PG_GETARG_TEXT_P(4);
+	handle_val = text_to_cstring(handle_null);
+
+	if (strcmp(handle_val,"raise_exception") == 0)
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("null jsonb value")));
+	}
+	else if (strcmp(handle_val, "use_json_null") == 0)
+	{
+		Datum	  newval;
+
+		newval = DirectFunctionCall1(jsonb_in, CStringGetDatum("null"));
+
+		/* XXX can we stomp on fcinfo->args like this? */
+		fcinfo->args[2].value = newval;
+		fcinfo->args[2].isnull = false;
+		return jsonb_set(fcinfo);
+	}
+	else if (strcmp(handle_val, "delete_key") == 0)
+	{
+		return jsonb_delete_path(fcinfo);
+	}
+	else if (strcmp(handle_val, "return_target") == 0)
+	{
+		Jsonb	   *in = PG_GETARG_JSONB_P(0);
+		PG_RETURN_JSONB_P(in);
+	}
+	else
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("need delete_key, return_target, use_json_null, or raise_exception")));
+	}
+}
+
 /*
  * SQL function jsonb_delete_path(jsonb, text[])
  */
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 58ea5b982b..60686c6a75 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -9289,6 +9289,9 @@
 { oid => '3304',
   proname => 'jsonb_delete_path', prorettype => 'jsonb',
   proargtypes => 'jsonb _text', prosrc => 'jsonb_delete_path' },
+{ oid => '8945', descr => 'Set part of a jsonb, handle NULL value',
+  proname => 'jsonb_set_lax', prorettype => 'jsonb', proisstrict => 'f',
+  proargtypes => 'jsonb _text jsonb bool text', prosrc => 'jsonb_set_lax' },
 { oid => '3305', descr => 'Set part of a jsonb',
   proname => 'jsonb_set', prorettype => 'jsonb',
   proargtypes => 'jsonb _text jsonb bool', prosrc => 'jsonb_set' },
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index a2a19f8104..c58efffd46 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -4511,6 +4511,61 @@ select jsonb_set('{"a": {"b": [1, 2, 3]}}', '{a, b, non_integer}', '"new_value"'
 ERROR:  path element at position 3 is not an integer: "non_integer"
 select jsonb_set('{"a": {"b": [1, 2, 3]}}', '{a, b, NULL}', '"new_value"');
 ERROR:  path element at position 3 is null
+-- jsonb_set_lax
+\pset null NULL
+-- pass though non nulls to jsonb_set
+select jsonb_set_lax('{"a":1,"b":2}','{b}','5') ;
+  jsonb_set_lax   
+------------------
+ {"a": 1, "b": 5}
+(1 row)
+
+select jsonb_set_lax('{"a":1,"b":2}','{d}','6', true) ;
+      jsonb_set_lax       
+--------------------------
+ {"a": 1, "b": 2, "d": 6}
+(1 row)
+
+-- using the default treatment
+select jsonb_set_lax('{"a":1,"b":2}','{b}',null);
+    jsonb_set_lax    
+---------------------
+ {"a": 1, "b": null}
+(1 row)
+
+select jsonb_set_lax('{"a":1,"b":2}','{d}',null,true);
+        jsonb_set_lax        
+-----------------------------
+ {"a": 1, "b": 2, "d": null}
+(1 row)
+
+-- errors
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, true, null);
+ERROR:  need delete_key, return_target, use_json_null, or raise_exception
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, true, 'no_such_treatment');
+ERROR:  need delete_key, return_target, use_json_null, or raise_exception
+-- explicit treatments
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment := 'raise_exception') as raise_exception;
+ERROR:  null jsonb value
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment := 'return_target') as return_target;
+  return_target   
+------------------
+ {"a": 1, "b": 2}
+(1 row)
+
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment := 'delete_key') as delete_key;
+ delete_key 
+------------
+ {"a": 1}
+(1 row)
+
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment := 'use_json_null') as use_json_null;
+    use_json_null    
+---------------------
+ {"a": 1, "b": null}
+(1 row)
+
+\pset null
 -- jsonb_insert
 select jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"');
          jsonb_insert          
diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index efd4c45185..bda9cb8a18 100644
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -1153,6 +1153,26 @@ select jsonb_set('{"a": [1, 2, 3]}', '{a, non_integer}', '"new_value"');
 select jsonb_set('{"a": {"b": [1, 2, 3]}}', '{a, b, non_integer}', '"new_value"');
 select jsonb_set('{"a": {"b": [1, 2, 3]}}', '{a, b, NULL}', '"new_value"');
 
+-- jsonb_set_lax
+
+\pset null NULL
+
+-- pass though non nulls to jsonb_set
+select jsonb_set_lax('{"a":1,"b":2}','{b}','5') ;
+select jsonb_set_lax('{"a":1,"b":2}','{d}','6', true) ;
+-- using the default treatment
+select jsonb_set_lax('{"a":1,"b":2}','{b}',null);
+select jsonb_set_lax('{"a":1,"b":2}','{d}',null,true);
+-- errors
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, true, null);
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, true, 'no_such_treatment');
+-- explicit treatments
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment := 'raise_exception') as raise_exception;
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment := 'return_target') as return_target;
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment := 'delete_key') as delete_key;
+select jsonb_set_lax('{"a":1,"b":2}', '{b}', null, null_value_treatment := 'use_json_null') as use_json_null;
+
+\pset null
 
 -- jsonb_insert
 select jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"');

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Postgresql Jobs]     [Postgresql Admin]     [Postgresql Performance]     [Linux Clusters]     [PHP Home]     [PHP on Windows]     [Kernel Newbies]     [PHP Classes]     [PHP Books]     [PHP Databases]     [Postgresql & PHP]     [Yosemite]

  Powered by Linux