[WIP] Convert X-Location-Id into tags for non-view responses #21

Plopix · 2017-11-04T21:22:57Z

issue: https://jira.ez.no/browse/EZP-28183

Attempts to fix #20

tag the Response with X-Location-Id that are not view
Test the purge (not working for me yet) see PR Purge type HTTP just does not work #22

andrerom · 2017-11-05T12:58:48Z

src/EventSubscriber/HttpCacheResponseSubscriber.php

@@ -40,7 +40,7 @@ public static function getSubscribedEvents()
    public function configureCache(FilterResponseEvent $event)
    {
        $view = $event->getRequest()->attributes->get('view');
-        if (!$view instanceof CachableView || !$view->isCacheEnabled()) {
+        if ($view !== null && ( !$view instanceof CachableView || !$view->isCacheEnabled() )) {


why this change? We don't want to set ttl on non ContentView response automatically. People can set their own s-maxage depending on what logic they have and what they need.

At lest let's keep this out of this bug fix.

Ok I have used @vidarl logic here, it breaks the unit tests though, can someone fix them?

andrerom · 2017-11-05T13:27:29Z

src/ResponseTagger/Delegator/XLocationIdTagger.php

+    ) {
+        $this->repository        = $repository;
+        $this->contentInfoTagger = $contentInfoTagger;
+        $this->locationTagger    = $locationTagger;


I get what you are trying to do here, but let's just map the location id to xkey equivalent here, and unset the x-Location-Id parameter to not let that stay around.

PR's like #7 and similar are meant to add the other tags based on the data already loaded, to avoid us having to load repository objects several times.

In other words, you won't need any dependencies here.

--

In addtion, this PR should remove the following lines as it will be covered here instead: https://github.com/ezsystems/ezplatform-http-cache/blob/master/src/Proxy/TagAwareStore.php#L76-L78

Plopix · 2017-11-05T18:54:41Z

ping @andrerom @bdunogier @vidarl

I have reworked it @andrerom and I have used your name @vidarl
@andrerom I have kept the dependency on the LocationTaggger to get the parent location for free.
I did not want to remove X-Location-Id because other taggers (in the actual pattern) could use it but I suppose it is fine and only this one will play with X-Location-id...

bdunogier

You could add a Spec for the tagger. You'll see, phpsec is great :)

bdunogier · 2017-11-06T09:47:47Z

src/ResponseTagger/Delegator/XLocationIdBCTagger.php

+use EzSystems\PlatformHttpCacheBundle\ResponseTagger\ResponseTagger;
+use Symfony\Component\HttpFoundation\Response;
+
+class XLocationIdBCTagger implements ResponseTagger


Could you add a summary of what the Tagger does, and in what context it is (not) useful ?

bdunogier · 2017-11-06T09:48:42Z

src/ResponseTagger/Delegator/XLocationIdBCTagger.php

+    const LOCATION_ID_HEADER = 'X-Location-Id';
+
+    /**
+     * XLocationIdTagger constructor.


Nitpick: I'd remove that block altogether. Including the @param.

bdunogier · 2017-11-06T09:51:02Z

src/ResponseTagger/Delegator/XLocationIdBCTagger.php

+        foreach ($locationIds as $locationId) {
+            $location = $this->repository->getLocationService()->loadLocation($locationId);
+            $this->locationTagger->tag($configurator, $response, $location);
+            $response->headers->remove('X-Location-Id');


You should replace 'X-Location-ID' with self::LOCATION_ID_HEADER.

good catch! Fixed.

andrerom · 2017-11-06T10:01:04Z

src/EventSubscriber/HttpCacheResponseSubscriber.php

-        $view = $event->getRequest()->attributes->get('view');
-        if (!$view instanceof CachableView || !$view->isCacheEnabled()) {
-            return;
+        if ($event->getRequest()->attributes->has('is_rest_request') && $event->getRequest()->attributes->get('is_rest_request') === true) {


I still don't see why this needs to be added, afaik REST already sets s-maxage doesn't it? Is this needed to solve this bug or can this be handled separately?

Yes, that is required because for REST requests the kernel does not return a view.
Then, as it is not a view, it returns null and nothing is tagged accordingly.
Please note that when we return null here, the response is cached for the default time.

bdunogier · 2017-11-06T15:49:28Z

src/ResponseTagger/Delegator/XLocationIdBCTagger.php

+
+        $locationIds = explode("|", $response->headers->get(static::LOCATION_ID_HEADER));
+        foreach ($locationIds as $locationId) {
+            $location = $this->repository->getLocationService()->loadLocation($locationId);


After internal discussions, we aren't very comfortable with loading locations in this part. What happens if loading fails, for instance because of permissions ? We only need to do this because the Value Objects used to generate this header aren't available anymore.

Did you consider going for a simpler tagging, and only generate location-<locationId> ? It would avoid having to load the location.

On the long term, this will have to be fixed in the proper layers anyway. For instance, we have two pull-requests about REST & HTTP caching, both in regards to REST includes: ezsystems/ezpublish-kernel#1741 & ezsystems/ezpublish-kernel#1789 (WIP). They've been here for a while, but do touch the same areas.

As an alternative, an option would be to override the CachedValue value object visitor from the REST server. This is where we generate the X-Location-ID header. As for available data, we have both the location ID and the the cached value object, meaning that we can use Response Taggers.

Plopix · 2017-11-06T15:50:25Z

@andrerom @bdunogier @vidarl I have changed stuff.
Also, the code was breaking as repository was not injected anymore and we need it to load the location... not sure you want that. let me know.

Plopix · 2017-11-06T16:33:42Z

And we would not mess with the '$view' in the subscriber. It sounds better to tag there directly indeed. But how about custom devs that are using X-Location-Id? that needs to be working right? ok for location i will remove the load etc. Questions remain to know where to do it correctly.

Plopix · 2017-11-08T00:05:26Z

I suppose this PR is not useful anymore right? if so, you can close.

andrerom · 2017-11-08T08:46:29Z

Thanks for the pointers here @Plopix, we'll handle this in #25

@Plopix

With this bundle, REST HttpCache is not cleared when moving content, editing content types, (...) And given the time it will take to fix REST server to return data that can be used for ResponseTaggers and how this also affects custum responses given, as we now only purge by specific tags. Adding this is thus safests. This somewhat brings back what @Plopix suggested in #21, essentially making sure we add all tags also to X-Location responses by loading location, but to be on the safe side it does it using sudo() and catching NotFound. Opted not to reuse ReponseTaggers here as they don't fully fit, and as we want to rewrite this header independently of if view cache is enabled or not.

@Plopix

With this bundle, REST HttpCache is not cleared when moving content, editing content types, (...) And given the time it will take to fix REST server to return data that can be used for ResponseTaggers and how this also affects custum responses given, as we now only purge by specific tags. Adding this is thus safests. This somewhat brings back what @Plopix suggested in #21, essentially making sure we add all tags also to X-Location responses by loading location, but to be on the safe side it does it using sudo() and catching NotFound. Opted not to reuse ReponseTaggers here as they don't fully fit, and as we want to rewrite this header independently of if view cache is enabled or not.

@Plopix

With this bundle, REST HttpCache is not cleared when moving content, editing content types, (...) And given the time it will take to fix REST server to return data that can be used for ResponseTaggers and how this also affects custum responses given, as we now only purge by specific tags. Adding this is thus safests. This somewhat brings back what @Plopix suggested in #21, essentially making sure we add all tags also to X-Location responses by loading location, but to be on the safe side it does it using sudo() and catching NotFound. Opted not to reuse ReponseTaggers here as they don't fully fit, and as we want to rewrite this header independently of if view cache is enabled or not.

andrerom reviewed Nov 5, 2017

View reviewed changes

andrerom mentioned this pull request Nov 5, 2017

EZEE-1780: Plugin support #19

Merged

Plopix mentioned this pull request Nov 5, 2017

Purge type HTTP just does not work #22

Merged

bdunogier reviewed Nov 6, 2017

View reviewed changes

andrerom reviewed Nov 6, 2017

View reviewed changes

Convert X-Location-Id into tag for non-view response

def9db2

bdunogier reviewed Nov 6, 2017

View reviewed changes

andrerom mentioned this pull request Nov 7, 2017

EZP-28183: Requests (incl from REST) with X-Location-Id not tagged #25

Merged

1 task

andrerom closed this Nov 8, 2017

andrerom mentioned this pull request Nov 27, 2017

EZP-28326: Http Cache not updated when moving content on Varnish & Symfony Proxy #37

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Convert X-Location-Id into tags for non-view responses #21

[WIP] Convert X-Location-Id into tags for non-view responses #21

Plopix commented Nov 4, 2017 •

edited by andrerom

Loading

andrerom Nov 5, 2017 •

edited

Loading

Plopix Nov 5, 2017 •

edited

Loading

andrerom Nov 5, 2017 •

edited

Loading

Plopix commented Nov 5, 2017

bdunogier left a comment

bdunogier Nov 6, 2017

Plopix Nov 6, 2017

bdunogier Nov 6, 2017

Plopix Nov 6, 2017

bdunogier Nov 6, 2017

Plopix Nov 6, 2017

andrerom Nov 6, 2017 •

edited

Loading

Plopix Nov 6, 2017

bdunogier Nov 6, 2017 •

edited

Loading

Plopix commented Nov 6, 2017

Plopix commented Nov 6, 2017 via email •

edited

Loading

Plopix commented Nov 8, 2017

andrerom commented Nov 8, 2017

[WIP] Convert X-Location-Id into tags for non-view responses #21

[WIP] Convert X-Location-Id into tags for non-view responses #21

Conversation

Plopix commented Nov 4, 2017 • edited by andrerom Loading

andrerom Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

Plopix Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

andrerom Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

Plopix commented Nov 5, 2017

bdunogier left a comment

Choose a reason for hiding this comment

bdunogier Nov 6, 2017

Choose a reason for hiding this comment

Plopix Nov 6, 2017

Choose a reason for hiding this comment

bdunogier Nov 6, 2017

Choose a reason for hiding this comment

Plopix Nov 6, 2017

Choose a reason for hiding this comment

bdunogier Nov 6, 2017

Choose a reason for hiding this comment

Plopix Nov 6, 2017

Choose a reason for hiding this comment

andrerom Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Plopix Nov 6, 2017

Choose a reason for hiding this comment

bdunogier Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Plopix commented Nov 6, 2017

Plopix commented Nov 6, 2017 via email • edited Loading

Plopix commented Nov 8, 2017

andrerom commented Nov 8, 2017

Plopix commented Nov 4, 2017 •

edited by andrerom

Loading

andrerom Nov 5, 2017 •

edited

Loading

Plopix Nov 5, 2017 •

edited

Loading

andrerom Nov 5, 2017 •

edited

Loading

andrerom Nov 6, 2017 •

edited

Loading

bdunogier Nov 6, 2017 •

edited

Loading

Plopix commented Nov 6, 2017 via email •

edited

Loading