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

system-test: retain custom headers #226

Merged
merged 1 commit into from Apr 15, 2013

Conversation

Projects
None yet
2 participants
Contributor

jeffkaufman commented Apr 4, 2013

No description provided.

@oschaaf oschaaf commented on the diff Apr 5, 2013

test/nginx_system_test.sh
@@ -415,4 +415,13 @@ fetch_until -save -recursive $URL 'grep -c .pagespeed.ic' 2 # 2 images optimized
# is tuned to avoid that.
check_file_size "$OUTDIR/*256x192*Puzzle*" -le 8157 # resized
+start_test Custom headers remain on resources, but cache should be 1 year.
+URL="$TEST_ROOT/compressed/hello_js.custom_ext.pagespeed.ce.HdziXmtLIV.txt"
+echo $WGET_DUMP $URL
+RESOURCE_HEADERS=$($WGET_DUMP $URL)
+check_from "$RESOURCE_HEADERS" egrep -q 'X-Extra-Header: 1'
+# The extra header should only be added once, not twice.
+check_not_from "$RESOURCE_HEADERS" egrep -q 'X-Extra-Header: 1, 1'
@oschaaf

oschaaf Apr 5, 2013

Owner

Not sure, but do we need to test for the case where we get

X-Extra-Header: 1
X-Extra-Header: 1

Instead of

 X-Extra-Header: 1, 1

here?

@jeffkaufman

jeffkaufman Apr 5, 2013

Contributor

Good catch! Not only should we be checking this, but we're getting it wrong.

@jeffkaufman

jeffkaufman Apr 5, 2013

Contributor

Check added in: bf14e06

Owner

oschaaf commented Apr 5, 2013

LGTM

Contributor

jeffkaufman commented Apr 5, 2013

To reproduce duplicate header issue:

$ curl -D- http://localhost:8050/mod_pagespeed_test/compressed/hello_js.custom_ext.pagespeed.ce.HdziXmtLIV.txt

Observe:

X-Extra-Header: 1
...
X-Extra-Header: 1
Contributor

jeffkaufman commented Apr 5, 2013

The relevant config is:

server {
  add_header X-Extra-Header 1;

  location /mod_pagespeed_test/compressed/ {
     # Even though this is also in the server block it must be repeated here    
     # because add_header directives are only inherited if you don't define     
     # more of them.  See: http://serverfault.com/questions/400197              
     add_header X-Extra-Header 1;
  }
}

We're serving /mod_pagespeed_test/compressed/hello_js.custom_ext with one X-Extra-Header: 1 and /mod_pagespeed_example/images/256x192xPuzzle.jpg.pagespeed.ic.AOSDvKNItv.jpg with two extra headers.

I tried adding back into the config:

location ~ "\.pagespeed\.([a-z]\.)?[a-z]{2}\.[^.]{10}\.[^.]+" { }

on the theory that the problem was that location /mod_pagespeed_test/compressed/ was matching the .pagespeed. resource and adding another header, but that had no effect, presumably because of the add_header in the server block.

I think the problem is simply that the add_header in the server block gets applied to the finished .pagespeed. resource after we run.

We can fix this with:

location ~ "\.pagespeed\.([a-z]\.)?[a-z]{2}\.[^.]{10}\.[^.]+" {
  add_header "" "";
}

but it makes me sad. The reason this works is that add_header with empty arguments doesn't add any headers but does disable any headers added at other stages of the configuration. This is actually more useful than just here: it solves the problem of people who have broad add_header Cache-Control in their server blocks getting duplicate Cache-Control headers. Can we do this in code instead of configuration?

Owner

oschaaf commented Apr 5, 2013

@jeffkaufman
It might be worthwhile to have a look at copy_response_headers_to_ngx for this. Looking at it, it seems to always append headers using ngx_list_push. I can't take a close look myself right now, but that might explain it? Should we clear the current response headers before we copy over the response headers that we calculated?

Contributor

jeffkaufman commented Apr 15, 2013

What should we do about this? I'm inclined to just go with the empty add_header hack.

I don't think copy_response_headers_to_ngx will work because we're a content handler when we apply to a .pagespeed. resource, which means add_headers will run after us.

Owner

oschaaf commented Apr 15, 2013

@jeffkaufman "What should we do about this? I'm inclined to just go with the empty add_header hack."

Makes me a little sad, but I can't come up with something better at this moment.

Contributor

jeffkaufman commented Apr 15, 2013

Yeah, I don't like it either.

Ok to merge as is?

Owner

oschaaf commented Apr 15, 2013

Yes

jeffkaufman added a commit that referenced this pull request Apr 15, 2013

@jeffkaufman jeffkaufman merged commit b9a36fd into master Apr 15, 2013

@jeffkaufman jeffkaufman deleted the jefftk-test-caching-headers branch Apr 15, 2013

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