On-demand REST resource embedding

[bdunogier]

Implements EZP-26033
Specification
Mostly ready for review and testing, one minor (deprecation) todo list item remaining

See the specification for a full technical description of the feature and its implementation.

TODO

• Rename the value loader, with an interface (HrefRestResourceLoader ?)
• Cover multi media-type (example: Location.Content loads a ContentInfo. The media-type attribute must be used for the value load request.
• Apply to other visitors once approved
• Update specification
• Design and implement Request format (x-ez-embed-value header)
• Add unit test for the loader
• Add support for lists in request format
• Prevent listElement from showing up twice in the stackPath
• Update existing tests that break
• Handle automatic parent expansion in request format (ex: Content.Owner.MainLocation implies automatic expansion of Owner first).
• Add unit test for the ResourceLink and ResourceRouteReference visitors
• Add a custom attribute to the reference if loading failed (permissions or multiple load).
• Add a recursion protected value loader, that refuses to load the same resource more than once. When it happens, throws an exception.
• Look for remaining $this->generator->generate() calls in visitors that need replacing
• Rename the parameters property in ResourceRouteReference
• Add unit test for UniqueUriValueLoader
• Deprecate the old way to visit stuff, if it turns out to be necessary
• Rename to REST include to align with JSON API concept wise where query is used instead of header simplifying use/examples

[glye]

Looks good, insofar as less code to write, test and maintain.

[yannickroger]

I like it ! 😄

[pspanja]

Would it be possible to have a Loader implementation using HttpKernel, making proper sub-requests with ESI?

[bdunogier]

@pspanja I need to think about it again (it was like a month ago now), but in my vision of this architecture, yes, this was achievable.

I'll be happy to discuss the details with you.

[andrerom]

I'm assuming @pspanja asks to be able to keep documents separated in Proxy cache to get best possible cache efficiency.

One alternative we could explore is HTTP/2 server push, but I have no idea how Varnish behaves with that. At least with ESI it will be able to. It's just more tricky to get the resulting json(/xml..) document to be valid.

[bdunogier]

Well, there are two main ways to see this:

1. generate ESI tags in the response, and let the reverse proxy handle it
2. use HttpKernel (I guess that's what @pspanja meant) during rendering to load the payload of embedded resources.

Problem with 2 is that it doesn't really improve caching: the root resource that is requested will still be cached as a whole, still requiring multi-tagging.

1 is a solution I did think about when specifying this feature, but it has a few implications. The two main ones are making sure that the embedded payload is valid in the context of the parent (XML header for instance), and being able to specify the output format (XML/JSON) by means of an URL alone. We currently do it via the Accept header. Url would require that we add support for .json / .xml.

[bdunogier]

Refactored checking of expansion to a dedicated service, with use-cases in a unit-test: https://github.com/ezsystems/ezpublish-kernel/pull/1741/files#diff-675a7b1950edeb252504e0978c6065e4R50. It specifies how lists content expansion is requested, as well as automatic parent loading.

[dpobel]

could we have something a bit more explicit instead ? Also, it's not written anywhere but I guess the same will happen if for instance the embedded object has been removed (for instance the Owner of a Content Item) ? Ideally, the REST consumer should be able to differentiate those 2 cases. For instance, an attribute on the link would work well I think

[bdunogier]

An attribute on the link is easy. Any preferred format/contents ?

[bdunogier]

Added a TODO in any case.

[yannickroger]

Nitpick: You can remove 'Cookie: XDEBUG_SESSION=PHPSTORM'

[bdunogier]

Good point :-)

[bdunogier]

@pcardiga we should talk about it further, but I think that the feature can be tested by QA.

It won't work for the time being with HTTP cache (as in cache will be stale until ttl expires on embed data), but we have planned on covering that with @andrerom's work. But meanwhile, the feature itself can be tested in order to check that it matches the specifications, I think.

[yannickroger]

Just a note. We should not merge this until we started prototyping the JS Client (and also maybe platform UI) to benefit from this patch. We need to have a real feedback on its usage before having to keep BC on it for a long time.

So before this has been done, I am -1 on merging this.

[bdunogier]

It can not be merged (or at least used) before HTTP cache multi-tagging is implemented as well anyway, Yannick.

[dpobel]

The PR ezsystems/PlatformUIBundle#687 uses this branch. As written there, I think there are 2 issues:

1. it seems like the ContentInfo has been removed from the Location response. This is breaking some parts of PlatformUI.
2. there's a recursion protection which prevents the system from embedding the same object several times. For instance 2 versions can have the same creator (or you can have 2 drafts for the same Content) and this is something very complicated to handle in the PlatformUI.

[andrerom]

Would it be possible to instead use query param and align with JSON API? Or are there some implementation details that makes it hard to use query param?

As in:

curl -X GET http://localhost:8000/api/ezp/v2/content/objects/1?include=Content.MainLocation

[andrerom]

(using header will make it hard to cache for instance)

[andrerom]

Regarding the http/2 point earlier, it's pretty much here by now: https://blog.apisyouwonthate.com/lets-stop-building-apis-around-a-network-hack-9a68f7e83dd2 (a very oponionated blog, but worth the read to see how it could work)