Skip to content

[JsonStreamer] Add include_nullable_properties option #60730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: 7.4
Choose a base branch
from
Open

[JsonStreamer] Add include_nullable_properties option #60730

wants to merge 1 commit into from
+255 −113

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Jun 6, 2025 •
edited
Loading

Q A
Branch? 7.4
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

A really common use case in APIs is to be able to skip properties with null value (for instance, this use case is required to make the JsonStreamer component working with API Platform).

This PR adds the option skip_null_properties that allow to skip null properties during stream writing.
This PR skips properties with nulln values during stream writing.
This PR adds the option include_null_properties that allow to encode null properties during stream writing.

This requires to define dynamic object prefixes (such as '' and ','). That's why the PHP generated code has been updated to merge as many prefixes as possible in raw strings and therefore yield a bit bigger chunks.

@carsonbot carsonbot added this to the 7.4 milestone Jun 6, 2025
@soyuka
Copy link
Contributor

soyuka commented Jun 6, 2025

I'm wondering if this shouldn't actually be the default, I would even not add this as an option as this would simplify the code if a value is null we just skip it. The JsonStreamer should be used for maximum performances, null values just add noise and have rather few impact when used in Javascript as they'll just get evaluated as undefined.

@mtarld mtarld force-pushed the feat/skip-nullable branch from 2b0f89a to 7982333 Compare June 7, 2025 08:16
@mtarld
Copy link
Contributor Author

mtarld commented Jun 7, 2025

I agree with @soyuka; in my opinion, the less noise we have, the better. I updated the PR in that way.

@mtarld mtarld changed the title [JsonStreamer] Allow to skip properties with null value [JsonStreamer] Skip properties with null value Jun 7, 2025
@nicolas-grekas
Copy link
Member

Let's target 7.3 then

@mtarld
Copy link
Contributor Author

mtarld commented Jun 27, 2025

This PR needs #60544, which has been merged in 7.4.
Maybe we can backport it - as it is only internal changes?

nicolas-grekas
nicolas-grekas approved these changes Jun 27, 2025
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Go for 7.4 then

@stof
Copy link
Member

stof commented Jun 27, 2025

Omitting a property from the JSON or setting it to null is not equivalent, and might behave differently:

  • it is observable in Javascript, which has undefined and null as different types (and if you do a.myProp === null, it won't be true for undefined)
  • it is different in term of OpenAPI or json-schema specs (it is possible to have a nullable required property in such spec)
  • it might have an impact for parsers in other languages that might be strict about such properties

Always stripping null properties from the output is a no-go to me, as it makes the component incompatible with some kinds of output (and this would be a BC break for projects that rely on those cases)

@alexandre-daubois
Copy link
Member

alexandre-daubois commented Jun 27, 2025 •
edited
Loading

I agree with @stof: consuming APIs with fields that can sometimes appear or disappear can really be disturbing. That's also true if a documentation is provided: the response may be different from what's written in the doc because some fields are missing.

To me, it is semantically really different to have a null value or no value at all and it may not be treated the same way in underlying code. The option you proposed first, skip_null_properties, looks better to me.

stof
stof reviewed Jun 27, 2025
View reviewed changes
src/Symfony/Component/JsonStreamer/Write/PhpGenerator.php
@@ -304,7 +345,7 @@ private function flushYieldBuffer(array $context): string
$yieldBuffer = $this->yieldBuffer;
$this->yieldBuffer = '';

return $this->yield("'$yieldBuffer'", $context);
return $this->yield('"'.$yieldBuffer.'"', $context);
Copy link
Member

Choose a reason for hiding this comment

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

be careful. Using double quotes unlocks many escape sequences in PHP, and yieldString does not perform any escaping (except for $ now) so it expects the caller to pass a string being the content of a string literal, instead of being the content of the string it wants to yield.
Are we sure that there is nothing impacted by that ? We might have injection of escape sequences rather than injection of interpolation.

Btw, yieldString might need to be renamed if it is not about yielding the given string (taking care internally of turning it into a PHP string literal with proper escaping) but about yielding some string literal instead (where the caller has to deal with escaping), in order to reduce the risk of confusion for future contributors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we sure that there is nothing impacted by that ? We might have injection of escape sequences rather than injection of interpolation.

I'm deliberately using an interpolated string because it allows yielding larger, already-assembled chunks instead of many smaller fragments, which improves performance by reducing the number of yield operations.

Btw, yieldString might need to be renamed [...]

The function is now called yieldInterpolatedString to make it explicit that the string is not a literal (and will be escaped)

yieldString does not perform any escaping (except for $ now) [...]

I now escape the string like the following:

$string = addcslashes($string, "\\\"\n\r\t\v\e\f");

if ($escapeDollar) {
    $string = addcslashes($string, '$');
}

(pulled for the table from php documentation

@soyuka
Copy link
Contributor

soyuka commented Jun 27, 2025

I see your point, I'm convinced the null type may be required in JSON. I still think that the default for this component should be that it's disabled by default. From an architecture point of view, when it comes to resource oriented APIs or JSON-LD, we don't recommend the use of null values, it adds unnecessary noise and adds complexity to front ends.
Also as we're all about performances and band-width consumption when it comes to streaming I think that we should go with the less the better.

@norkunas norkunas mentioned this pull request Jun 30, 2025
Closed
@mtarld mtarld force-pushed the feat/skip-nullable branch from 7982333 to 52fd94a Compare July 1, 2025 08:18
@mtarld mtarld changed the title [JsonStreamer] Skip properties with null value [JsonStreamer] Add include_nullable_properties option Jul 1, 2025
@mtarld
Copy link
Contributor Author

mtarld commented Jul 1, 2025

You're right @stof, omitting a property from the JSON or setting it to null is not the same.
Therefore, I reintroduced the option, but inverted it so that the default will be to omit null properties (the option became include_nullable_properties).

@mtarld mtarld force-pushed the feat/skip-nullable branch from 52fd94a to 20821d6 Compare July 1, 2025 08:22
@mtarld mtarld force-pushed the feat/skip-nullable branch from 20821d6 to 48b755d Compare July 1, 2025 08:34
@alexandre-daubois
Copy link
Member

Ah yes! I wanted to comment back to suggest an opt-in to include null values but forgot about it. It seems to be the good compromise!

@soyuka
Copy link
Contributor

soyuka commented Jul 1, 2025

What about include_null_values instead of include_nullable_properties?

@mtarld
Copy link
Contributor Author

mtarld commented Jul 1, 2025

This might be misleading, IMO.
For example, if we want to stream [null, null, null], without include_null_values, what can we expect? [null, null, null]? []? "property" here is important to me.

@alexandre-daubois
Copy link
Member

alexandre-daubois commented Jul 1, 2025 •
edited
Loading

include_nullable_properties gives the impression that if set to false, properties that could be null will not be included in the output. That's why I'm a bit reluctant for this naming

@mtarld
Copy link
Contributor Author

mtarld commented Jul 1, 2025

I see your point, therefore what about include_null_properties, or maybe include_null_property_values?

@alexandre-daubois
Copy link
Member

alexandre-daubois commented Jul 1, 2025 •
edited
Loading

Both removes the ambiguity, definitely better! Maybe the first one is more accurate (because you include the property and its value, not only the value)

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

@stof stof stof left review comments

@alexandre-daubois alexandre-daubois alexandre-daubois left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Feature JsonStreamer Status: Reviewed
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.
6 participants
@mtarld @soyuka @nicolas-grekas @stof @alexandre-daubois @carsonbot