-
-
Notifications
You must be signed in to change notification settings - Fork 887
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.
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
Improve Input/Output with normalizers #2495
Conversation
161f236
to
cd5067c
Compare
1fba45a
to
3408ca9
Compare
{ | ||
return RecoverPasswordInput::class === ($context['input_class'] ?? null); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example will be used in the documentation. @ragboyjr maybe that you have remarks about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is part of the new requirements of input/output classes are that we need to create our own normalizers/denormalizers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm working on a graph to explain how things works but it's definitely better then using persisters and providers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I can't speak for everyone, but my use case with input/output classes would be that I'm creating a custom handler for a specific endpoint. So instead of allowing ApiP to use their defaults, I'm intercepting it.
With messenger enabled, this is incredibly elegant, you set your input class, it gets routed to messenger which then routes to a service tagged as a message handler which is responsible for handling the input dto, creating the resource, persist it, then return it.
Without messenger, I feel like the next best solution is you'd set the input class, create a custom controller for the specific operation, and set api_persist=false since your controller will handle it.
With these new changes, that flow gets a lot more complicated since we'd know need to setup a new normalizer/denormalizer to bring our array data into the input dto, and then route it into the system. I'm not sure business logic should be put into data persisters/providers or serializers.
My goal would be that input/output classes get automatically normalized/denormalized according to the custom services created via messenger or via a custom controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without messenger, I feel like the next best solution is you'd set the input class, create a custom controller for the specific operation, and set api_persist=false since your controller will handle it.
About this, we talked about deprecating api_persist
and things like this in favor of input_class=false
(does the same thing).
My goal would be that input/output classes get automatically normalized/denormalized according to the custom services created via messenger or via a custom controller.
I think that this is definitely feasible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So my understanding with api_persist=false was that it disables the write listener. Why would input_class=false disable that? Also input_class=null
which is the default means use resource_class for input, input_class=SomeClass
means use that for input, input_class=false
means disable the write listener?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a shortcut introduced by @dunglas in e10d658#diff-0170cc702a42034d94c5d83ccadad1deR81. It totally makes sense when you say "output_class=false" it means that you don't want any output, the response will therefore be empty. With input_class=false
it says "don't read my data" (skips the deserialization).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, that makes sense, I think you meant output_class=class
is the same as api_persist=false
not input_class=false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sorry!
3408ca9
to
89dfb20
Compare
I broke a lot of tests, I'll fix them when/if we validate the changes. |
8e6b043
to
c849588
Compare
@soyuka are you still moving forward with the normalizer/denormalizer design for handling input/output classes? |
c849588
to
dfbde56
Compare
Yes it's almost RFR waiting for tests to pass! |
@soyuka Can you provide an example of you foresee the new system working with normalizers? It looks as if this PR expects us to put business logic in normalizers and treat them more like factories or something, was just a bit concerned on that point. |
dfbde56
to
ae5ab60
Compare
I've put my example in a separated commit for you: ae5ab60 I want to use these for the docs after. |
Thanks @soyuka Going off of the examples you provided in that commit, I don't think using normalizers to utilize input/output classes is the best way to integrate these features for a few reasons:
If I may offer an alternative solution (I'd also be very willing to code up this feature and submit a PR), I suggest we should focus on streamlining the configuration for using custom controller actions and/or messenger handlers for the standard ways of utilizing input/output classes per resource operations. Message Handlers, like Custom Controllers are very simple to make and autowire with SF out of the box. Controllers and Message Handlers can easily wrap a domain or application service, and for dx, we'd even be able to expose application services as Controllers or Message Handlers. The moment a user wants to use a custom input class, they are already relinquishing the automation from API Platform in favor of an explicit hand written write model (while not sacrificing features with read model). So utilizing a custom controller action or message handler would better fit with expectations on implementing custom write model. |
Not necessarily, IMO we can extend that (de)normalization principle to something wider, take something in input and give back something else on output. We're already doing this with identifiers for example. Actually, let me illustrate the more complex case we're facing today with input/output classes. Say we have a
We draw multiple diagrams with @dunglas and we ended up with the following: Note that we use every elements of our current chain (validation, persister, provider) without changing them.
There is no persisting logic actually, and indeed the normalization would just return the Input or Output class regarding which side we're on. Or maybe I missed something.
About controller's and messenger handler's, they could be a good alternative but in my above example they can still be added to the stack and Api Platform doesn't depend on them. We need to think a bit outside the symfony's box here because we also don't want to be to tight to symfony, for example Api Platform needs to work with laravel at some point (joins the #2506 event simplification proposal). |
@@ -257,8 +257,8 @@ private function getHydraOperation(string $resourceClass, ResourceMetadata $reso | |||
} | |||
|
|||
$shortName = $resourceMetadata->getShortName(); | |||
$inputClass = $resourceMetadata->getTypedOperationAttribute($operationType, $operationName, 'input_class', null, true); | |||
$outputClass = $resourceMetadata->getTypedOperationAttribute($operationType, $operationName, 'output_class', null, true); | |||
$inputClass = $resourceMetadata->getTypedOperationIOClass($operationType, $operationName, 'input', false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could maybe use constants for input
/ output
?
@@ -42,9 +42,9 @@ final class ItemNormalizer extends AbstractItemNormalizer | |||
private $componentsCache = []; | |||
private $resourceMetadataFactory; | |||
|
|||
public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ResourceMetadataFactoryInterface $resourceMetadataFactory, array $defaultContext = []) | |||
public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ResourceMetadataFactoryInterface $resourceMetadataFactory, array $defaultContext = [], bool $allowDtoClass = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ResourceMetadataFactoryInterface $resourceMetadataFactory, array $defaultContext = [], bool $allowDtoClass = false) | |
public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ResourceMetadataFactoryInterface $resourceMetadataFactory, array $defaultContext = [], bool $allowIOClass = false) |
{ | ||
parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, null, null, false, $defaultContext); | ||
parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, null, null, false, $defaultContext, $allowDtoClass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, null, null, false, $defaultContext, $allowDtoClass); | |
parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, null, null, false, $defaultContext, $allowIOClass); |
src/JsonLd/ContextBuilder.php
Outdated
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getIOResourceContext($object, array $context, int $referenceType = UrlGeneratorInterface::ABS_PATH): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe could we drop the IO
part in the name? It works for any object actually
{ | ||
/** | ||
* Creates a JSON-LD context based on the given object. | ||
* Usually this is used with an Input or Output DTO object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Usually this is used with an Input or Output DTO object. | |
* Usually this is used with an Input or Output objects. |
* | ||
* @author Antoine Bluchet <soyuka@gmail.com> | ||
*/ | ||
interface IOContextBuilderInterface extends ContextBuilderInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anonymous instead of IO? It can be used in a generic way to serialize any object in JSON-LD.
{ | ||
parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, $classMetadataFactory, null, false, $defaultContext); | ||
parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, $classMetadataFactory, null, false, $defaultContext, $allowDtoClass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, $classMetadataFactory, null, false, $defaultContext, $allowDtoClass); | |
parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, $classMetadataFactory, null, false, $defaultContext, $allowIOClass); |
@@ -41,9 +41,9 @@ final class ItemNormalizer extends AbstractItemNormalizer | |||
private $resourceMetadataFactory; | |||
private $contextBuilder; | |||
|
|||
public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, ContextBuilderInterface $contextBuilder, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, array $defaultContext = []) | |||
public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, ContextBuilderInterface $contextBuilder, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, array $defaultContext = [], bool $allowDtoClass = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, ContextBuilderInterface $contextBuilder, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, array $defaultContext = [], bool $allowDtoClass = false) | |
public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, ResourceClassResolverInterface $resourceClassResolver, ContextBuilderInterface $contextBuilder, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, array $defaultContext = [], bool $allowIOClass = false) |
return ['class' => null]; | ||
} | ||
|
||
if (null === $ioAttribute) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be the first test (nitpicking)
$ioAttribute = ['class' => $ioAttribute]; | ||
} | ||
|
||
if (!isset($ioAttribute['name'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use the reflection instead? getShortName()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, it should be done in the factory to allow caching, instead of directly here.
/** | ||
* Get the resource I/O class if any. | ||
*/ | ||
public function getTypedResourceIOClass(string $ioType, $fallback = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove this method and normalize the data in the factory to always populate an array?
@@ -119,7 +137,15 @@ public function normalize($object, $format = null, array $context = []) | |||
*/ | |||
public function supportsDenormalization($data, $type, $format = null) | |||
{ | |||
return $this->localCache[$type] ?? $this->localCache[$type] = $this->resourceClassResolver->isResourceClass($type); | |||
if ('elasticsearch' === $format) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (ApiPlatform\Core\Bridge\Elasticsearch\Serializer\ItemNormalizer::FORMAT === $format)
The problem with our approach is that we breaks the signature of the Symfony normalizer: What we could maybe do is:
I think it will also address @ragboyjr concerns and it's slight change. WDYT? |
ae5ab60
to
066faa4
Compare
OK, @soyuka thank you for the flow chart, it helps me understand a lot more where you guys are coming from. First, @dunglas using transformers to go from DTO -> resource after the denormalization step makes a LOT more sense IMHO. Users wouldn't have to make denormalizers, they could just make transformers, and those could probably setup as simply static methods on the resources to provide that transformation as well. Regrading the flow chart, I think understand why we have the disconnect. From my limited perspective, input classes served as an escape hatch from ApiP for supporting CQRS, the idea being, if you want to create your own service to handle persistence of a resource from an input class, you could. Whereas It looks like you guys are trying to automate a lot of that process with the idea of transformers. The way I envisioned is like the following: The parts in yellow showing the custom code a user would write. ApiP already has the concept of readable/writeable properties which allow for different schemas to be created vs read. I'm not sure how much use input/output transformers would get since this PR doesn't seem to address the use case of making CQRS integration more seamless for the end user. |
066faa4
to
34d9ea2
Compare
features/main/input_output.feature
Outdated
@@ -0,0 +1,212 @@ | |||
Feature: DTO input and output | |||
In order to use an hypermedia API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to use an hypermedia API | |
In order to use a hypermedia API |
995fc20
to
4e14e63
Compare
} catch (InvalidArgumentException $e) { | ||
$context = $this->initContext(\get_class($object), $context); | ||
$rawData = parent::normalize($object, $format, $context); | ||
if (!\is_array($rawData)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless? You return $rawData
in all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that if $rawData
isn't an array it can't be merged with $data
and must be returned directly so might not be useless
$resourceClass = $this->resourceClassResolver->getResourceClass($object, $context['resource_class'] ?? null, true); | ||
} catch (InvalidArgumentException $e) { | ||
$rawData = parent::normalize($object, $format, $context); | ||
if (!\is_array($rawData)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless?
src/JsonLd/ContextBuilder.php
Outdated
*/ | ||
public function getAnonymousResourceContext($object, array $context, int $referenceType = UrlGeneratorInterface::ABS_PATH): array | ||
{ | ||
$id = $context['iri'] ?? '_:'.spl_object_id($object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using spl_object_id()
drops support for PHP 7.1. Otherwise you have to add symfony/polyfill-php72
as dependency...
97ed480
to
9a89a39
Compare
9a89a39
to
8abad25
Compare
features/main/input_output.feature
Outdated
I need to be able to use DTOs on my resources as Input or Output objects. | ||
|
||
@createSchema | ||
Scenario: Create a resource with a custom Input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scenario: Create a resource with a custom Input. | |
Scenario: Create a resource with a custom Input |
features/main/input_output.feature
Outdated
"bat": "test" | ||
} | ||
""" | ||
Then I add "Content-Type" header equal to "application/ld+json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I add "Content-Type" header equal to "application/ld+json" | |
When I add "Content-Type" header equal to "application/ld+json" |
features/main/input_output.feature
Outdated
} | ||
""" | ||
Then the response status code should be 201 | ||
Then I add "Content-Type" header equal to "application/ld+json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I add "Content-Type" header equal to "application/ld+json" | |
When I add "Content-Type" header equal to "application/ld+json" |
} catch (InvalidArgumentException $e) { | ||
$context = $this->initContext(\get_class($object), $context); | ||
$rawData = parent::normalize($object, $format, $context); | ||
if (!\is_array($rawData)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that if $rawData
isn't an array it can't be merged with $data
and must be returned directly so might not be useless
{ | ||
return [ | ||
// no input class defined | ||
[[], null], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe those comments could have been used to give more descriptive errors
'no input class defined' => [[], null],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not necessary an error, more that the metadata will be defined like this.
8abad25
to
0a6d61d
Compare
Move data transform to normalizers
0a6d61d
to
d7885e7
Compare
Merging this, please don't hesitate to leave more comments we can still come back to them before a stable release! |
@soyuka thanks for your work on this PR, it looks like it was quite a challenge! Even though I tried to follow the discussion with @ragboyjr and @dunglas above. It's still not clear how an input_class DTO can be handled by the messenger persister. Let's say I have a I assumed I could set the Before this MR was merged I was able to get this to work, albeit a minor workaround which I no longer can recall. In my understanding after this MR I need to create two resources to work around this. A This feels dirty to me, am I missing the correct approach? |
In fact nothing changed in how resources are persisted. Say we leave the messenger on the side for now. When you send a different representation of a class (eg use an
When you add the
Can you show me the difference between your input and your resource? |
Thank you for your reply @soyka. In my 'CQRS' scenario I would like to handle an instance of the input class as a message. Such that a Now I can set different input classes for both PUT and POST, however in the end the message handler will always receive a For now I have created separate I think the approach @ragboyjr would make much more sense in CQRS-setup. Although I am fully aware that the scope of this project is much wider, it would be nice if there was a proper configuration for it. |
I've found a good solution, it just needs a quick fix see #2590. |
This seems to work! Other users who want to have a similar approach could even alter DataTransformer to handle all input-classes of a specific interface. Then I would not have to write one for each specific message. <?php
namespace App\DataTransformer;
use ApiPlatform\Core\DataTransformer\DataTransformerInterface;
use App\Contract\DomainMessage;
class DomainMessageTransformer implements DataTransformerInterface {
public function transform($object, string $to, array $context = [])
{
return $object;
}
public function supportsTransformation($object, string $to, array $context = []): bool
{
if (\is_object($object)) {
return false;
}
$className = $context['input']['class'] ?? null;
if (is_null($className)) {
return false;
}
return is_subclass_of($className, DomainMessage::class);
}
} |
Exactly! |
It seems (correct me if I'm wrong) this PR introduced a dangerous assumption - Was this change intentional that now |
@kiler129 Don't worry. We've changed it so that it only handles API resources (but also non-resources reachable from a resource). Please try v2.4.0 and let us know if you still have any issues with that. |
Input/Output through normalizers.
Outputs can be used in collections.
Functional tests through behat with input/output specifications by operation and on the resource.
Tests with relations on DTO classes, the Json-ld normalizer now works with any class (it doesn't have to be a ResourceClass anymore).
When a class is not a resource class but gets denormalized, its ResourceMetadata defaults to a shortname based on the DTO classname.
When an IRI is needed, the blank node identifier is set to :
_:md5(serialize($object))
.