change: avoided generating Content-Type header when getting response …

[spacewander]

…headers.

The previous behavior assumed that Nginx will always generate the
Content-Type header if it is missing.
However, in some cases, like the upstream response doesn't have the Content-Type
header, Nginx will not generate the Content-Type header. So generating
Content-Type header when getting response headers may create an unexpected
Content-Type header in the response.

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

[thibaultcha]

Are we fine with breaking the behavior described in #92? Using the nginx configuration provided in the issue, the current master branch produces the following responses (which seem correct):

$ http :8080/set/  
HTTP/1.1 200 OK
Connection: keep-alive
Content-Type: text/html
Date: Fri, 04 Jan 2019 00:05:06 GMT
Server: nginx/1.13.6 (no pool)
Transfer-Encoding: chunked
x-hello: world

text/html

$ http :8080/noset/                      
HTTP/1.1 200 OK
Connection: keep-alive
Content-Type: text/html
Date: Fri, 04 Jan 2019 00:05:12 GMT
Server: nginx/1.13.6 (no pool)
Transfer-Encoding: chunked

text/html

But with this patch, the Content-Type header isn't even set in the first case, and is not retrieved by the ngx_lua API at all (while lua_use_default_type is enabled):

$ http :8080/set/
HTTP/1.1 200 OK
Connection: keep-alive
Date: Fri, 04 Jan 2019 00:04:14 GMT
Server: nginx/1.13.6 (no pool)
Transfer-Encoding: chunked
x-hello: world

nil

$ http :8080/noset/
HTTP/1.1 200 OK
Connection: keep-alive
Content-Type: text/html
Date: Fri, 04 Jan 2019 00:04:20 GMT
Server: nginx/1.13.6 (no pool)
Transfer-Encoding: chunked

nil

[spacewander]

@thibaultcha
I have submitted another PR to fix the problem you pointed out: #1449

[thibaultcha]

@spacewander It seems to work well 👍 However, since breaking the use case mentioned above is a regression introduced by this commit, the commit in your other PR (#1449) should be squashed into this PR (in the resulting commit). This is in order to ensure atomicity. After all, the behavior described above is not failing in the master branch, only because of this commit, so it is this commit that must be updated accordingly.

Will you close your other PR and squash its patch in this PR? Thanks!

[thibaultcha]

When rebasing #1449 on top of this branch, we do get the Content-Type header in the response as expected:

$ http :8080/set/
HTTP/1.1 200 OK
Connection: keep-alive
Content-Type: text/html
Date: Sat, 05 Jan 2019 03:10:52 GMT
Server: nginx/1.13.6 (no pool)
Transfer-Encoding: chunked
x-hello: world

nil

chasum@x1c ~ $ http :8080/noset/
HTTP/1.1 200 OK
Connection: keep-alive
Content-Type: text/html
Date: Sat, 05 Jan 2019 03:10:56 GMT
Server: nginx/1.13.6 (no pool)
Transfer-Encoding: chunked

nil

But we still experience a regression in that accessing ngx.header["content-type"] returns nil.

[spacewander]

However, since breaking the use case mentioned above is a regression introduced by this commit

It's my bad that I didn't mention that the regression is introduced by the 017e004. Currently, if one has set the response header (which doesn't need to be Content-Type), OpenResty will not generate the default Content-Type. This is OK before that commit, because we used to generate the Content-Type once the response header is set. You could reproduce the issue via replacing ngx.print(ngx.header["content-type"]) with ngx.print('ok').

But we still experience a regression in that accessing ngx.header["content-type"] returns nil.

I think it is not a regression, but a better way to deal with missing Content-Type header.
Previously, ngx.header["content-type"] returns the default type when there is not Content-Type header, because it assumed the default type will be used eventually. However, this assumption is incorrect (for instance, in the test cases in this PR).

Instead of overdoing, I think it would be better to return the real Content-Type header. When the header is missing, return nil instead of the default one. If you run ngx.header["content-type"] in header_filter_by_lua phase, you could get the final, correct Content-Type header.

[thibaultcha]

@spacewander Gotcha. I confirmed the regression indeed comes from 017e004 and pushed fixes and tweaks to your proposed patch in #1449. Thank you for clarifying.

I think it is not a regression, but a better way to deal with missing Content-Type header.

Yes, I have to say I agree.