Skip to content

[JsonPath] Add FrameworkBundle integration #60083

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

Open
wants to merge 1 commit into
base: 7.3
Choose a base branch
from
+211 −77
Open

[JsonPath] Add FrameworkBundle integration #60083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use Symfony\Component\HttpClient\HttpClient;
use Symfony\Component\HttpFoundation\Cookie;
use Symfony\Component\HttpFoundation\IpUtils;
use Symfony\Component\JsonPath\JsonPath;
use Symfony\Component\JsonStreamer\StreamWriterInterface;
use Symfony\Component\Lock\Lock;
use Symfony\Component\Lock\Store\SemaphoreStore;
Expand Down Expand Up @@ -184,6 +185,7 @@ public function getConfigTreeBuilder(): TreeBuilder
$this->addWebhookSection($rootNode, $enableIfStandalone);
$this->addRemoteEventSection($rootNode, $enableIfStandalone);
$this->addJsonStreamerSection($rootNode, $enableIfStandalone);
$this->addJsonPathSection($rootNode, $enableIfStandalone);

return $treeBuilder;
}
Expand Down Expand Up @@ -2742,4 +2744,16 @@ private function addJsonStreamerSection(ArrayNodeDefinition $rootNode, callable
->end()
;
}

private function addJsonPathSection(ArrayNodeDefinition $rootNode, callable $enableIfStandalone): void
{
$rootNode
->children()
->arrayNode('json_path')
->info('JsonPath configuration')
->{$enableIfStandalone('symfony/json-path', JsonPath::class)}()
->end()
->end()
;
}
}
14 changes: 14 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
use Symfony\Component\HttpKernel\DataCollector\DataCollectorInterface;
use Symfony\Component\HttpKernel\DependencyInjection\Extension;
use Symfony\Component\HttpKernel\Log\DebugLoggerConfigurator;
use Symfony\Component\JsonPath\JsonPath;
use Symfony\Component\JsonStreamer\Attribute\JsonStreamable;
use Symfony\Component\JsonStreamer\JsonStreamWriter;
use Symfony\Component\JsonStreamer\StreamReaderInterface;
Expand Down Expand Up @@ -474,6 +475,10 @@ public function load(array $configs, ContainerBuilder $container): void
$this->registerJsonStreamerConfiguration($config['json_streamer'], $container, $loader);
}

if ($this->readConfigEnabled('json_path', $container, $config['json_path'])) {
$this->registerJsonPathConfiguration($config['json_path'], $container, $loader);
}

if ($this->readConfigEnabled('lock', $container, $config['lock'])) {
$this->registerLockConfiguration($config['lock'], $container, $loader);
}
Expand Down Expand Up @@ -2119,6 +2124,15 @@ private function registerJsonStreamerConfiguration(array $config, ContainerBuild
}
}

private function registerJsonPathConfiguration(array $config, ContainerBuilder $container, PhpFileLoader $loader): void
{
if (class_exists(JsonPath::class)) {
throw new LogicException('JsonPath support cannot be enabled as the JsonPath component is not installed. Try running "composer require symfony/json-path".');
}

$loader->load('json_path.php');
}

private function registerPropertyInfoConfiguration(array $config, ContainerBuilder $container, PhpFileLoader $loader): void
{
if (!interface_exists(PropertyInfoExtractorInterface::class)) {
Expand Down
21 changes: 21 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/json_path.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\DependencyInjection\Loader\Configurator;

use Symfony\Component\JsonPath\JsonCrawler;
use Symfony\Component\JsonPath\JsonCrawlerInterface;

return static function (ContainerConfigurator $container) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add a functional test to make sure that the services are properly registered and usable? (You can find such tests in Symfony\Bundle\FrameworkBundle\Tests\Functional

$container->services()
->set('json_path.crawler', JsonCrawler::class)
->alias(JsonCrawlerInterface::class, 'json_path.crawler');
};
4 changes: 4 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HtmlSanitizer\HtmlSanitizer;
use Symfony\Component\HttpClient\HttpClient;
use Symfony\Component\JsonPath\JsonPath;
use Symfony\Component\JsonStreamer\JsonStreamWriter;
use Symfony\Component\Lock\Store\SemaphoreStore;
use Symfony\Component\Mailer\Mailer;
Expand Down Expand Up @@ -1014,6 +1015,9 @@ class_exists(SemaphoreStore::class) && SemaphoreStore::isSupported() ? 'semaphor
'json_streamer' => [
'enabled' => !class_exists(FullStack::class) && class_exists(JsonStreamWriter::class),
],
'json_path' => [
'enabled' => !class_exists(FullStack::class) && class_exists(JsonPath::class),
]
];
}

Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@
"symfony/expression-language": "^6.4|^7.0",
"symfony/html-sanitizer": "^6.4|^7.0",
"symfony/http-client": "^6.4|^7.0",
"symfony/json-path": "7.3.*",
"symfony/json-streamer": "7.3.*",
"symfony/lock": "^6.4|^7.0",
"symfony/mailer": "^6.4|^7.0",
"symfony/messenger": "^6.4|^7.0",
Expand All @@ -70,7 +72,6 @@
"symfony/workflow": "^6.4|^7.0",
"symfony/yaml": "^6.4|^7.0",
"symfony/property-info": "^6.4|^7.0",
"symfony/json-streamer": "7.3.*",
"symfony/uid": "^6.4|^7.0",
"symfony/web-link": "^6.4|^7.0",
"symfony/webhook": "^7.2",
Expand Down
31 changes: 31 additions & 0 deletions src/Symfony/Component/JsonPath/CrawlerInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\JsonPath;

use Symfony\Component\JsonPath\Exception\InvalidArgumentException;
use Symfony\Component\JsonPath\Exception\JsonCrawlerException;

/**
* @author Alexandre Daubois <alex.daubois@gmail.com>
*
* @experimental
*/
interface CrawlerInterface
{
/**
* @return list<array|string|float|int|bool|null>
*
* @throws InvalidArgumentException When the data source provided to the crawler cannot be decoded
* @throws JsonCrawlerException When a syntax error occurs in the provided JSON path
*/
public function find(string|JsonPath $query): array;
}
43 changes: 27 additions & 16 deletions src/Symfony/Component/JsonPath/JsonCrawler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
*
* @experimental
*/
final class JsonCrawler implements JsonCrawlerInterface
final class JsonCrawler implements CrawlerInterface, JsonCrawlerInterface
{
private const RFC9535_FUNCTIONS = [
'length' => true,
Expand All @@ -39,16 +39,23 @@ final class JsonCrawler implements JsonCrawlerInterface
];

/**
* @param resource|string $raw
* @param resource|string|array $data
*/
public function __construct(
private readonly mixed $raw,
) {
if (!\is_string($raw) && !\is_resource($raw)) {
throw new InvalidArgumentException(\sprintf('Expected string or resource, got "%s".', get_debug_type($raw)));
public function __construct(private mixed $data = [])
{
if (!\is_string($data) && !\is_resource($data) && !\is_array($data)) {
throw new \TypeError(\sprintf('Argument #1 ($data) must be of type string, array or resource, %s given.', get_debug_type($data)));
}
}

/**
* @param resource|string|array $data
*/
public function fromJson(mixed $data): CrawlerInterface
Copy link
Member

Choose a reason for hiding this comment

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

What about making the method static? And I would question if we really need an interface for it? And if so, if it's name wouldn't better be JsonCrawlerFactoryInterface.

Copy link
Member Author

@alexandre-daubois alexandre-daubois Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Making the method static wouldn't allow us to plug properly with DI and allow further steps like this one.

The "factory" name is not used per this comment and this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, we can have static factories in the DI.

But my main question here is about the aim of that method. In fact, such code will look weird to me:

$foo = (new JsonCrawler('"something"'))
    ->fromJson('"otherThing"');

Instead, static method looks more logical to me (and still allows you to use the builder pattern):

$foo = JsonCrawler::fromJson('"otherThing"')
    >withCustomFunctions(...);

Copy link
Member Author

@alexandre-daubois alexandre-daubois Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In your last snippet, you call withCustomFunctions() yourself, but the goal is to have everything wired correctly so custom functions are automatically registered with an attribute, and you "never" call the function yourself if you use FWB. I'm not sure we'd be able to achieve this with static factories (or any static behavior being used)?

Copy link
Contributor

Choose a reason for hiding this comment

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

You might be able to combine static factories and service configurators maybe?

Copy link
Member

Choose a reason for hiding this comment

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

Hell no, not configurators.
A static factory would miss the point of configuring transformer functions.

But all this makes me think we're thinking out of the blue. Let's not do anything until we actually do have such transformers.

If we have some lessons to take from this PR to prepare the next steps, let's keep that, but let's remove the autowiring/DI part to the moment where we'll really need it.

{
return new self($data);
}

public function find(string|JsonPath $query): array
{
return $this->evaluate(\is_string($query) ? new JsonPath($query) : $query);
Expand All @@ -58,29 +65,33 @@ private function evaluate(JsonPath $query): array
{
try {
$tokens = JsonPathTokenizer::tokenize($query);
$json = $this->raw;
$json = $this->data;

if (\is_resource($this->raw)) {
if (\is_resource($this->data)) {
if (!class_exists(Splitter::class)) {
throw new \LogicException('The JsonStreamer package is required to evaluate a path against a resource. Try running "composer require symfony/json-streamer".');
}

$simplified = JsonPathUtils::findSmallestDeserializableStringAndPath(
$tokens,
$this->raw,
$this->data,
);

$tokens = $simplified['tokens'];
$json = $simplified['json'];
}

try {
$data = json_decode($json, true, 512, \JSON_THROW_ON_ERROR);
} catch (\JsonException $e) {
throw new InvalidJsonStringInputException($e->getMessage(), $e);
}
if (\is_array($json)) {
$current = [$json];
} else {
try {
$data = json_decode($json, true, 512, \JSON_THROW_ON_ERROR);
} catch (\JsonException $e) {
throw new InvalidJsonStringInputException($e->getMessage(), $e);
}

$current = [$data];
$current = [$data];
}

foreach ($tokens as $token) {
$next = [];
Expand Down
10 changes: 2 additions & 8 deletions src/Symfony/Component/JsonPath/JsonCrawlerInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,6 @@

namespace Symfony\Component\JsonPath;

use Symfony\Component\JsonPath\Exception\InvalidArgumentException;
use Symfony\Component\JsonPath\Exception\JsonCrawlerException;

/**
* @author Alexandre Daubois <alex.daubois@gmail.com>
*
Expand All @@ -22,10 +19,7 @@
interface JsonCrawlerInterface
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is our factory and the interface used to be used in service injection.

One of the next step is adding something like withCustomFunctions() that gets a tagged iterator of invokable classes to use in JsonPath filters (the RFC allows custom functions in filters).

Copy link
Member

Choose a reason for hiding this comment

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

I wonder though if that doesn't mean that we should rather split the crawler and it's factory. Currently both concerned are mixed in one class which also means that the crawler needs to allow being constructed without data while that shouldn't be needed if the factory was a different class, right?

Copy link
Member Author

@alexandre-daubois alexandre-daubois Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed. We discussed a bit about it with @nicolas-grekas. We agreed that we're fine having everything in one class as well as having a no-op crawler when created without data (i.e. returning an empty array for all paths). Is there a limitation that you see with this implementation maybe?

I went for the separate factory class at first, but I actually like the fact that if you need a crawler in your service, you can inject JsonCrawlerInterface directly instead of having a factory that then creates the crawler.

Copy link
Member

@GromNaN GromNaN Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Having the 2 interfaces implemented by the same class is not a problem when using interfaces for typing.
But I'm not convinced by the name of JsonCrawlerInterface.

I would prefer CrawlerFromJsonInterface which reuses the method name and gives the intention.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer JsonCrawlerInterface - it feels less convoluted to me, and is explicit about the intended use case - querying json documents.

I also prefer one class to implement the two interfaces. Optimizing the constructor doesn't look worth the split to me.

Copy link
Member

Choose a reason for hiding this comment

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

My main concern with mixing responsibilities in one class is that you could have a crawler in an intermediate state while building and then accidentally pass around the wrong crawler instance which would then behave differently than what you did expect. By separating the crawler and its factory (or probably rather a builder) PHP's type system would save you from such a mistake.

{
/**
* @return list<array|string|float|int|bool|null>
*
* @throws InvalidArgumentException When the JSON string provided to the crawler cannot be decoded
* @throws JsonCrawlerException When a syntax error occurs in the provided JSON path
* @param resource|string|array $data
*/
public function find(string|JsonPath $query): array;
public function fromJson(mixed $data): CrawlerInterface;
}
Loading
Loading