Skip to content

Fix improper detection and handling of html_safe buffer in CacheHelper #2080

Merged
merged 3 commits into from Jul 24, 2011

3 participants

@lhahne
lhahne commented Jul 15, 2011

This fixes the latest incarnation of #1537 where CacheHelper might try to use splice! for a html_safe buffer which converts the buffer unsafe. This change has been tested against Rails's tests and slim-lang which originally raised this issue.

@spastorino
Ruby on Rails member

We need a test case in order to merge this.

@lhahne
lhahne commented Jul 15, 2011

I think test_fragment_caching in caching_tests.rb already tests the bahavior of this function. In addition, even larger changes to this function, such as 114b5e4, have been accepted without specific tests. Anyways, I'll see if I can write a test case over the weekend.

@spastorino
Ruby on Rails member

@lhahne the commit you're pointing fixes some previously committed failing tests.

@lhahne
lhahne commented Jul 15, 2011

Ok. I know a way to break the old code so I can write some tests once I discover how to change the buffer in tests.

lhahne added some commits Jul 17, 2011
@lhahne lhahne Added tests for the output_buffer returned by CacheHelper
The output_buffer returned by CacheHelper should be html_safe if the original buffer is html_safe.
bc5ccd0
@lhahne lhahne made sure that the possible new output_buffer created by CacheHelper …
…is of the same type as the original
39a4f67
@lhahne
lhahne commented Jul 17, 2011

Ok. I added two tests one of which fails against the current 3-0-stable. After my fix, both tests pass. In addition, I modified CacheBuffer so that any possible new output_buffer is of the original type and made the tests reflect this.

@kommen
kommen commented Jul 19, 2011

This seems to be related to #2150

@kommen

@lhahne: As the output_buffer variable in this line is still pointing to the OutputBuffer created at the beginning of the test, you're explicitly testing here that the old output_buffer is still html_safe. Is this intended? It makes little sense to me, as it is replaced anyway.

Acutally, that depends. If you run this test against the current 3-0-stable with an html_safe output buffer which isn't ActionView::OutputBuffer (that's actually the second test), the execution will reach the else part of the if in fragment_for which causes the buffer to stay the same but converts it to unsafe when splice! is called.

You're right. This is true for 3-0-stable. However, since 03d01ec, which is in 3-1-stable, we don't test for ActionView::OutputBuffer explicitly any more, just for html_safe?. So I guess these asserts can stay for 3-0-stable, we don't need them in 3-1-stable (and master)?

If you are going to remove this, then I think you could also remove the assert_nothing_raised because there isn't much raising happening anymore.

@spastorino
Ruby on Rails member

Closed in PR 2219

@spastorino spastorino closed this Jul 23, 2011
@spastorino spastorino reopened this Jul 23, 2011
@spastorino
Ruby on Rails member

We need to backport PR 2219 to 3-0-stable

@kommen
kommen commented Jul 23, 2011

@spastorino in #2219 the regression fixed there is introduced by 03d01ec, which is not in 3-0-stable. So merging this pull request will suffice (assuming we're not backporting 03d01ec to 3-0-stable as well).

Though I think we should update the cache helper test to match the test from master.

@spastorino spastorino merged commit eead13f into rails:3-0-stable Jul 24, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.