8000 [JsonStreamer] Max depth handling and JSON errors by mtarld · Pull Request #59646 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[JsonStreamer] Max depth handling and JSON errors #59646

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
Merged
merged 1 commit into from
Mar 26, 2025
Merged

[JsonStreamer] Max depth handling and JSON errors #59646

merged 1 commit into from
Mar 26, 2025

Conversation

mtarld
Copy link
Contributor
@mtarld mtarld commented Jan 29, 2025 •
edited
Loading
Q A
Branch? 7.3
Bug fix? yes
New feature? yes
Deprecations? no
Issues
License MIT

Until now, max depth was handled like the following:

  1. Creating a DataModelNodeInterface that is at most 512 deep, and adding a ExceptionNode when this depth is reached.
  2. Generating an encoder based on that tree, that adds a throw line whenever encountering an ExceptionNode.

But this approach creates some issues:

  • When encoding an object that has a property containing a 512 deep "json encodable data" such as a very nested array, this won't throw any exception even though the encoded data is 513 deep.
  • The generated encoders are very big files in case of self-referencing objects:
return static function (mixed $data, \Psr\Container\ContainerInterface $normalizers, array $options): \Traversable {
    yield '{"self":';
    if (null === $data->self) {
        yield 'null';
    } else {
		yield '{"self":';
		if (null === $data->self->self) {
        	yield 'null';
	    } else {
			// 510 more times until we throw an exception
		}
  		yield '}';
	}
  	yield '}';
};
  • The created DataModelNodeInterface contains useless repeated nodes and is memory-consuming in case of self-referencing objects.

This PR fixes these issues by using another approach:

  • Creating "ghost objects" when encountering an already seen object while constructing a DataModelNodeInterface (like what is done for decoding). These ghosts will prevent nodes to be repeated.
  • Creating "generators" callable in the encoders that are responsible to encode these ghost objects. In that way, encoder PHP files will be way lighter (and will be generated faster, for example the test suite went from 00:03.877 to 00:00.122).
return static function (mixed $data, \Psr\Container\ContainerInterface $normalizers, array $options): \Traversable {
    $generators['Foo'] = static function ($data, $depth) use ($normalizers, $options, &$generators) {
        if ($depth >= 512) {
            throw new NotEncodableValueException('Maximum stack depth exceeded');
        }
        yield '{"@self":';
        if ($data->self instanceof Foo) {
            yield from $generators['Foo']($data->self, $depth + 1);
        } else (null === $data->self) {
            yield 'null';
        }
        yield '}';
    };
    yield from $generators['Foo']($data, 0);
};
  • Use these generators only for ghost objects, as calling functions costs a bit in terms of performances.
  • Specify the proper depth in json_encode depending on the current depth:
return static function (mixed $data, \Psr\Container\ContainerInterface $normalizers, array $options): \Traversable {
    yield '{"@id":';
    yield \json_encode($data->id, depth: 511);
    yield '}';
};

Moreover, JSON encoding errors were not gathered at all.

That's why this PR updates the json_encode call to use the JSON_THROW_ON_ERROR flag, and catches/convert the resulting exceptions to NotEncodableValueException:

return static function (mixed $data, \Psr\Container\ContainerInterface $normalizers, array $options): \Traversable {
    try {
        yield \json_encode($data, \JSON_THROW_ON_ERROR, 512);
    } catch (\JsonException $e) {
        throw new \Symfony\Component\JsonEncoder\Exception\NotEncodableValueException($e->getMessage(), 0, $e);
    }
};

And the MaxDepthException has been converted to a NotEncodableValueException with the "Maximum stack depth exceeded" message to be the same as the native json_encode max depth exception.

@carsonbot carsonbot added this to the 7.3 milestone Jan 29, 2025
nicolas-grekas
nicolas-grekas reviewed Feb 10, 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.

I'm not sure about the term "ghost". "placehoder" maybe instead?

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Feb 10, 2025 •
edited
Loading

(or "mock"?)

@mtarld mtarld changed the title [JsonEncoder] Max depth handling and json errors [JsonStreamer] Max depth handling and JSON errors Mar 3, 2025
@carsonbot carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 26, 2025
@fabpot
Copy link
Member
fabpot commented Mar 26, 2025

Thank you @mtarld.

@fabpot fabpot merged commit f7aae0b into symfony:7.3 Mar 26, 2025
11 checks passed
@mtarld mtarld deleted the feat/max-depth branch March 26, 2025 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

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

Successfully merging this pull request may close these issues.
4 participants
@mtarld @nicolas-grekas 34CC @fabpot @carsonbot
0