Skip to content
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

@Relation annotation attributes are not available in the URL generators #174

Open
bgaillard opened this issue Nov 24, 2014 · 11 comments
Open

Comments

@bgaillard
Copy link

Hi, I'm currently trying to generate multiple CURIE links having absolute hrefs.

Because I'm using Nelmio to document my APIs the CURIE absolute urls I have to generate must have the following structure.

Collection Resource : http://myserver/doc/#get--rest-{rel}.
Entity Resource : http://myserver/doc/#get--rest-{rel}-id.

To generate those CURIE links here are the Hateoas annotations I'm using on a sample entity class.

/**
 * ...
 * 
 * @Hateoas\Relation(
 *     "curies",
 *     attributes = {
 *         "name" = "collection-doc",
 *         "templated" = true
 *     },
 *     href = @Hateoas\Route(
 *         "nelmio_api_doc_index",
 *         absolute = true,
 *         parameters = { "name" = "collection-doc" }
 *     )
 * )
 * @Hateoas\Relation(
 *     "curies",
 *     attributes = {
 *         "name" = "entity-doc",
 *         "templated" = true
 *     },
 *     href = @Hateoas\Route(
 *         "nelmio_api_doc_index",
 *         absolute = true,
 *         parameters = { "name" = "entity-doc" }
 *     )
 * )
*/
class SampleEntity { ... }

Because I have to generate absolute URLs I'm using the @Hateoas\Route annotation with the name of the Nelmio route (i.e nelmio_api_doc_index).

Then, to generate my CURIE links I'm using a custom URL generator :

/**
     * {@inheritdoc}
     */
    public function generate($name, array $parameters, $absolute = false)
    {
        // If the route is associated to Nelmio then the generation is for a HAL CURIE Link
        if($name === 'nelmio_api_doc_index') {

            // CURIE Link for Collection Resource documentation
            if($parameters['name'] === 'collection-doc') {

                return $this->symfonyUrlGenerator->generate($name, array(), $absolute) . '#get--rest-{rel}';

            }

            // CURIE Link for Entity Resource documentation
            else if($parameters['name'] === 'entity-doc') {

                return $this->symfonyUrlGenerator->generate($name, array(), $absolute) . '#get--rest-{rel}-id';

            }

            throw new \RuntimeException('CURIE links expect a \'name\' parameter !');

        }

        // Otherwise the generation is for a HAL Link to a standard REST resource
        return $this->symfonyUrlGenerator->generate($name, $parameters, $absolute);

    }

Notice the use of the parameters attribute in the @Hateoas\Route annotations and then the retrieval of the name parameter in my custom URL generator.

Here, because the Hateoas generator does not have an access to the attributes attribute I'm forced to define my own conventions (i.e adding a custom parameter name in the @Hateoas\Route annotation and then retrieve this parameter in my URL generator).

It would be good to have an access to the attributes inside the Hateoas URL generators.

What do you think about that ?

@bgaillard bgaillard changed the title Relation attributes are not available in the URL generators @Relation annotation attributes are not available in the URL generators Nov 24, 2014
@adrienbrault
Copy link
Contributor

We could easily add the name and attributes as an expression variable for the route parameters (not elsewhere: embed, exclusion, name, attributes).

Though I'm not sure about the feature

@bgaillard
Copy link
Author

Hi @adrienbrault, I'm not sure I understand exactly what you mean ... "as an expression variable for the route parameters". Could you give a code sample ?

Though I'm not sure about the feature

Does anyone has the same need as us ?

Thanks

@giosh94mhz
Copy link
Contributor

I think @adrienbrault intends something like (mutatis mutandis):

/**
 * [...]
 * @Hateoas\Route(
 *     "nelmio_api_doc_index",
 *     absolute = true,
 *     parameters = "expr(object.getAttribute())"
 * )
 * [...]
 */

Does anyone has the same need as us ?

I'm also trying to integrate ApiDocBundle and HateoasBundle, even if it is not my priority right now.

@bgaillard
Copy link
Author

Hi and sorry to respond so late !

I understand the use of an expression variable but where this expression variable would be added, inside the Route annotation ? If this is the case I think it will duplicate informations specified in the Relation annotation.

In my use case everything I need is inside attributes of the Relation annotation but we could consider a developer could need to have an access to other informations in a Url generator.

In fact to be more generic what would be perfect and very flexible is to have an access to a the description of the current Relation (and the annotations it contains) in the Url Generator (or even better having an access to the whole context attached to the property which describes the relation).

For example we could have an access to a getCurrentRelation and setCurrentRelation in the Hateoas Url generator interface.

interface HateoasUrlGeneratorInterface extends \Hateoas\UrlGenerator\UrlGeneratorInterface {
    public function generate($name, array $parameters, $absolute = false);
    function getCurrentRelation();
    function setCurrentRelation(Relation $relation);
}

An other solution could be to add a fourth parameter to the generate method.

interface HateoasUrlGeneratorInterface extends \Hateoas\UrlGenerator\UrlGeneratorInterface {
    public function generate($name, array $parameters, $absolute = false, Relation $relation = null);
}

Then in the Url generator we have an access to the whole description of the relation for which one to generate a Url.

class UrlGenerator implements UrlGeneratorInterface {

    public function generate($name, array $parameters, $absolute = false, Relation $relation)
    {
        $relationAttributes = $relation->getAttributes();
        if($relationAttributes['name'] === 'collection-doc') {
            ...
        } else if($relationAttributes['name'] === 'entity-doc') {
            ...
        }
    }
}

What do you think about this proposition ?

@giosh94mhz
Copy link
Contributor

@bgaillard I now have a clear picture of your point of view (I think at least).

IMO, the current interface of UrlGeneratorInterface is simple enough and compatible with Symfony\Component\Routing\Generator\UrlGeneratorInterface; adding a custom parameters will diverge, and probably introduce some BC, so I would not change it.

On the other hand, adding a getter/setter for Relation will require a "RelationAwareInterface` (or something similar) and then generate will depends on context which is not good OOP IMO.

So why not solving this as already done for the Symfony Router? The generator parameters should be pre-filled with automatic value, which are not strictly parameters, but are still useful for route generation. Your scenario will translate to:

/**
 * [...]
 * No duplication here.
 * @Hateoas\Relation(
 *     "curies",
 *     attributes = {
 *         "name" = "entity-doc",
 *         "templated" = true
 *     },
 *     href = @Hateoas\Route(
 *         "nelmio_api_doc_index",
 *         absolute = true
 *     )
 * )
*/
class SampleEntity { ... }

And then into the generator you receive a more flexible parameters array:

class UrlGenerator implements UrlGeneratorInterface {

    // $parameters will contain _attributes key
    public function generate($name, array $parameters, $absolute = false)
    {
        $relationAttributes = $parameters['_attributes'];
        if($relationAttributes['name'] === 'collection-doc') {
            $url = ...
        } else if($relationAttributes['name'] === 'entity-doc') {
            $url = ...
        }
        return $url;
    }
}

Note the _attribute parameter; this will solve your problem, and also can be expanded with other "magic" parameters like _format.

What do you think? I can easily provide a PR for this.

@bgaillard
Copy link
Author

Hi @giosh94mhz and thanks for your quick response on this.

IMO, the current interface of UrlGeneratorInterface is simple enough and compatible with Symfony\Component\Routing\Generator\UrlGeneratorInterface; adding a custom parameters will diverge, and probably introduce some BC, so I would not change it.

Yes, I agree with on this.

Note the _attribute parameter; this will solve your problem, and also can be expanded with other "magic" parameters like _format.

I'm not really sure I understand what you propose.

If for example the @Relation annotation had the following parameters.

/**
 * @Hateoas\Relation(
 *     a = {},
 *     b = {},
 *     c = {},
 *     ...
 * }
 */

What you propose is having an access to __a, __b and __c in the parameters parameter of the Url generator generate method ?

This seems strange to me and I think this solution has several disavantages :

  • In the Url Generator the developer has only an access to a parameters parameter. Then he has to guess the name of the keys in this array. With a direct access to a Relation object we have code completion and this is less subject to coding errors
  • This solution will be more difficult to understand. In most cases I'm not happy with "magic things" hidden in the code and behavior of a library.

In my opinion a solution with a specific software interface would be better, more flexible and easier to maintain.

On the other hand, adding a getter/setter for Relation will require a "RelationAwareInterface` (or something similar) and then generate will depends on context which is not good OOP IMO.

Adding a RelationAwareInterface with getters/setters could respond to my needs. But the result of the getCurrentRelation() will be different depending on the generate call which is not really good (i.e the state of the Url Generator will change depending on the calls to the generate method).

Perhaps 2 different interfaces in Hateaos could do it :

  • Keep the Symfony 2 UrlGeneratorInterface interface to not break existing implementations
  • Add a new RelationAwareUrlGeneratorInterface
// UrlGeneratorInterface.php
interface UrlGeneratorInterface {
    public function generate($name, array $parameters, $absolute = false);
}

// RelationAwareUrlGeneratorInterface.php
interface RelationAwareUrlGeneratorInterface {
    public function generate($name, array $parameters, $absolute = false, Relation $relation);
}

Then a the moment of Url Generation (is it here https://github.com/willdurand/Hateoas/blob/master/src/Hateoas/Factory/LinkFactory.php ?) ...

if($urlGenerator instanceof RelationAwareUrlGeneratorInterface) {
    $urlGenerator->generate($name, $parameters, $isAbsolute, $relation);
} else if($urlGenerator instanceof UrlGeneratorInterface) {
    $urlGenerator->generate($name, $parameters, $isAbsolute);
}

What do you think about that ?

@giosh94mhz
Copy link
Contributor

@bgaillard

What you propose is having an access to __a, __b and __c in the parameters parameter of the Url generator generate method ?

No, is not what I mean. I agree with you that this is ugly. :) I mean that the generator will receive:

$parameters = [
    'your_routing_param' => 'param_value',
    '_attributes' => [
        'a' => 'value1',
        'b' => 'value2',
        'c' => 'value3',
    ]
]

I'll open a PR for this if I get the time, so it may be clear. This seems magical, but it's actually in line with HAL (_link, _embedded) and Symfony (_format, _method, _locale); obviously this must be documented.

If not choosing this way, I like the two interfaces idea, but I think that the RelationAwareUrlGeneratorInterface tent to overlap with the LinkFactory responsibility.

These are my thought, but the maintainers will have the final word. :)

@adrienbrault
Copy link
Contributor

@bgaillard I don't like the idea of making the url generator aware of the library's Configuration objects.

Here's what I suggest you do:

/**
 * ...
 * 
 * @Hateoas\Relation(
 *     "curies",
 *     attributes = {
 *         "name" = "collection-doc",
 *         "templated" = true
 *     },
 *     href = @Hateoas\Route(
 *         "nelmio_api_doc_index",
 *         absolute = true,
 *         parameters = { "anchor" = "get--rest-{rel}" }
 *     )
 * )
 * @Hateoas\Relation(
 *     "curies",
 *     attributes = {
 *         "name" = "entity-doc",
 *         "templated" = true
 *     },
 *     href = @Hateoas\Route(
 *         "nelmio_api_doc_index",
 *         absolute = true,
 *         parameters = { "anchor" = "get--rest-{rel}-id" }
 *     )
 * )
*/
class SampleEntity { ... }
public function generate($name, array $parameters, $absolute = false)
{
    $anchor = '';
    if (isset($parameters['anchor'])) {
        $anchor = $parameters['anchor'];
        unset($parameters['anchor']);
    }

    return parent::generate($name, $parameters, $absolute) . $anchor;

}

And if you find that writing that annotation everytime is long and annoying, then you can introduce you own annotation for this:

/**
 * ...
 * 
 * @YourVendor\Curie("collection-doc")
 * @YourVendor\Curie("entity-doc")
*/
class SampleEntity { ... }

And add a new Hateoas configuration extension:

namespace YourVendor;

use Hateoas\Configuration\Metadata\ConfigurationExtensionInterface;
use Hateoas\Configuration;

class HateoasCurieExtension implements ConfigurationExtensionInterface
{
    public function decorate(ClassMetadataInterface $classMetadata)
    {
        $relations = $this->getCurieRelations($classMetadata->getName());

        foreach ($relations as $relation) {
            $classMetadata->addRelation($relation);
        }
    }

    private function getCurieRelations($className)
    {
        // use annotation driver to find your own annotation, and convert that to hateoas relation
        $relations = [];
        foreach ($annotations as $annotation) {
            if (!$annotation instanceof MyVendor\Curie) {
                continue;
            }

            $relations[] = new Configuration\Relation(
                'curies',
                new Configuration\Route('nelmio_api_doc_index', ['anchor' => 'get--rest-{rel}'], true),
                null,
                ['name' => $annotation->name, 'templated' => true]
            );
        }

        return $relations;
    }
}

@giosh94mhz
Copy link
Contributor

@adrienbrault this is mind blowing! :) I haven't tough of the problem from this PoV.

Anyway, since attributes are strictly related to the link and not a simple configuration, I don't see many issues in embedding the attributes inside the parameters for the generator sake; there are some drawback that I don't see?

@adrienbrault
Copy link
Contributor

@giosh94mhz Yes, let's say you do that with this:

/**
 * ...
 * 
 * @Hateoas\Relation(
 *     "curies",
 *     attributes = {
 *         "name" = "entity-doc",
 *         "templated" = true
 *     },
 *     href = @Hateoas\Route(
 *         "nelmio_api_doc_index",
 *         absolute = true
 *     )
 * )
*/
class SampleEntity { ... }

With the symfony router, the generated url would be /api/doc?name=entity-doc&templated=true which is not what 99% of the people would expect.

@bgaillard
Copy link
Author

@giosh94mhz A, ok now I understand well the solution you proposed.

@adrienbrault Thanks for the code samples, I think defining a new annotation is a good idea for us.

I don't like the idea of making the url generator aware of the library's Configuration objects.

Ok, in my opinion the @Route name and absolute attribute are already "library's configuration" stuffs / "objects", it implies the Url generator is already aware of a sub-set of the library's Configuration objects. So I though it was logical to also have an access to the configuration of the Relation annotation.

Anyway playing with the parameters parameter of the @Route annotation allow us to do what we want for now. So if you consider solving this issue by adding new code inside the library will not add enough value don't hesitate to close it.

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

No branches or pull requests

4 participants