Add Allow header in HTTP Response #250

Closed
wants to merge 8 commits into from

5 participants

@borisguery

I create a new because issue #52 is also about relation links.

RESTful responses should include Allow header as stated in rfc2616:

The Allow entity-header field lists the set of methods supported
by the resource identified by the Request-URI.

Note that I'm new to Symfony2 and the way I determine the available methods may not be the most elegant.

@asm89
FriendsOfSymfony member

I like the idea of this, but your current implementation with RouteCollection#all() will trigger a load of the route collection on every request (normally this isn't needed as everything is cached). I guess we'll have to add some kind of cache that keeps the allowed methods for the routes in the system?

@borisguery

Agree, I though about it while I was writing this snippet. I guess we could store an array like this:

array(
    '/path/to/the/resource' => array('GET', 'POST', 'PUT', 'DELETE', 'PATCH');
);

But what kind of strategy would you advise to cache it effectively?

@stof
FriendsOfSymfony member

such array is impossible to build as soon as you have a placeholder in the route

@borisguery

I don't get your point, why would the placeholders would be a problem?

Afaik, an URI to a resource is unique as well as the resource itself meaning that if you have some different pattern including a placeholder it will not match the same uri/resource.

Currently a router:debug tends to prove this:

api_get_user          GET    /api/users/{id}.{_format}
api_put_user          PUT    /api/users/{id}.{_format}
api_get_users         GET    /api/users.{_format}
api_post_users        POST   /api/users.{_format}

However, if you have a more flexible or bullet-proof strategy I'll be happy to know it since I didn't dive in the Router API so far

@asm89
FriendsOfSymfony member

@stof Actually you can because you correlate different routes based on the original pattern. For routes generated by the route loader of this bundle that would work just fine.

@borisguery I think you should be able to create an array like:

<?php
array(
    'api_get_user'  => array('GET', 'PUT'),
    // ..
    'api_get_users' => array('GET', 'POST'),
)

If that array is cached there is no need to load the entire route collection to determine the allowed request methods.

@borisguery

I've written a quick implementation using the CacheWarmerInterface to generate the data once.

Feel free to comment it.

@lsmith77 lsmith77 commented on an outdated diff Jun 20, 2012
CacheWarmer/AllowedHttpMethodsCacheWarmer.php
+ public function __construct(ContainerInterface $container)
+ {
+ $this->container = $container;
+ }
+
+ /**
+ * Checks whether this warmer is optional or not.
+ *
+ * Optional warmers can be ignored on certain conditions.
+ *
+ * A warmer should return true if the cache can be
+ * generated incrementally and on-demand.
+ *
+ * @return Boolean true if the warmer is optional, false otherwise
+ */
+ function isOptional()
@lsmith77
FriendsOfSymfony member

missing public (see also later methods)

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

This is very cool. However I don't think we should use allowed_http_methods as the name, since once we do implement #52 imho it should be down via the same listener.

@stof stof and 1 other commented on an outdated diff Jun 20, 2012
CacheWarmer/AllowedHttpMethodsCacheWarmer.php
+
+use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmer,
+ Symfony\Component\DependencyInjection\ContainerInterface;
+
+/**
+ * CacheWarmer to generate Allow-ed for each routes
+ *
+ * @author Boris Guéry <guery.b@gmail.com>
+ */
+
+class AllowedHttpMethodsCacheWarmer extends CacheWarmer
+{
+
+ private $container;
+
+ public function __construct(ContainerInterface $container)
@stof
FriendsOfSymfony member
stof added a note Jun 20, 2012

you should not inject the full container but only your dependency (the router in such case)

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 2 others commented on an outdated diff Jun 20, 2012
EventListener/AllowedHttpMethodsListener.php
+
+class AllowedHttpMethodsListener
+{
+ /**
+ * @var ContainerInterface
+ */
+ private $container;
+
+ /**
+ * Constructor.
+ *
+ * @param ContainerInterface $container container
+ */
+ public function __construct(ContainerInterface $container)
+ {
+ $this->container = $container;
@stof
FriendsOfSymfony member
stof added a note Jun 20, 2012

please inject your real dependencies instead of depending on the full container

Something like

public function __construct(RouterInterface $router, AllowedHttpMethodsCacheWarmer $cacheWarmer, $kernelCacheDir)

?

@lsmith77
FriendsOfSymfony member

yes

@stof
FriendsOfSymfony member
stof added a note Jun 22, 2012

except you don't need the router :)

hehe that's the end of the week :) done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Jun 20, 2012
DependencyInjection/Configuration.php
@@ -47,6 +47,8 @@ public function getConfigTreeBuilder()
->thenInvalid('The query_fetcher_listener option does not support %s. Please choose one of '.json_encode($this->forceOptionValues))
->end()
->end()
+ ->scalarNode('allowed_http_methods_listener')->defaultTrue()
+ ->end()
@stof
FriendsOfSymfony member
stof added a note Jun 20, 2012

the ->end() call should be at the end of the previous line

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Jun 20, 2012
DependencyInjection/FOSRestExtension.php
@@ -137,6 +137,10 @@ public function load(array $configs, ContainerBuilder $container)
$container->setParameter($this->getAlias().'.query_fetch_listener.set_params_as_attributes', true);
}
}
+
+ if (!empty($config['allowed_http_methods_listener'])) {
@stof
FriendsOfSymfony member
stof added a note Jun 20, 2012

simply if ($config['allowed_http_methods_listener']) { as it will always contain a boolean

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Jun 20, 2012
EventListener/AllowedHttpMethodsListener.php
@@ -0,0 +1,62 @@
+<?php
+
+/*
+ * This file is part of the FOSRestBundle package.
+ *
+ * (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\EventListener;
+
+use Symfony\Component\HttpKernel\Event\FilterResponseEvent,
+ Symfony\Component\HttpKernel\HttpKernelInterface,
@stof
FriendsOfSymfony member
stof added a note Jun 20, 2012

this use statement is useless

indeed :) done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Jun 20, 2012
CacheWarmer/AllowedHttpMethodsCacheWarmer.php
+ $processedRoutes = array();
+
+ foreach ($router->getRouteCollection()->all() as $name => $route) {
+
+ if (!isset($processedRoutes[$route->getPattern()])) {
+ $processedRoutes[$route->getPattern()] = array(
+ 'methods' => array(),
+ 'names' => array(),
+ );
+ }
+
+ $processedRoutes[$route->getPattern()]['names'][] = $name;
+
+ $requirements = $route->getRequirements();
+ if (isset($requirements['_method'])) {
+ $processedRoutes[$route->getPattern()]['methods'][] = $requirements['_method'];
@stof
FriendsOfSymfony member
stof added a note Jun 20, 2012

what about routes having a requirement looking like GET|POST ?

well I didn't think about such pattern, I will fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Jun 20, 2012
Resources/config/allowed_http_methods_listener.xml
+
+ <parameters>
+ <parameter key="fos_rest.allowed_http_methods_listener.class">FOS\RestBundle\EventListener\AllowedHttpMethodsListener</parameter>
+ <parameter key="fos_rest.allowed_http_methods_cache_warmer.class">FOS\RestBundle\CacheWarmer\AllowedHttpMethodsCacheWarmer</parameter>
+ </parameters>
+
+ <services>
+
+ <service id="fos_rest.allowed_http_methods_listener" class="%fos_rest.allowed_http_methods_listener.class%">
+ <tag name="kernel.event_listener" event="kernel.response" method="onKernelResponse" />
+ <argument type="service" id="service_container" />
+ </service>
+
+ <service id="fos_rest.allowed_http_methods_cache_warmer" class="%fos_rest.allowed_http_methods_cache_warmer.class%">
+ <tag name="kernel.cache_warmer" />
+ <argument type="service" id="service_container" />
@stof
FriendsOfSymfony member
stof added a note Jun 20, 2012

wrong indentation

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@borisguery borisguery * Refactored dependencies
* Fixed indentations
* Now allows multiple methods at once (GET|POST)
* Fixed if condition
* Updated service arguments injection
ace9e3b
@borisguery

@lsmith77, concerning the naming, how would you name it? I though of HateoasListener. What about it?

@lsmith77
FriendsOfSymfony member

hmm definitely better .. but i need to ponder that a bit if we wouldn't be selling the concepts of "Hateoas" short with the scope of this listener.

@stof stof commented on an outdated diff Jun 22, 2012
CacheWarmer/AllowedHttpMethodsCacheWarmer.php
+ * file that was distributed with this source code.
+ */
+
+namespace FOS\RestBundle\CacheWarmer;
+
+use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmer,
+ Symfony\Component\Routing\RouterInterface;
+
+/**
+ * CacheWarmer to generate Allow-ed for each routes
+ *
+ * @author Boris Guéry <guery.b@gmail.com>
+ */
+class AllowedHttpMethodsCacheWarmer extends CacheWarmer
+{
+
@stof
FriendsOfSymfony member
stof added a note Jun 22, 2012

please remove the extra empty line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jun 22, 2012
CacheWarmer/AllowedHttpMethodsCacheWarmer.php
+ $processedRoutes[$route->getPattern()] = array(
+ 'methods' => array(),
+ 'names' => array(),
+ );
+ }
+
+ $processedRoutes[$route->getPattern()]['names'][] = $name;
+
+ $requirements = $route->getRequirements();
+ if (isset($requirements['_method'])) {
+ $methods = explode('|', $requirements['_method']);
+ $processedRoutes[$route->getPattern()]['methods'] = array_merge(
+ $processedRoutes[$route->getPattern()]['methods'],
+ $methods
+ );
+ }
@stof
FriendsOfSymfony member
stof added a note Jun 22, 2012

If a route does not have any requirement, it should allow all methods. Currently, it will return no method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jun 22, 2012
EventListener/AllowedHttpMethodsListener.php
+ $this->cacheWarmer = $cacheWarmer;
+ $this->kernelCacheDir = $kernelCacheDir;
+ }
+
+ /**
+ * @param FilterResponseEvent $event
+ */
+ public function onKernelResponse(FilterResponseEvent $event)
+ {
+ $cacheFile = $this->kernelCacheDir . '/fos_rest/allowed_http_methods.php';
+
+ if (!is_file($cacheFile)) {
+ $this->cacheWarmer->warmUp($this->kernelCacheDir);
+ }
+
+ $allowedMethods = require $cacheFile;
@stof
FriendsOfSymfony member
stof added a note Jun 22, 2012

There is an issue here. It will not refresh the cache automatically in debug mode when you do a change.

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

I would move the extraction logic to a class (implementing an interface to allow replacing the implementation if someone wants to get the data from elsewhere than the router) that uses the ConfigCache to cache the data. And then, the listener would only get the values from it without bothering about the way the class caches it. The current implementation does not allow to use the ConfigCache as it splits the handling of the cache in 2 different places.
And then, the cache warming is easy: trigger the logic in other class (which could implement the CacheWarmerInterface itself to avoid adding another class, but this should not be its primary interface)

@borisguery

So you think about something like this:

<?php
interface AllowedMethodsLoaderInterface
{
    function getAllowedMethods();
}
<?php
class AllowedMethodsRouterLoader implements AllowedMethodsLoaderInterface
{

    public function __construct(Router $router, $isDebug = false);

    public function getAllowedMethods()
    {
        $routes = $this->router->getRouteCollections()->all();
        // Process routes and extract methods

        return $allowedMethods;
    }
}

Given your suggestion, I though such decorator could be a good idea because it would ease the implementation of cache in any other loaders.

<?php
class AllowedMethodsConfigCacheLoader implements AllowedMethodsLoaderInterface, CacheWarmerInterface
{
    public function __construct(AllowedMethodsLoaderInterface $loader, $file, $isDebug = false)
    {
        $this->loader = $loader;
        $this->cache = new ConfigCache($file, $isDebug);
    }

    public function getAllowedMethods()
    {
        if (!$this->cache->isFresh()) {
            $this->cache->write(sprintf('<?php return %s;', var_export($loader->getAllowedMethods(), true));
        }

        return require $this->cache;
    }
}

The listener now just needs a loader implements the given interface to fetch the allowed methods.

<?php
class AllowedHttpMethodsListener
{

    public function __construct(AllowedMethodsLoaderInterface $loader)
    {
        $this->loader = $loader;
    }

    public function onKernelResponse(FilterResponseEvent $event)
    {
        // Do something onKernelResponse
    }
}

(Note that I wrote it as-is so it may contains errors)

@stof
FriendsOfSymfony member

The interface indeed looks like what I thought about. but the caching should be done in the router loader itself as it is the only place where you can have a powerful caching, taking care of the resources. A wrapper around another loader simply cannot have enough data to refresh the cache in dev.

@borisguery

@stof I don't get your point on the wrapper, which data do we need to have a powerful caching? The cache loader doesn't need to be aware of where we get the data from.

I actually think that using such decorator helps to don't tie the cache system with the loader itself. If we're using an interface, it is mostly because we suppose we could have several loaders in place of the RouterLoader. If we put cache inside, we will need to implement cache in each loader and it doesn't follow the DRY principle imho.

I may miss a point or two, but the ConfigCache only needs data, a file to write to and a debug flag.

@stof
FriendsOfSymfony member

@borisguery the routing resources. Look at the ConfigCache. If you don't pass the resources when writing, it will never refresh itself, even in debug mode. And this would mean that you would have to clear your cache manually in dev each time you do a change in the routing (which is exactly what we want to avoid)

@borisguery

@stof mm ok got it, I didn't knew about this resources things.

@borisguery borisguery * Moved logic to a Loader class implementing AllowedMethodsLoaderInte…
…rface

* Added cache using ConfigCache and implemented CacheWarmer to ease cache warning in production env
* Shortened AllowedMethods by removing Http
* Added cache_dir config and fos_rest.cache_dir parameter
7224b18
@stof stof commented on an outdated diff Jul 8, 2012
DependencyInjection/FOSRestExtension.php
@@ -41,6 +41,11 @@ public function load(array $configs, ContainerBuilder $container)
$loader->load('util.xml');
$loader->load('request.xml');
+ $container->setParameter(
+ 'fos_rest.cache_dir',
+ $container->getParameterBag()->resolveValue($config['cache_dir'])
@stof
FriendsOfSymfony member
stof added a note Jul 8, 2012

don't resolve it here. It makes no sense as the container does it anyway when needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Jul 8, 2012
...llowedMethodsLoader/AllowedMethodsLoaderInterface.php
@@ -0,0 +1,32 @@
+<?php
+/*
+ * This file is part of the FOSRestBundle package.
+ *
+ * (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\EventListener\AllowedMethodsLoader;
@stof
FriendsOfSymfony member
stof added a note Jul 8, 2012

the EventListener namespace should only contain the listeners. This has nothing to do in a subnamespace of EventListener

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jul 8, 2012
...r/AllowedMethodsLoader/AllowedMethodsRouterLoader.php
+ }
+ }
+
+ $allowedMethods = array();
+
+ foreach ($processedRoutes as $processedRoute) {
+ if (count($processedRoute['methods']) > 0) {
+ foreach ($processedRoute['names'] as $name) {
+ $allowedMethods[$name] = array_unique($processedRoute['methods']);
+ }
+ }
+ }
+
+ $this->cache->write(
+ sprintf('<?php return %s;', var_export($allowedMethods, true)),
+ $this->router->getRouteCollection()->getResources()
@stof
FriendsOfSymfony member
stof added a note Jul 8, 2012

please don't call $router->getRouteCollection() twice. This methods triggers the routing loaders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@borisguery borisguery * Removed parameter resolving
* Store RouteCollection to avoid triggering twice
* Fixed namespaces
608f305
@borisguery

I fixed both comments. Do you have any advices about the namespace?

@lsmith77
FriendsOfSymfony member

it definitely needs a rebase to be merge able according to github

@stof
FriendsOfSymfony member

Well, AllowedMethodsLoader is a good idea, but it should either be directly in the bundle root, or in Request. Definitely not in EventListener. @lsmith77 thoughts ?

@borisguery

@lsmith77 yeah sure, I badly worked in master, I'll create a new branch, rebase and open a new PR to let you merge properly when things will be done.

@lsmith77
FriendsOfSymfony member

sorry .. traveling atm and so i am less organized. i will comment tomorrow ..

@lsmith77 lsmith77 commented on an outdated diff Jul 11, 2012
DependencyInjection/Configuration.php
->scalarNode('query_fetcher_listener')->defaultFalse()
->validate()
->ifNotInArray($this->forceOptionValues)
->thenInvalid('The query_fetcher_listener option does not support %s. Please choose one of '.json_encode($this->forceOptionValues))
->end()
->end()
+ ->scalarNode('allowed_methods_listener')->defaultTrue()->end()
@lsmith77
FriendsOfSymfony member

i think we default all other listeners to false

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

i guess the direction for HATEOS link content is likely going to be done via the serializer as per #246 and not via a listener .. meaning that the naming of this listener doesn't need to be reconsidered. so i think i am ok with this change, but i do think the listener should be off by default like the others.

@borisguery borisguery Updated Loader namespace
* Disabled listener by default
* Fixed doc accordingly
* Fixed config
68ee031
@borisguery

I chose AllowedMethodsLoader as namespace and disabled the listener by default.

If all is ok for you, I'll branch, rebase and open a new PR. Let me know

@lsmith77
FriendsOfSymfony member

ah sorry .. could you move the loader to a Response subnamespace so that we do not clutter the root. but yeah .. haven't tried it .. but i trust @stof's review ..

@stof stof commented on an outdated diff Jul 12, 2012
AllowedMethodsLoader/AllowedMethodsLoaderInterface.php
+/**
+ * AllowedMethodsLoaderInterface
+ *
+ * @author Boris Guéry <guery.b@gmail.com>
+ */
+interface AllowedMethodsLoaderInterface
+{
+ /**
+ * Returns the allowed http methods
+ *
+ * array(
+ * 'some_route' => array('GET', 'POST'),
+ * 'another_route' => array('DELETE', 'PUT'),
+ * );
+ *
+ * @abstract
@stof
FriendsOfSymfony member
stof added a note Jul 12, 2012

please remove this @abstract. It does not make any sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jul 12, 2012
AllowedMethodsLoader/AllowedMethodsRouterLoader.php
+ Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;
+
+/**
+ * AllowedMethodsRouterLoader implementation using RouterInterface to fetch
+ * allowed http methods
+ *
+ * @author Boris Guéry <guery.b@gmail.com>
+ */
+class AllowedMethodsRouterLoader implements AllowedMethodsLoaderInterface, CacheWarmerInterface
+{
+ /**
+ * @var RouterInterface
+ */
+ private $router;
+
+
@stof
FriendsOfSymfony member
stof added a note Jul 12, 2012

you have an extra empty line here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Jul 12, 2012
AllowedMethodsLoader/AllowedMethodsRouterLoader.php
@@ -0,0 +1,114 @@
+<?php
+/*
+ * This file is part of the FOSRestBundle package.
+ *
+ * (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\AllowedMethodsLoader;
+
+use Symfony\Component\Routing\RouterInterface,
+ Symfony\Component\Config\ConfigCache,
+ Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;
@stof
FriendsOfSymfony member
stof added a note Jul 12, 2012

please use one use statement per class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@borisguery borisguery Moved to Response sub-namespace
* fixed code according to CS
a4b981d
@lsmith77
FriendsOfSymfony member

ping .. still needs a rebase

@lsmith77
FriendsOfSymfony member

ok .. i did the rebase and also fixed some minor issues in the docs

@lsmith77 lsmith77 closed this Jul 28, 2012
@willdurand
FriendsOfSymfony member

Is everything automatic once you enable the listener? Is it based on routing requirements?

@lsmith77
FriendsOfSymfony member

yes .. it should be automatic
and yes its based on the requirements:
https://github.com/FriendsOfSymfony/FOSRestBundle/pull/250/files#L6R71

@borisguery

Hi,

Sorry I was travelling for a while, and very busy, thanks for finishing the work.

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