Skip to content

MDEV-38033(backport) / MDEV-40185: JSON_ARRAY_INTERSECT / JSON_OBJECT_FILTER_KEYS / MDEV-39742 11.4 JSON functions not interuptable#5288

Open
grooverdan wants to merge 5 commits into
MariaDB:11.4from
grooverdan:MDEV-39742
Open

MDEV-38033(backport) / MDEV-40185: JSON_ARRAY_INTERSECT / JSON_OBJECT_FILTER_KEYS / MDEV-39742 11.4 JSON functions not interuptable#5288
grooverdan wants to merge 5 commits into
MariaDB:11.4from
grooverdan:MDEV-39742

Conversation

@grooverdan

Copy link
Copy Markdown
Member

MDEV-38033, from #4755 should have been targeting 11.4 rather than 12.3. Simple unmodified backport.

MDEV-40185: JSON_ARRAY_INTERSECT / JSON_OBJECT_FILTER_KEYS - in prepartion for the handling of MDEV-39742 kill handling some basic json error handling constructs needed to be included/corrected.

MDEV-39742 11.4 JSON functions not interuptable completes this for 11.4, noting there's one small json_normalize that will be resolve on the merge up of the 10.11 functions.

abhishek593 and others added 5 commits June 29, 2026 11:49
(11.4 backport from 12.3)

Fix a bug in Json_schema_items::validate where it was missing a
json_skip_level() call. Without this skip, the validator would
incorrectly recurse into non-scalar array elements (like objects)
and try to validate their internal keys/values against the array's
item schema.
Since 24ee5fd the result of the killed_ptr
is part of the je->s.error after json_scan_next.
…ling

JSON_ARRAY_INTERSECT had an obvious flaw that the arguments may have
been swapped internally in fix_length_and_dec resulting in the error
messaging being for the wrong argument. Fixed by keeping track of being
swapped.

JSON_ARRAY_INTERSECT called prepare_json_and_create_hash from the
fix_length_and_dec. It the server can take errors at this point
but if warnings are generated, there must be a return value.

The handling of error message is implemented in multiple functions.

Item_func_json_array_intersect::prepare_json_and_create_hash (and
similar in Item_func_json_object_filter_keys::fix_length_and_dec),

We handle rjson_read errors, and errors to arguments are forced
to a syntax error.

create_hash, used by JSON_ARRAY_INTERSECT and
JSON_OBJECT_FILTER_KEYS ,if returning a true value, has
either a JS error, or a memory allocation error. Because this
is pre-val_str() these need to be errors. Once its an error
null_value= 1 is required.

create_hash needs to handle json parsing errors, potentially
being killed or a timeout occurring too.

It makes sense to handle the other arguments in val_str() as
an error too for consistency.

JSON_OBJECT_TO_ARRAY now results in syntax warning on parsing
a non-object augment.
The JSON functions added in 11.4 where not interuptable
with a KILL QUERY or exceeding the max_statement_time.

* JSON_ARRAY_INTERSECT
* JSON_OBJECT_FILTER_KEYS
* JSON_OBJECT_TO_ARRAY
* JSON_SCHEMA_VALID

This behaviour is corrected by setting the killed_ptr
of the json_engine_t structure.

JSON_KEY_VALUE was added in 11.4, however as it doesn't call
json_next_value(), its processing doesn't check the killed pointer
of the json engine. So its quick anyway.
Same result, just consolidating the implementation.
@grooverdan grooverdan added the MariaDB Foundation Pull requests created by MariaDB Foundation label Jun 29, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances error handling and query interruption support for several JSON functions, including JSON_ARRAY_INTERSECT, JSON_OBJECT_FILTER_KEYS, and JSON_OBJECT_TO_ARRAY, ensuring they raise proper syntax errors instead of silently returning NULL. It also fixes a bug in JSON_SCHEMA_VALID (MDEV-38033) regarding array-of-object validation. The review feedback focuses on simplifying the error-handling control flow in sql/item_jsonfunc.cc by removing redundant goto labels and avoiding convoluted jumps into the middle of conditional blocks.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread sql/item_jsonfunc.cc
Comment on lines +5616 to +5634
if (json_read_value(je1))
goto je_error;

if (!(is_array= (je1->value_type == JSON_VALUE_ARRAY)))
{
je1->s.error= JE_SYN;
goto je_error;
}

if (create_hash(je1, &items, item_hash_inited, &hash_root))
{
if (je1->s.error)
je_error:
report_json_error_level(js, je1, swapped ? 1 : 0,
Sql_condition::WARN_LEVEL_ERROR);
else
my_error(ER_OUT_OF_RESOURCES, MYF(0));
return true;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Jumping into the middle of an if block using goto is highly discouraged as it makes the control flow convoluted and hard to read. We can completely eliminate the goto and the je_error label by handling the errors directly in place.

  if (json_read_value(je1))
  {
    report_json_error_level(js, je1, swapped ? 1 : 0,
                            Sql_condition::WARN_LEVEL_ERROR);
    return true;
  }

  if (!(is_array= (je1->value_type == JSON_VALUE_ARRAY)))
  {
    je1->s.error= JE_SYN;
    report_json_error_level(js, je1, swapped ? 1 : 0,
                            Sql_condition::WARN_LEVEL_ERROR);
    return true;
  }

  if (create_hash(je1, &items, item_hash_inited, &hash_root))
  {
    if (je1->s.error)
      report_json_error_level(js, je1, swapped ? 1 : 0,
                              Sql_condition::WARN_LEVEL_ERROR);
    else
      my_error(ER_OUT_OF_RESOURCES, MYF(0));
    return true;
  }

Comment thread sql/item_jsonfunc.cc
Comment on lines +5829 to 5847
if (json_read_value(&je2))
goto je_error;
if (je2.value_type != JSON_VALUE_ARRAY)
{
je2.s.error= JE_SYN;
goto je_error;
}

if (create_hash(&je2, &items, hash_inited, &hash_root))
{
if (je2.s.error)
report_json_error(js2, &je2, 0);
je_error:
report_json_error_level(js2, &je2, 1,
Sql_condition::WARN_LEVEL_ERROR);
else
my_error(ER_OUT_OF_RESOURCES, MYF(0));
null_value= 1;
return FALSE;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similarly, we can avoid jumping into the middle of the if block here by handling the errors directly in place, which improves readability and maintainability.

  if (json_read_value(&je2))
  {
    report_json_error_level(js2, &je2, 1,
                            Sql_condition::WARN_LEVEL_ERROR);
    null_value= 1;
    return FALSE;
  }
  if (je2.value_type != JSON_VALUE_ARRAY)
  {
    je2.s.error= JE_SYN;
    report_json_error_level(js2, &je2, 1,
                            Sql_condition::WARN_LEVEL_ERROR);
    null_value= 1;
    return FALSE;
  }

  if (create_hash(&je2, &items, hash_inited, &hash_root))
  {
    if (je2.s.error)
      report_json_error_level(js2, &je2, 1,
                              Sql_condition::WARN_LEVEL_ERROR);
    else
      my_error(ER_OUT_OF_RESOURCES, MYF(0));
    null_value= 1;
    return FALSE;
  }

Comment thread sql/item_jsonfunc.cc
Comment on lines +5550 to +5557
if (json_read_value(&je2))
goto je2_error_return;

if (je2.value_type != JSON_VALUE_ARRAY)
{
je2.s.error= JE_SYN;
goto je2_error_return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The je2_error_return label is redundant because je2.s.error is guaranteed to be non-zero in these error paths. We can simplify the control flow by jumping directly to error_return.

  if (json_read_value(&je2))
    goto error_return;

  if (je2.value_type != JSON_VALUE_ARRAY)
  {
    je2.s.error= JE_SYN;
    goto error_return;
  }

Comment thread sql/item_jsonfunc.cc
Comment on lines 5583 to +5587
error_return:
if (je2.s.error)
report_json_error(js2, &je2, 1);
je2_error_return:
report_json_error_level(js2, &je2, swapped ? 0 : 1,
Sql_condition::WARN_LEVEL_ERROR);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Remove the redundant je2_error_return label to simplify the error handling block.

error_return:
  if (je2.s.error)
    report_json_error_level(js2, &je2, swapped ? 0 : 1,
                            Sql_condition::WARN_LEVEL_ERROR);

Comment thread sql/item_jsonfunc.cc
Comment on lines +5767 to +5774
if (json_read_value(&je1))
goto je1_error_return;

if(filter_keys(&je1, str, items))
goto null_return;
if (je1.value_type != JSON_VALUE_OBJECT)
{
je1.s.error= JE_SYN;
goto je1_error_return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The je1_error_return label is redundant because je1.s.error is guaranteed to be non-zero in these error paths. We can simplify the control flow by jumping directly to error_return.

  if (json_read_value(&je1))
    goto error_return;

  if (je1.value_type != JSON_VALUE_OBJECT)
  {
    je1.s.error= JE_SYN;
    goto error_return;
  }

Comment thread sql/item_jsonfunc.cc
Comment on lines 5801 to +5805
error_return:
if (je1.s.error)
report_json_error(js1, &je1, 0);
je1_error_return:
report_json_error_level(js1, &je1, 0,
Sql_condition::WARN_LEVEL_ERROR);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Remove the redundant je1_error_return label to simplify the error handling block.

error_return:
  if (je1.s.error)
    report_json_error_level(js1, &je1, 0,
                            Sql_condition::WARN_LEVEL_ERROR);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

MariaDB Foundation Pull requests created by MariaDB Foundation

Development

Successfully merging this pull request may close these issues.

3 participants