EZP-25613 Add a way to register resources with the rest root #1619

wizhippo · 2016-03-23T16:06:50Z

JIRA: https://jira.ez.no/browse/EZP-25613

This adds the ability to add resources in the rest root so rest consumers can discover them. For example you can register the resource and use the capi auto discovery to find the uri for the resource.

To add a resource a bundle would add the resource using the PrependExtensionInterface and pre-pending the new resource to the config.

wizhippo · 2016-03-23T17:42:50Z

tests fail on RootTest. This is because the Root now expects an array of Resources to be provided. Should I pass an arbitrary array of resources and test them or should I replicate the hard coded resources in the original Root value object visitor by creating array of resources based on the resources in default_settings.yml?

bdunogier · 2016-03-23T20:24:12Z

Can you please create an issue on JIRA if there isn't one, and link the PR to the issue ? I'll assign it to you and move it to dev.

bdunogier · 2016-03-23T20:26:59Z

eZ/Publish/Core/REST/Common/Values/Resource.php

+ */
+class Resource extends RestValue
+{
+    public $name;


phpdoc with description + example for public properties of value objects.

bdunogier · 2016-03-23T20:33:26Z

Looks good overall.

One thing I'd like to see improved is the separation between the Controller and the Root Value construction. Ideally, the Root value would be obtained via a service, and that service would have an implementation that uses an array of Resource objects, and uses the expression engine to generate the hrefs.

wizhippo · 2016-03-24T15:07:37Z

Test are failing because the default_settings.yml are not loaded so the resources are not defined. They pass locally. Ideas on how I can fix this?

emodric · 2016-03-24T21:46:38Z

eZ/Bundle/EzPublishRestBundle/EzPublishRestBundle.php

+        /** @var \eZ\Bundle\EzPublishCoreBundle\DependencyInjection\EzPublishCoreExtension $eZExtension */
+        $eZExtension = $container->getExtension('ezpublish');
+        $eZExtension->addConfigParser(new ConfigParser\RestResources());
+        $eZExtension->addDefaultSettings(__DIR__ . '/Resources/config', ['default_settings.yml']);


Why was loading the default_settings.yml file moved here? I don't think there's a reason for that.

I suspect this is the reason why the tests are not loading the config.

bdunogier · 2016-03-25T10:37:05Z

Okay, this should cover it. It's mostly about naming, but as Camus said: "Misnaming things adds to the evil of the world."

Also, if you want to give it a try, it would be good to improve the REST functional tests, and check that we have the links we expect. This change removes some coverage from the visitor's unit tests, and I think we want to check that the root resource behaves as we expect it to.

bdunogier · 2016-03-25T10:41:41Z

eZ/Bundle/EzPublishRestBundle/Resources/config/default_settings.yml

@@ -4,3 +4,83 @@ parameters:

    # URI part that all REST routes begin with. By this a REST request is recognized.
    ezpublish_rest.path_prefix: /api/ezp/v2
+
+    ezpublish_rest.root_resources:


Why don't you write directly to the siteaccess aware container config ? You could then skip loading the container config altogether: https://github.com/ezsystems/ezpublish-kernel/pull/1619/files#diff-0b45d72cefd7a4d4849d6d15e1527b31R40.

ezsettings.default.rest_root_resources: content: ...

I like this, this simpler. In service.yml I can just pass '$rest_root_resources$'. We would lose the checks from the semantic config.

wizhippo · 2016-03-25T18:12:02Z

To add a test for the default configured resources. What would be a good test name? Would I just load in the resources from the default_settings.yml build a Root and test against the original tests removed in RootTest?

…covery

bdunogier · 2016-03-27T20:43:46Z

To add a test for the default configured resources.

I meant a functional test, that uses the actual application, and validates the responses.
That way we are sure that the rest resource doesn't change without reasons (it should check that the expected elements are there, but it shouldn't check that there are no extra elements).

wizhippo · 2016-03-27T23:11:25Z

So update https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Bundle/EzPublishRestBundle/Tests/Functional/RootTest.php?

Edit: added

bdunogier · 2016-03-28T09:48:24Z

So update https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Bundle/EzPublishRestBundle/Tests/Functional/RootTest.php?

Yep

wizhippo · 2016-03-28T13:04:17Z

@bdunogier Not sure if you saw but I had added the tests.

bdunogier · 2016-03-28T14:23:45Z

I had a quick look on mobile, it seems fine. I'll check again from a proper device (easter holiday here).

bdunogier · 2016-04-06T10:00:47Z

Looks good to me now.

Opinion @dpobel / @emodric ?

emodric · 2016-04-06T10:21:55Z

So how do we actually register custom root resources, apart from adding directly to internal ezsettings.default.rest_root_resources, which is discouraged? This probably needs semantic config, much like config for registering custom field view templates.

Other than that: 👍

bdunogier · 2016-04-06T10:31:08Z

eZ/Publish/Core/REST/Common/Values/Resource.php

+/**
+ * This class represents a resource.
+ */
+class Resource extends RestValue


Too bad RestValue doesn't implement the base ValueObject. We could have used the same properties scheme :/

dpobel · 2016-04-08T06:53:58Z

nice! besides the missing JIRA issue +1

bdunogier · 2016-04-08T07:23:54Z

eZ/Publish/Core/REST/Server/Service/ExpressionRouterRootResourceBuilder.php

+            $resources[] = new Values\Resource(
+                $name,
+                $resource['mediaType'],
+                $language->evaluate($resource['href'], [


Should we have a try/catch here ? what will evaluate() do if the expression is invalid ?

@dpobel Missing JIRA issue? https://jira.ez.no/browse/EZP-25613

@bdunogier It throws an SyntaxError exception. Do you want the message more specific like new SyntaxError("Invalid expression '${resource['href']}' in resource '$name'"); or throw a different exception? If not to throw an exception then what?

An exception is good. It should end up as a 500 in REST, and that's good, I think.

Should be good as is. evaluate will throw an SyntaxError already.

bdunogier · 2016-04-29T13:30:18Z

Great, let's merge this then. We can take care of semantic config in another pull-request.

wizhippo · 2016-04-29T14:48:23Z

Not sure it counts but you can do:

ez_publish_rest:
    system:
        default:
            rest_root_resources:
                someresource:
                    mediaType: ''
                    href: 'router.generate("ezpublish_rest_createContent")'

wizhippo mentioned this pull request Mar 23, 2016

[WIP] Add a way to register resources with the rest root #1566

Closed

bdunogier reviewed Mar 23, 2016
View reviewed changes

wizhippo changed the title ~~[WIP] Add a way to register resources with the rest root~~ [WIP] EZP-25613 Add a way to register resources with the rest root Mar 23, 2016

emodric reviewed Mar 24, 2016
View reviewed changes

bdunogier reviewed Mar 25, 2016
View reviewed changes

[WIP] Add a way to register resources with the rest root for auto dis…

4ed450e

…covery

wizhippo closed this Mar 27, 2016

wizhippo reopened this Mar 27, 2016

Add default configured resources root element functional tests.

9faf3c5

bdunogier reviewed Apr 6, 2016
View reviewed changes

wizhippo added 2 commits April 6, 2016 12:15

Add semantic config

0b9d39a

CS

1fa8f21

bdunogier reviewed Apr 8, 2016
View reviewed changes

bdunogier merged commit c6b896d into ezsystems:master Apr 29, 2016

andrerom changed the title ~~[WIP] EZP-25613 Add a way to register resources with the rest root~~ EZP-25613 Add a way to register resources with the rest root Apr 29, 2016

wizhippo deleted the dynamic-rest-uri branch April 29, 2016 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-25613 Add a way to register resources with the rest root #1619

EZP-25613 Add a way to register resources with the rest root #1619

wizhippo commented Mar 23, 2016 •

edited by bdunogier

wizhippo commented Mar 23, 2016

bdunogier commented Mar 23, 2016

bdunogier Mar 23, 2016

bdunogier commented Mar 23, 2016

wizhippo commented Mar 24, 2016

emodric Mar 24, 2016

bdunogier commented Mar 25, 2016

bdunogier Mar 25, 2016

wizhippo Mar 25, 2016

wizhippo commented Mar 25, 2016

bdunogier commented Mar 27, 2016

wizhippo commented Mar 27, 2016

bdunogier commented Mar 28, 2016

wizhippo commented Mar 28, 2016

bdunogier commented Mar 28, 2016 via email

bdunogier commented Apr 6, 2016

emodric commented Apr 6, 2016

bdunogier Apr 6, 2016

dpobel commented Apr 8, 2016

bdunogier Apr 8, 2016

wizhippo Apr 8, 2016

bdunogier Apr 26, 2016

wizhippo Apr 26, 2016

bdunogier commented Apr 29, 2016

wizhippo commented Apr 29, 2016

EZP-25613 Add a way to register resources with the rest root #1619

EZP-25613 Add a way to register resources with the rest root #1619

Conversation

wizhippo commented Mar 23, 2016 • edited by bdunogier

wizhippo commented Mar 23, 2016

bdunogier commented Mar 23, 2016

Choose a reason for hiding this comment

bdunogier commented Mar 23, 2016

wizhippo commented Mar 24, 2016

Choose a reason for hiding this comment

bdunogier commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wizhippo commented Mar 25, 2016

bdunogier commented Mar 27, 2016

wizhippo commented Mar 27, 2016

bdunogier commented Mar 28, 2016

wizhippo commented Mar 28, 2016

bdunogier commented Mar 28, 2016 via email

bdunogier commented Apr 6, 2016

emodric commented Apr 6, 2016

Choose a reason for hiding this comment

dpobel commented Apr 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunogier commented Apr 29, 2016

wizhippo commented Apr 29, 2016

wizhippo commented Mar 23, 2016 •

edited by bdunogier