Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

fix for non-themed pages for anonymous + gzip #11

Merged
merged 8 commits into from Nov 18, 2012

Conversation

Projects
None yet
4 participants
Owner

ebrehault commented Nov 13, 2012

see https://dev.plone.org/ticket/12038 for bug description

ebrehault added some commits Nov 10, 2012

@ebrehault ebrehault ThemeTransform.transformIterable must return a serialized content so …
…if GZipTransform is part of the chain, it will change it in an iterable safely.
f72e556
@ebrehault ebrehault those new gzip tests are actually NOT failling without the related fi…
…x in ThemeTransform.transformIterable, but there is not obvious way to reproduce the https://dev.plone.org/ticket/12038 bug with browser test (nor selenium by the way).
b27c980
@ebrehault ebrehault fix https://dev.plone.org/ticket/12038 d8246c3

As per the documentation at http://pypi.python.org/pypi/plone.transformchain, plone.app.transform is returning an iterable, a repoze.xmliter object. This defers serialization of the result tree until the result is needed, giving later transforms the opportunity of modifying the tree.

On my checkout, your tests pass whether this line is changed or not.

Owner

ebrehault commented on f72e556 Nov 13, 2012

As mentionned here http://pypi.python.org/pypi/plone.transformchain , the 3 ITransform methods can return an iterable, a unicode or a string.
So I think that, considering any later tranform will accept iterable, unicode or string, it might not make any difference if ThemeTransform returns a string instead of an iterable. But maybe I am missing something here (performances consideration maybe ?)

Nevertheless, returning a string does fix the bug.

Another way to fix it is to change https://github.com/plone/plone.app.caching/blob/master/plone/app/caching/gzip.py#L55 : if we do not return iter(result), but just result, the bug is also fixed.
But I thought it might have more side-effects if we change plone.app.caching transform rather than plone.app.theming transform.
What do you think ?

Note: I know my tests pass without my fix. I can reproduce the bug with wget, or manually with a web browser but I haven't been able to write a test which fails: I tried with test browser and selenium.
Nevertheless, I thought it was good anyway to have some extra tests about p.a.theming / p.a.caching integration.

Member

lrowe commented Nov 13, 2012

@ebrehault Could you add the steps to reproduce the bug to trac? The original report sounded exactly like an expected 304 Not Modified response (the etag stays the same if theming is on or off.)

Also, have you tried writing any tests for this? I can only see a test fixture in your changeset which is never used in any tests. I suspect the gzip transform is just not kicking in your test browser or selenium experiments because of a missing Accept-Encoding: gzip request header.

@optilude The necessity of the gzip transform returning something different seems to be down to this test: https://github.com/plone/plone.transformchain/blob/master/plone/transformchain/zpublisher.py#L70
Any idea whether the second part of that test, transformed is not result, is really necessary?

I can't tell by inspection why the change to plone.app.caching should change anything. @ebrehault Could you step through applyTransformOnSuccess (https://github.com/plone/plone.transformchain/blob/master/plone/transformchain/zpublisher.py#L76) and see what changes when you try the plone.app.caching gzip fix?

Owner

ebrehault commented Nov 13, 2012

oops right, I forgot to add my actual tests, here is test_caching.py (added to pull request)

Steps to reproduce the bug:

  • install Plone from https://github.com/plone/buildout.coredev branch 4.3 (but with last stable 4.2, it does the same)
  • create a new Plone site with "Diazo theme support" + "HTTP Caching support"
  • go to Site setup / Theming, and enable the Example theme
  • go to Site setup / caching / Import settings, choose "Without caching proxy" and click Import
  • go to Site setup / caching / Global settings, select "Enable caching" and "Enable GZip compression", and click Save
  • open the home page with manager access: it is themed
  • log out, it is still themed
  • but open the home page with another web session (for instance with private navigation in Chrome): it is not themed

I am pdbing to see what is actually going on, there is necessarily a difference in the transform chain between my post-logout anonymous request and my fresh-session anonymous request but I do not see it for now.

Owner

ebrehault commented Nov 13, 2012

Ok I found the problem, it is not in gzip, it is in plone.app.caching.operations.ramcache (which runs later in the chain):

https://github.com/plone/plone.app.caching/blob/master/plone/app/caching/operations/ramcache.py#L46

makes ''.join(result) and then return None, so the iterable is consummed, we get nothing instead.

It should actually return a new iterable containing the previous content.

If you agree, I propose to:

  • make a fix for plone.app.caching.operations.ramcache,
  • remove my change in plone.app.theming ThemeTransform.transformIterable
  • but let my tests in plone.app.theming (it makes more sense to test caching/theming integration in theming rather than in caching, no ?)
Member

lrowe commented Nov 13, 2012

@ebrehault Hmm. Returning None from the transform should keep the existing transform, see: https://github.com/plone/plone.transformchain/blob/master/plone/transformchain/transformer.py#L50

So the failure is caused by the interaction of the gzip transform returning iter(result) rather than result, then the ramcache transform consumes the iterator leaving nothing to return to the client.

Maybe @optilude can chime in here... I think the correct fix is to modify plone.transformchain to remove the transformed is not result part of the test then change the gzip transform to return the original result.

Adding tests to plone.app.theming to test caching integration is a good idea, but we need to find a way to for the tests to work somehow. Maybe setting the header "Accept-Encoding: gzip" on the request will do the job.

Owner

ebrehault commented Nov 13, 2012

For string or unicode, that's correct, returning None preserves the existing result.

But transformIterable is more delicate: it takes result as parameter, if it reads it (by calling str() or by iterating it in anyway), then result becomes empty, and if, in addition, transformIterable returns None, then result is not re-filled with a new value, so it remains empty.

So if the previous step in the transform chain returned a string or a unicode, no problem, but if it was an iterable, then the transform chain will start processing an empty result.

To me, ramcache transform behaviour is definitely not good.

Owner

ebrehault commented Nov 14, 2012

The test is now correct (it does fail), and I have made the fix here:
plone/plone.app.caching#3

Owner

ebrehault commented Nov 18, 2012

@lrowe plone.app.caching is fixed, so can you merge my tests here ?

Member

lrowe commented Nov 18, 2012

I'm travelling so please go ahead and merge your tests.
On Nov 18, 2012 6:42 PM, "Eric BREHAULT" notifications@github.com wrote:

@lrowe https://github.com/lrowe plone.app.caching is fixed, so can you
merge my tests here ?


Reply to this email directly or view it on GitHubhttps://github.com/plone/plone.app.theming/pull/11#issuecomment-10489547.

@ebrehault ebrehault added a commit that referenced this pull request Nov 18, 2012

@ebrehault ebrehault Merge pull request #11 from plone/ebrehault_fix_12038
fix for non-themed pages for anonymous + gzip
88e3b95

@ebrehault ebrehault merged commit 88e3b95 into master Nov 18, 2012

1 check failed

default The Travis build failed
Details
Contributor

garbas commented Dec 6, 2012

@ebrehault @lrowe
any idea why we still getting errors in tests here
tests are failing after you merged test_caching.py in ... which should be fixed with plone/plone.app.caching#3 but tests are still failing.

Owner

davisagli commented Dec 6, 2012

Don't worry about those tests; they were actually failing due to plone/plone.app.layout#13 and are now fixed.

@ebrehault ebrehault deleted the ebrehault_fix_12038 branch Nov 13, 2014

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