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

Already on GitHub? Sign in to your account

[WIP] Add request content deserializer #249

Closed
wants to merge 22 commits into
from

Conversation

Projects
None yet
9 participants

orfin commented Jun 15, 2012

Hi,

this addon allows to use DataTransferObjects as an argument of controller actions. It's based on Matthias Noback ParamConverter [http://php-and-symfony.matthiasnoback.nl/2012/03/symfony2-creating-a-paramconverter-for-deserializing-request-content/].

It is very convenient in post methods.

Sample usage:

Controller:

// ...
public function postTestsAction(Test $client) { 
// ... 
}

Test class:

<?php

namespace Acme\DemoBundle\Model;

use FOS\RestBundle\Request\DataTransferObject;
use JMS\SerializerBundle\Annotation\XmlRoot;
use JMS\SerializerBundle\Annotation\Type;

/**
 * @XmlRoot("test") 
 */
class Test extends DataTransferObject {

    /**
     * @Type("string") 
     */
    public $field_one;

    /**
     * @Type("boolean") 
     */
    public $field_two;

    public function __construct() {

    }

}

Objects must extends DataTransferObject due to "RestActionReader" mechanism (otherwise controller's actions params will be included in the route).

By default it's turned off.

To turn it on just add to your config:

fos_rest:
  request_content_param_converter: ~    

In default mode, exceptions from Serializer are 'hidden' and 'empty' new object of given type is created. But if you add:

fos_rest:
  request_content_param_converter:
        exception_on_fault: true

all serializer exceptions will be thrown.

TODO:

  • missing documentation
  • need tests!

btw. This is my first github pull request ;-)

@stof stof and 1 other commented on an outdated diff Jun 16, 2012

Request/DataTransferObject.php
+ *
+ * (c) FriendsOfSymfony <http://friendsofsymfony.github.com/>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace FOS\RestBundle\Request;
+
+/**
+ * DataTransferObject
+ *
+ * All objects which you want to deserialize from the Request content
+ * must extend DataTransferObject.
+ *
+ * Instances should accept empty constructor.
@stof

stof Jun 16, 2012

Owner

why this restriction ?

@stof

stof Jun 16, 2012

Owner

especially as you never use this

@orfin

orfin Jun 16, 2012

This restriction is connected to imagin/FOSRestBundle@ccfd53b . This follows from the behaviour of FOSRest routes generator - all arguments of a controller method which aren't instances of Request or QueryFetcherInterface will be added as a route parameters. I had to somehow mark DTObjects to not be involved into route.

@stof

stof Jun 16, 2012

Owner

but an empty base class is a hard requirement as PHP does not have multiple inheritance

@orfin

orfin Jun 16, 2012

Yes, im aware of that 'weird requirement' but I didnt have any idea how to pass FOSRest route generator limitation.
Anyway: multiple inheritance won't be neccessery as only the parent class must inherit DTO.
(notice line: https://github.com/imagin/FOSRestBundle/blob/17df9f878fae7c756b48999332dc52c6b2c4b9ba/Routing/Loader/Reader/RestActionReader.php#L272

$argumentClass->isSubclassOf($class)

(btw is there better way on github to add link to a code fragment?

@orfin

orfin Jun 16, 2012

Ok, probably we can check if on an argument is set ParamConverter annotation. I will try to make it that way.

@stof

stof Jun 16, 2012

Owner

@orfin it means that all the classes you want to use this way need to extend DTO. What if you want to use a third party class as parent ? you can't.

As it is empty, using an interface would be better

Owner

stof commented Jun 16, 2012

And please follow the Symfony2 coding standards for the code.

Owner

stof commented Jun 16, 2012

@orfin many more CS violations are left in your code. Most of them can probably be fixed by running the PHP-CS-Fixer on your file.

@stof stof and 1 other commented on an outdated diff Jun 16, 2012

Request/ParamConverter/RequestContentParamConverter.php
+ $this->serializer = $serializer;
+ $this->options = $options;
+ }
+
+ /**
+ * Only objects are supported.
+ *
+ * @param ConfigurationInterface $configuration
+ * @return boolean
+ */
+ public function supports(ConfigurationInterface $configuration) {
+ if (!$configuration->getClass()) {
+ return false;
+ }
+
+ return true;
@stof

stof Jun 16, 2012

Owner

this is wrong. You should not support any class here as you require extending DTO

@stof stof commented on an outdated diff Jun 16, 2012

DependencyInjection/FOSRestExtension.php
@@ -137,6 +137,17 @@ public function load(array $configs, ContainerBuilder $container)
$container->setParameter($this->getAlias().'.query_fetch_listener.set_params_as_attributes', true);
}
}
+
+ if (!empty($config['request_content_param_converter'])) {
+ if ($config['request_content_param_converter']['enabled']) {
+ $loader->load('request_content_param_converter.xml');
+
+ // Default - false
@stof

stof Jun 16, 2012

Owner

this is useless (and error-prone as it is likely to be forgotten if the default value is modified)

@stof stof and 2 others commented on an outdated diff Jun 16, 2012

Request/DataTransferObjectInterface.php
+ * (c) FriendsOfSymfony <http://friendsofsymfony.github.com/>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace FOS\RestBundle\Request;
+
+/**
+ * All objects which you want to deserialize from the Request content
+ * must implements DataTransferObjectInterface.
+ *
+ * Instances should accept empty constructor.
+ *
+ * @author Antoni Orfin <a.orfin@imagin.com.pl>
+ * @copyright (c) 2012 IMAGIN IT Marcin Engelmann
@stof

stof Jun 16, 2012

Owner

you should remove this. We don't want extra copyright for each file of the library depending of who contributed it (we keep extra copyright header only when the code is copied from an existing library)

@orfin

orfin Jun 16, 2012

So what is a proper way to mark that I've written it as an employee of IMAGIN IT?

@lsmith77

lsmith77 Jun 17, 2012

Owner

doesn't your email address show this sufficiently?

@stof stof commented on an outdated diff Jun 16, 2012

Request/ParamConverter/RequestContentParamConverter.php
+use Symfony\Component\HttpFoundation\Request;
+use JMS\SerializerBundle\Serializer\SerializerInterface;
+use JMS\SerializerBundle\Exception\Exception;
+
+/**
+ * It supports deserializing content of the request into needed object.
+ *
+ * Based on http://php-and-symfony.matthiasnoback.nl/2012/03/symfony2-creating-a-paramconverter-for-deserializing-request-content/
+ * by Matthias Noback (http://php-and-symfony.matthiasnoback.nl)
+ *
+ * @author Antoni Orfin <a.orfin@imagin.com.pl>
+ * @copyright (c) 2012 IMAGIN IT Marcin Engelmann
+ */
+class RequestContentParamConverter implements ParamConverterInterface
+{
+
@stof

stof Jun 16, 2012

Owner

you should remove this extra empty line

@stof stof commented on the diff Jun 16, 2012

Request/ParamConverter/RequestContentParamConverter.php
+ * file that was distributed with this source code.
+ */
+
+namespace FOS\RestBundle\Request\ParamConverter;
+
+use Sensio\Bundle\FrameworkExtraBundle\Request\ParamConverter\ParamConverterInterface;
+use Sensio\Bundle\FrameworkExtraBundle\Configuration\ConfigurationInterface;
+use Symfony\Component\HttpFoundation\Request;
+use JMS\SerializerBundle\Serializer\SerializerInterface;
+use JMS\SerializerBundle\Exception\Exception;
+
+/**
+ * It supports deserializing content of the request into needed object.
+ *
+ * Based on http://php-and-symfony.matthiasnoback.nl/2012/03/symfony2-creating-a-paramconverter-for-deserializing-request-content/
+ * by Matthias Noback (http://php-and-symfony.matthiasnoback.nl)
@stof

stof Jun 16, 2012

Owner

you should add another @author annotation for Matthias

@stof stof commented on an outdated diff Jun 16, 2012

Request/ParamConverter/RequestContentParamConverter.php
+ *
+ * @param ConfigurationInterface $configuration
+ * @return boolean
+ */
+ public function supports(ConfigurationInterface $configuration)
+ {
+ $class = $configuration->getClass();
+
+ if (!$class) {
+ return false;
+ }
+
+ $argumentClass = new \ReflectionClass($class);
+
+ $interfaceNames = $argumentClass->getInterfaceNames();
+ $hasInterface = in_array('FOS\RestBundle\Request\DataTransferObjectInterface', $interfaceNames);
@stof

stof Jun 16, 2012

Owner

you should use implementInterface instead of using an in_array check in PHP. The Reflection code is probably optimized for the lookup.

@stof stof commented on an outdated diff Jun 16, 2012

Request/ParamConverter/RequestContentParamConverter.php
+ : $request->getFormat($request->headers->get('Content-Type'));
+
+ try {
+ $object = $this->serializer->deserialize(
+ $request->getContent(), $class, $format
+ );
+ } catch (Exception $e) {
+
+ /**
+ * If request_content_param_converter.exception_on_fault is set to false
+ * don't throw Exception but new Object (using empty constructor)
+ */
+
+ if (isset($this->options['exception_on_fault']) && $this->options['exception_on_fault'] === true) {
+ throw $e;
+ } else {
@stof

stof Jun 16, 2012

Owner

no need to use else as the if leaves the flow of the function

@stof stof commented on an outdated diff Jun 16, 2012

Request/ParamConverter/RequestContentParamConverter.php
+ $format = null === $contentType
+ ? $request->getRequestFormat()
+ : $request->getFormat($request->headers->get('Content-Type'));
+
+ try {
+ $object = $this->serializer->deserialize(
+ $request->getContent(), $class, $format
+ );
+ } catch (Exception $e) {
+
+ /**
+ * If request_content_param_converter.exception_on_fault is set to false
+ * don't throw Exception but new Object (using empty constructor)
+ */
+
+ if (isset($this->options['exception_on_fault']) && $this->options['exception_on_fault'] === true) {
@stof

stof Jun 16, 2012

Owner

why using an array of options and then doing isset checks when you only need one param ? pass the boolean directly as a property

@stof stof and 1 other commented on an outdated diff Jun 16, 2012

Routing/Loader/Reader/RestActionReader.php
@@ -267,7 +273,12 @@ private function getMethodArguments(\ReflectionMethod $method)
$argumentClass = $argument->getClass();
if ($argumentClass) {
foreach ($ignoreClasses as $class) {
- if ($argumentClass->getName() === $class || $argumentClass->isSubclassOf($class)) {
+ $interfaceNames = $argumentClass->getInterfaceNames();
+
+ $interfacesIntersect = array_intersect($ignoreInterfaces, $interfaceNames);
+ $hasInterface = !empty($interfacesIntersect);
@stof

stof Jun 16, 2012

Owner

This is wrong. If there is several ignored classes, you will do the interface check several times.. You should add a second foreach loop over the ignored interfaces and use implementInterface in it.

@orfin

orfin Jun 16, 2012

Yep, done by mistake ;-)

Happy to see this! I will soon take a look at the code myself, since I have used the SerializedParamConverter in production and have some experience with it. Implementing the DataTransferObjectInterface does not seem to me like a very good idea, especially since it does nothing but mark the class as deserializable as a controller argument. It suffices to give the param converter a very low priority (like the -100 you find in the service definition), so other converters can do their work and when no other converter was fit for the job, the SerializedParamConverter tries to deserialize the request content.

By the way, my email address for the "@author" line would be matthiasnoback@gmail.com.

@lsmith77 lsmith77 and 1 other commented on an outdated diff Jun 20, 2012

DependencyInjection/FOSRestExtension.php
@@ -137,6 +137,16 @@ public function load(array $configs, ContainerBuilder $container)
$container->setParameter($this->getAlias().'.query_fetch_listener.set_params_as_attributes', true);
}
}
+
+ if (!empty($config['request_content_param_converter'])) {
@lsmith77

lsmith77 Jun 20, 2012

Owner

the following if should just be "and'ed" into this one

@orfin

orfin Jun 22, 2012

Isn't it better readable with two ifs? (line of code will be very long if I'll use one if)

@lsmith77

lsmith77 Jun 22, 2012

Owner

well you can actually just do if (!empty($config['request_content_param_converter']['enabled'])) { alone

@orfin

orfin Jun 22, 2012

Script will crash if 'request_content_param_converter' key will be undefined won't it? ;-)

@lsmith77

lsmith77 Jun 22, 2012

Owner

it would not :)

<?php

var_dump(!empty($config['request_content_param_converter']['enabled']));
@lsmith77

lsmith77 Jun 22, 2012

Owner

isset/empty are special like that :)

@orfin

orfin Jun 22, 2012

yea I forgot about this behaviour. fixed! :P

@lsmith77 lsmith77 and 1 other commented on an outdated diff Jun 20, 2012

Resources/config/request_content_param_converter.xml
@@ -0,0 +1,24 @@
+<?xml version="1.0" ?>
+
+<container xmlns="http://symfony.com/schema/dic/services"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
+
+ <parameters>
+
+ <parameter key="fos_rest.request.param_converter.request_content_param_converter.class">FOS\RestBundle\Request\ParamConverter\RequestContentParamConverter</parameter>
@lsmith77

lsmith77 Jun 20, 2012

Owner

fos_rest.request.param_converter.request_content_param_converter seems a bit redundant

why not just fos_rest.request.param_converter

@orfin

orfin Jun 22, 2012

hmh maybe better will be fos_rest.request.param_converter.request_content because what if later someone will have an idea of making another param_converters?

@lsmith77

lsmith77 Jun 22, 2012

Owner

sounds better indeed

@lsmith77 lsmith77 commented on an outdated diff Jun 20, 2012

Request/ParamConverter/RequestContentParamConverter.php
+ *
+ * @throws Exception Only if request_content_param_converter.exception_on_fault is set to true
+ */
+ public function apply(Request $request, ConfigurationInterface $configuration)
+ {
+ $class = $configuration->getClass();
+
+ $contentType = $request->headers->get('Content-Type');
+
+ $format = null === $contentType
+ ? $request->getRequestFormat()
+ : $request->getFormat($request->headers->get('Content-Type'));
+
+ try {
+ $object = $this->serializer->deserialize(
+ $request->getContent(), $class, $format
@lsmith77

lsmith77 Jun 20, 2012

Owner

indenting is off here ..

@lsmith77 lsmith77 commented on the diff Jun 20, 2012

Request/ParamConverter/RequestContentParamConverter.php
+
+ /**
+ * Deserialize request content to object.
+ *
+ * @param Request $request
+ * @param ConfigurationInterface $configuration
+ *
+ * @throws Exception Only if request_content_param_converter.exception_on_fault is set to true
+ */
+ public function apply(Request $request, ConfigurationInterface $configuration)
+ {
+ $class = $configuration->getClass();
+
+ $contentType = $request->headers->get('Content-Type');
+
+ $format = null === $contentType
@lsmith77

lsmith77 Jun 20, 2012

Owner

the format should be determined via https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/EventListener/FormatListener.php#L101

aka imho this code should rely on the format being set properly. if necessary move the FormatListener up in priority.

@orfin

orfin Jun 22, 2012

Hmmh what if FormatListener will be turned off?

@lsmith77

lsmith77 Jun 22, 2012

Owner

the integration point would be $request->getRequestFormat() .. so if its not enabled .. there would just not be any "smarter" logic to determine the format.

@orfin

orfin Jun 22, 2012

Ok it makes sense ;-) ParamConverter's listener has priority '0' so it will be necessary to change FOS Format Listener priority to hmhm... '2'? Will it be OK?

@lsmith77

lsmith77 Jun 22, 2012

Owner

yeah .. though i would make it 10 or so .. also i can never remember if it needs to be negative or positive to run first :)

@stof

stof Jun 22, 2012

Owner

higher first

@orfin

orfin Jun 24, 2012

Hey. I think that FormatListener isn't apprioprate for this 'job' - it listens for an "Accept" header but in this ParamConverter we need info about "Contet-Type":

  1. User is making request with XML content body and headers: Content-Type: application/xml; Accept: application/json
  2. Serializer can deserialise it from XML format. (bcs Content-Type is set to app/xml)
  3. We are returning some response content
  4. Response content is being serialized into JSON format (bcs of Accept header)

if we would use FormatListener for deciding 'from which format we are gonna to deserialize" in (2) we will pass to deserializator JSON format as a base of deserializating.

So the only way to do it is based on the code from BodyListener (https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/EventListener/BodyListener.php#L53)

@lsmith77

lsmith77 Jun 24, 2012

Owner

yeah you are right. i got the listeners confused. this is more similar to the body decoding listener. that being said then we should imho require the correct content type or do nothing.

note we should maybe support some callback or other extension point to support custom media types.

@orfin

orfin Jun 24, 2012

Yep, listener similiar to a FormatListener should be added (like ContentTypeListener or smthing) with logic similar to the Format one - imho it should be considered as a next issue/pull request :-P

@lsmith77

lsmith77 Jun 24, 2012

Owner

hmm .. maybe you are right.

i also see that you likely used the BodyListener as inspiration:
https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/EventListener/BodyListener.php#L55

But I am not sure about my choice to call back to $request->getRequestFormat() .. i doubt anyone really will send decode able content that cannot set a proper content type.

@orfin

orfin Jun 24, 2012

It's easy to mismatch meaning of Request::getRequestFormat() method - it's about 'in which format should be Response returned' not like 'in which format is request given'. Im not sure if @fabpot named that method properly...
...and not sure what was behind of decision to name it like that.

Anyway it seems that there isn't any Request method like this one but for Content-Type. So it will be worth thinking 'what should content type listener really do - where to set that real content-type'.

//added:
Oh wait, we have MimeTypeListener [https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/EventListener/MimeTypeListener.php] which will do the work.
So why we just don't use: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Request.php#L997 in the ParamConverter AND BodyListener?!

orfin commented Jun 24, 2012

@matthiasnoback - happy to see you there :)
about DTO - the need of this weird limitation is described in comments:

it's all about how FOSRest is generating routes and how to bypass it :-/

Owner

lsmith77 commented Jul 11, 2012

the mime type lister is just about registering formats, not about actually setting the format of the request ... Request::setFormat() is another confusingly named method ..

Owner

lsmith77 commented Jul 11, 2012

can you add some more documentation?

Owner

lsmith77 commented Jul 28, 2012

@beberlei you might have a POV here ..
quite a bit has been done as part of SensioFrameworkExtraBundle and SimpleThingsFormSerializerBundle related to this

Contributor

beberlei commented Jul 28, 2012

this makes sense, but why not add this to jmsserializerbundle?

Owner

lsmith77 commented Nov 17, 2012

ping ..

nifr commented Nov 19, 2012

ping .. Any updates over here ? I need this functionality in almost every of my current projects. Would be really nice to have it merged.

Contributor

borisguery commented Nov 22, 2012

+1 any news on this? It would be really helpful to have this.

Kifah commented Dec 4, 2012

+1 on this

Owner

lsmith77 commented Dec 4, 2012

@orfin seems to be busy .. i am open to this .. but someone needs to wrap this up. also as @beberlei pointed out .. maybe best added to the JMS Serializer.

what would be needed to get this PR moving again? this would be incredibly useful on a few different projects and I'd be willing to jump in and do some work to get it merge-ready.

@tystr tystr referenced this pull request May 31, 2013

Merged

Request Body ParamConverter #463

@lsmith77 lsmith77 added the wip/poc label May 11, 2015

@lsmith77 lsmith77 closed this Jul 10, 2015

@lsmith77 lsmith77 removed the wip/poc label Jul 10, 2015

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