Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Headers API #161

Closed
pintsized opened this issue Sep 28, 2012 · 7 comments
Closed

Headers API #161

pintsized opened this issue Sep 28, 2012 · 7 comments

Comments

@pintsized
Copy link
Contributor

I find myself grappling with issues of case sensitivity around request headers. I've implemented some work arounds in Lua land, but it's not pretty, and I noticed we have this task on the ngx_lua short term todo:

use ngx_hash_t to optimize the built-in header look-up process for ngx.req.set_header, ngx.header.HEADER, and etc.

I realise the above is probably just a performance task, to make lookups for the known headers_in faster, before scanning the whole list on a miss? But perhaps it's an opportunity to revisit the API here.

The issue is that ngx.var.http_HEADER gives you case insensitive lookup of header fields (which is correct), so long as they are single values (so you can't get a table back for multiple values of the same header key, at least as far as I can tell?), whilst ngx.req.get_headers()[HEADER] gives you case sensitive lookup, but with the added benefit of returning a table of header values where appropriate.

Moreover, the ngx.header.HEADER API looks different to the ngx.req.get_headers() API, which is a shame.

I'd like to see ngx.req.header.HEADER implemented much like the response headers API, where you access header fields case __in__sensitively, and can get / set tables for multiple values where appropriate.

ngx.req.get_headers() still has its use - to iterate over the request headers. And I note that ngx.header is not a normal Lua table, so you can't iterate over response headers at all?

I guess it feels like there are too many internal limitations passed onto Lua-land here. From what I can tell, ngx.header["Content-Type"] matches ngx.header.content_type internally, because it's a more direct mapping to the headers_out structure, whereas ngx.req.get_headers() returns a true Lua table as a copy of the headers, leaving us to deal with case ourselves.

Perhaps if the Lua table from ngx.req.get_headers() actually gave keys lowercased / underscored, and we set a metatable to normalise the lookup, that would help?

Personally I'd like to see a nice consistent request / response header API, but if that's not possible / desirable, I'd settle for any solution that doesn't pass on case sensitivity in field names to the Lua programmer.

Thoughts?

@agentzh
Copy link
Member

agentzh commented Sep 28, 2012

Hello!

On Fri, Sep 28, 2012 at 2:06 AM, James Hurst [email protected] wrote:

I find myself grappling with issues of case sensitivity around request headers. I've implemented some work arounds in Lua land, but it's not pretty, and I noticed we have this task on the ngx_lua short term todo:

use ngx_hash_t to optimize the built-in header look-up process for ngx.req.set_header, ngx.header.HEADER, and etc.

I realise the above is probably just a performance task, to make lookups for the known headers_in faster, before scanning the whole list on a miss?

Yes.

But perhaps it's an opportunity to revisit the API here.

The issue is that ngx.var.http_HEADER gives you case insensitive lookup of header fields (which is correct), so long as they are single values (so you can't get a table back for multiple values of the same header key, at least as far as I can tell?), whilst ngx.req.get_headers()[HEADER] gives you case sensitive lookup, but with the added benefit of returning a table of header values where appropriate.

Yes.

Moreover, the ngx.header.HEADER API looks different to the ngx.req.get_headers() API, which is a shame.

Yes, I changed my mind after introducing the former and right before
implementing the latter :)

The ngx.header.HEADER uses (in my opinon) too much magic for the sake
of usability.

I'd like to see ngx.req.header.HEADER implemented much like the response headers API, where you access header fields case insensitively, and can get / set tables for multiple values where appropriate.

ngx.req.get_headers() still has its use - to iterate over the request headers.

I don't like the idea of adding ngx.req.header.HEADER which mostly
overlaps with the existing ngx.req.header API in terms of real
functionalities.

How about adding new options to ngx.req.get_headers() so that it
normalizes the header keys to all lowercase letters and no
underscores? That is,

ngx.req.get_headers(convert_lettercase, convert_underscores)

such that ngx.req.get_headers(true, true)["content-type"] will work in
a more similar way as ngx.header["content-type"].

And I note that ngx.header is not a normal Lua table, so you can't iterate over response headers at all?

We can make it iterate-able because LuaJIT 2.0 and Lua 5.2
implements the __pairs metamethod, which can solve the problem here :)

I guess it feels like there are too many internal limitations passed onto Lua-land here. From what I can tell, ngx.header["Content-Type"] matches ngx.header.content_type internally, because it's a more direct mapping to the headers_out structure, whereas ngx.req.get_headers() returns a true Lua table as a copy of the headers, leaving us to deal with case ourselves.

Perhaps if the Lua table from ngx.req.get_headers() actually gave keys lowercased / underscored, and we set a metatable to normalise the lookup, that would help?

Yes, adding the __index metamethod for lookup misses sounds like a good idea :)

Personally I'd like to see a nice consistent request / response header API, but if that's not possible / desirable, I'd settle for any solution that doesn't pass on case sensitivity in field names to the Lua programmer.

I agree :)

Best regards,
-agentzh

@agentzh
Copy link
Member

agentzh commented Oct 2, 2012

I've updated the ngx.req.get_headers() per your request in the git "devel" branch of ngx_lua. Here's the updated documentation for this method:

http://wiki.nginx.org/HttpLuaModule#ngx.req.get_headers

Could you take a look at it? If it's OK, I'll merge it back to "master".

This change will be included in the 0.6.9 release of ngx_lua and the 1.2.3.7 devel release of ngx_openresty.

Thanks!

@pintsized
Copy link
Contributor Author

That looks great, thanks for turning it around so quickly!

I'm glad that the normalised version is the default, I think that does make the most sense. So generally speaking people will only use the "raw" option when iterating over the headers, perhaps to serialise them in their original form. For lookups, the default behaviour is to ignore case.

Can you elaborate on the doc note about ngx.var.http_HEADER being preferable in some cases? Is this just because the lookup for known headers_in will be faster since nginx has already prepared them for us? I'm really just thinking for people new to this, since it's not all that obvious on first glance.

Perhaps something like:

Note that if you are only interested in the last received value for a single request header, it may be faster to use the predefined ngx.var.http_HEADER form. If you plan on accessing most or all of the request headers, or need access to multiple instance values for a given header, you'll need the ngx.var.get_headers() form.

Presuming I'm right about the performance reasons?

@agentzh
Copy link
Member

agentzh commented Oct 3, 2012

Hello!

On Wed, Oct 3, 2012 at 3:32 AM, James Hurst [email protected] wrote:

That looks great, thanks for turning it around so quickly!

I'm glad that the normalised version is the default, I think that does make the most sense. So generally speaking people will only use the "raw" option when iterating over the headers, perhaps to serialise them in their original form. For lookups, the default behaviour is to ignore case.

Yes, exactly. Glad you like it :)

Can you elaborate on the doc note about ngx.var.http_HEADER being preferable in some cases? Is this just because the lookup for known headers_in will be faster since nginx has already prepared them for us? I'm really just thinking for people new to this, since it's not all that obvious on first glance.

Actually I won't ngx.var.http_HEADER is preferable, because of the
following reasons:

  1. The standard $http_HEADER variable will always lookup a header by
    iterating through the ngx_list_t list, exactly the same as
    ngx.req.get_headers(). So they're both of the O(n) complexity. You can
    check out the ngx_http_variable_unknown_header function in
    src/http/ngx_http_variables.c.
  2. $http_HEADER cannot handle multi-value headers.

Perhaps something like:

Note that if you are only interested in the last received value for a single request header, it may be faster to use the predefined ngx.var.http_HEADER form. If you plan on accessing most or all of the request headers, or need access to multiple instance values for a given header, you'll need the ngx.var.get_headers() form.

I think we can mention ngx.var.http_HEADER there, but not the
performance discussion because it's too specific to implementation
details.

Best regards,
-agentzh

@pintsized
Copy link
Contributor Author

Actually I won't ngx.var.http_HEADER is preferable, because of the
following reasons:

  1. The standard $http_HEADER variable will always lookup a header by
    iterating through the ngx_list_t list, exactly the same as
    ngx.req.get_headers(). So they're both of the O(n) complexity. You can
    check out the ngx_http_variable_unknown_header function in
    src/http/ngx_http_variables.c.
  2. $http_HEADER cannot handle multi-value headers.

So actually it's now no longer preferable at all? I see what you mean - it's hard to generalise the performance comparison. I just think that new users will appreciate having clear confidence in which method to choose, and clear guidance will lead to more consistent user-land code.

I almost feel like (based on what you're saying), the $http_HEADER form is more an internal convenience that's perhaps not really worth mentioning, since ngx.req.get_headers() does the Right Thing with headers?

I don't have a strong opinion, it just struck me as ambiguous.

@agentzh
Copy link
Member

agentzh commented Oct 4, 2012

Hello!

On Thu, Oct 4, 2012 at 1:42 AM, James Hurst [email protected] wrote:

I almost feel like (based on what you're saying), the $http_HEADER form is more an internal convenience that's perhaps not really worth mentioning, since ngx.req.get_headers() does the Right Thing with headers?

Exactly. I hope ngx.req.get_headers() become the One True Way of doing
things in ngx_lua :) And ngx.var.http_HEADER is just a quick and dirty
hack for trivial Lua scripts :)

I don't have a strong opinion, it just struck me as ambiguous.

The culture of Nginx is kinda like Perl, I must say, that is, "There's
More Than One Way to Do It!" ;)

Best regards,
-agentzh

@agentzh
Copy link
Member

agentzh commented Oct 4, 2012

Anyway, I'm closing this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants