From 9922de6d9e4e92efa6262f30f9dfb2f5e1a35f0d Mon Sep 17 00:00:00 2001 From: Magnus Klaar Date: Tue, 28 Feb 2012 18:35:26 +0100 Subject: Tests and fixes for the generate_etag/2 callback The return value from the generate_etag/2 callback is expected to be a binary tagged with either weak or strong. This binary is quoted, and may be prefixed with W/ before it is set as the value of the ETag header in the response. For backwards compatibility with older handlers where the return value was expected to be a quoted binary a function has been added to parse any return values that are untagged binaries. All untagged binaries are expected to be a valid value for the ETag header. --- src/cowboy_http_rest.erl | 31 ++++++++++++------- src/cowboy_http_static.erl | 34 ++++++++++----------- test/http_SUITE.erl | 71 +++++++++++++++++++++++++++++++++++++++----- test/rest_resource_etags.erl | 30 +++++++++++++++++++ 4 files changed, 130 insertions(+), 36 deletions(-) create mode 100644 test/rest_resource_etags.erl diff --git a/src/cowboy_http_rest.erl b/src/cowboy_http_rest.erl index ee67b36..8bce1d4 100644 --- a/src/cowboy_http_rest.erl +++ b/src/cowboy_http_rest.erl @@ -41,7 +41,7 @@ charset_a :: undefined | binary(), %% Cached resource calls. - etag :: undefined | no_call | binary(), + etag :: undefined | no_call | {strong | weak, binary()}, last_modified :: undefined | no_call | calendar:datetime(), expires :: undefined | no_call | calendar:datetime() }). @@ -487,14 +487,10 @@ if_match_exists(Req, State) -> if_match(Req, State, EtagsList) -> {Etag, Req2, State2} = generate_etag(Req, State), - case Etag of - no_call -> - precondition_failed(Req2, State2); - Etag -> - case lists:member(Etag, EtagsList) of - true -> if_unmodified_since_exists(Req2, State2); - false -> precondition_failed(Req2, State2) - end + case lists:member(Etag, EtagsList) of + true -> if_unmodified_since_exists(Req2, State2); + %% Etag may be `undefined' which cannot be a member. + false -> precondition_failed(Req2, State2) end. if_match_musnt_exist(Req, State) -> @@ -534,7 +530,7 @@ if_none_match_exists(Req, State) -> if_none_match(Req, State, EtagsList) -> {Etag, Req2, State2} = generate_etag(Req, State), case Etag of - no_call -> + undefined -> precondition_failed(Req2, State2); Etag -> case lists:member(Etag, EtagsList) of @@ -810,10 +806,14 @@ set_resp_etag(Req, State) -> {Req2, State2}; Etag -> {ok, Req3} = cowboy_http_req:set_resp_header( - <<"Etag">>, Etag, Req2), + <<"ETag">>, encode_etag(Etag), Req2), {Req3, State2} end. +-spec encode_etag({strong | weak, binary()}) -> iolist(). +encode_etag({strong, Etag}) -> [$",Etag,$"]; +encode_etag({weak, Etag}) -> ["W/\"",Etag,$"]. + set_resp_expires(Req, State) -> {Expires, Req2, State2} = expires(Req, State), case Expires of @@ -834,6 +834,15 @@ generate_etag(Req, State=#state{etag=undefined}) -> case call(Req, State, generate_etag) of no_call -> {undefined, Req, State#state{etag=no_call}}; + %% Previously the return value from the generate_etag/2 callback was set + %% as the value of the ETag header in the response. Therefore the only + %% valid return type was `binary()'. If a handler returns a `binary()' + %% it must be mapped to the expected type or it'll always fail to + %% compare equal to any entity tags present in the request headers. + %% @todo Remove support for binary return values after 0.6. + {Etag, Req2, HandlerState} when is_binary(Etag) -> + [Etag2] = cowboy_http:entity_tag_match(Etag), + {Etag2, Req2, State#state{handler_state=HandlerState, etag=Etag2}}; {Etag, Req2, HandlerState} -> {Etag, Req2, State#state{handler_state=HandlerState, etag=Etag}} end; diff --git a/src/cowboy_http_static.erl b/src/cowboy_http_static.erl index 0ee996a..007cd16 100644 --- a/src/cowboy_http_static.erl +++ b/src/cowboy_http_static.erl @@ -96,15 +96,16 @@ %% %% The default behaviour can be overridden to generate an ETag header based on %% a combination of the file path, file size, inode and mtime values. If the -%% option value is a list of attribute names tagged with `attributes' a hex -%% encoded CRC32 checksum of the attribute values are used as the ETag header -%% value. +%% option value is a non-empty list of attribute names tagged with `attributes' +%% a hex encoded checksum of each attribute specified is included in the value +%% of the the ETag header. If the list of attribute names is empty no ETag +%% header is generated. %% %% If a strong ETag is required a user defined function for generating the %% header value can be supplied. The function must accept a proplist of the %% file attributes as the first argument and a second argument containing any -%% additional data that the function requires. The function must return a -%% `binary()' or `undefined'. +%% additional data that the function requires. The function must return a term +%% of the type `{weak | strong, binary()}' or `undefined'. %% %% ==== Examples ==== %% ``` @@ -130,7 +131,7 @@ %% {_, _Modified} = lists:keyfind(mtime, 1, Arguments), %% ChecksumCommand = lists:flatten(io_lib:format("sha1sum ~s", [Filepath])), %% [Checksum|_] = string:tokens(os:cmd(ChecksumCommand), " "), -%% iolist_to_binary(Checksum). +%% {strong, iolist_to_binary(Checksum)}. %% ''' -module(cowboy_http_static). @@ -161,7 +162,8 @@ filepath :: binary() | error, fileinfo :: {ok, #file_info{}} | {error, _} | error, mimetypes :: {fun((binary(), T) -> [mimedef()]), T} | undefined, - etag_fun :: {fun(([etagarg()], T) -> undefined | binary()), T}}). + etag_fun :: {fun(([etagarg()], T) -> + undefined | {strong | weak, binary()}), T}}). %% @private Upgrade from HTTP handler to REST handler. @@ -183,8 +185,9 @@ rest_init(Req, Opts) -> ETagFunction = case proplists:get_value(etag, Opts) of default -> {fun no_etag_function/2, undefined}; undefined -> {fun no_etag_function/2, undefined}; + {attributes, []} -> {fun no_etag_function/2, undefined}; {attributes, Attrs} -> {fun attr_etag_function/2, Attrs}; - {_, _}=EtagFunction1 -> EtagFunction1 + {_, _}=ETagFunction1 -> ETagFunction1 end, {Filepath, Req1} = cowboy_http_req:path_info(Req), State = case check_path(Filepath) of @@ -411,16 +414,13 @@ no_etag_function(_Args, undefined) -> %% @private A simple alternative is to send an ETag based on file attributes. -type fileattr() :: filepath | filesize | mtime | inode. --spec attr_etag_function([etagarg()], [fileattr()]) -> binary(). +-spec attr_etag_function([etagarg()], [fileattr()]) -> {strong, binary()}. attr_etag_function(Args, Attrs) -> - attr_etag_function(Args, Attrs, []). - --spec attr_etag_function([etagarg()], [fileattr()], [binary()]) -> binary(). -attr_etag_function(_Args, [], Acc) -> - list_to_binary(integer_to_list(erlang:crc32(Acc), 16)); -attr_etag_function(Args, [H|T], Acc) -> - {_, Value} = lists:keyfind(H, 1, Args), - attr_etag_function(Args, T, [term_to_binary(Value)|Acc]). + [[_|H]|T] = [begin + {_,Pair} = {_,{_,_}} = {Attr,lists:keyfind(Attr, 1, Args)}, + [$-|integer_to_list(erlang:phash2(Pair, 1 bsl 32), 16)] + end || Attr <- Attrs], + {strong, list_to_binary([H|T])}. -ifdef(TEST). diff --git a/test/http_SUITE.erl b/test/http_SUITE.erl index 9191f26..72892eb 100644 --- a/test/http_SUITE.erl +++ b/test/http_SUITE.erl @@ -29,7 +29,8 @@ file_200/1, file_403/1, dir_403/1, file_404/1, file_400/1]). %% http and https. -export([http_10_hostless/1]). %% misc. --export([rest_simple/1, rest_keepalive/1, rest_keepalive_post/1, rest_nodelete/1]). %% rest. +-export([rest_simple/1, rest_keepalive/1, rest_keepalive_post/1, + rest_nodelete/1, rest_resource_etags/1]). %% rest. %% ct. @@ -47,7 +48,8 @@ groups() -> static_function_etag, multipart] ++ BaseTests}, {https, [], BaseTests}, {misc, [], [http_10_hostless]}, - {rest, [], [rest_simple, rest_keepalive, rest_keepalive_post, rest_nodelete]}]. + {rest, [], [rest_simple, rest_keepalive, rest_keepalive_post, + rest_nodelete, rest_resource_etags]}]. init_per_suite(Config) -> application:start(inets), @@ -75,7 +77,7 @@ init_per_group(https, Config) -> application:start(public_key), application:start(ssl), DataDir = ?config(data_dir, Config), - cowboy:start_listener(https, 100, + {ok,_} = cowboy:start_listener(https, 100, cowboy_ssl_transport, [ {port, Port}, {certfile, DataDir ++ "cert.pem"}, {keyfile, DataDir ++ "key.pem"}, {password, "cowboy"}], @@ -84,7 +86,7 @@ init_per_group(https, Config) -> [{scheme, "https"}, {port, Port}|Config1]; init_per_group(misc, Config) -> Port = 33082, - cowboy:start_listener(misc, 100, + {ok,_} = cowboy:start_listener(misc, 100, cowboy_tcp_transport, [{port, Port}], cowboy_http_protocol, [{dispatch, [{'_', [ {[], http_handler, []} @@ -92,15 +94,16 @@ init_per_group(misc, Config) -> [{port, Port}|Config]; init_per_group(rest, Config) -> Port = 33083, - cowboy:start_listener(reset, 100, + {ok,_} = cowboy:start_listener(rest, 100, cowboy_tcp_transport, [{port, Port}], cowboy_http_protocol, [{dispatch, [{'_', [ {[<<"simple">>], rest_simple_resource, []}, {[<<"forbidden_post">>], rest_forbidden_resource, [true]}, {[<<"simple_post">>], rest_forbidden_resource, [false]}, - {[<<"nodelete">>], rest_nodelete_resource, []} + {[<<"nodelete">>], rest_nodelete_resource, []}, + {[<<"resetags">>], rest_resource_etags, []} ]}]}]), - [{port, Port}|Config]. + [{scheme, "http"},{port, Port}|Config]. end_per_group(https, Config) -> cowboy:stop_listener(https), @@ -512,7 +515,7 @@ static_function_etag(Arguments, etag_data) -> {_, _Modified} = lists:keyfind(mtime, 1, Arguments), ChecksumCommand = lists:flatten(io_lib:format("sha1sum ~s", [Filepath])), [Checksum|_] = string:tokens(os:cmd(ChecksumCommand), " "), - iolist_to_binary(Checksum). + {strong, iolist_to_binary(Checksum)}. %% http and https. @@ -623,3 +626,55 @@ rest_nodelete(Config) -> {ok, Data} = gen_tcp:recv(Socket, 0, 6000), {0, 12} = binary:match(Data, <<"HTTP/1.1 500">>), ok = gen_tcp:close(Socket). + +rest_resource_etags(Config) -> + %% The Etag header should be set to the return value of generate_etag/2. + fun() -> + %% Correct return values from generate_etag/2. + {Packet1, 200} = raw_resp([ + "GET /resetags?type=tuple-weak HTTP/1.1\r\n", + "Host: localhost\r\n", "Connection: close\r\n", "\r\n"], Config), + {_,_} = binary:match(Packet1, <<"ETag: W/\"etag-header-value\"\r\n">>), + {Packet2, 200} = raw_resp([ + "GET /resetags?type=tuple-strong HTTP/1.1\r\n", + "Host: localhost\r\n", "Connection: close\r\n", "\r\n"], Config), + {_,_} = binary:match(Packet2, <<"ETag: \"etag-header-value\"\r\n">>), + %% Backwards compatible return values from generate_etag/2. + {Packet3, 200} = raw_resp([ + "GET /resetags?type=binary-weak-quoted HTTP/1.1\r\n", + "Host: localhost\r\n", "Connection: close\r\n", "\r\n"], Config), + {_,_} = binary:match(Packet3, <<"ETag: W/\"etag-header-value\"\r\n">>), + {Packet4, 200} = raw_resp([ + "GET /resetags?type=binary-strong-quoted HTTP/1.1\r\n", + "Host: localhost\r\n", "Connection: close\r\n", "\r\n"], Config), + {_,_} = binary:match(Packet4, <<"ETag: \"etag-header-value\"\r\n">>), + %% Invalid return values from generate_etag/2. + {_Packet5, 500} = raw_resp([ + "GET /resetags?type=binary-strong-unquoted HTTP/1.1\r\n", + "Host: localhost\r\n", "Connection: close\r\n", "\r\n"], Config), + {_Packet6, 500} = raw_resp([ + "GET /resetags?type=binary-weak-unquoted HTTP/1.1\r\n", + "Host: localhost\r\n", "Connection: close\r\n", "\r\n"], Config) + end(), + + %% The return value of generate_etag/2 should match the request header. + fun() -> + %% Correct return values from generate_etag/2. + {_Packet1, 304} = raw_resp([ + "GET /resetags?type=tuple-weak HTTP/1.1\r\n", + "Host: localhost\r\n", "Connection: close\r\n", + "If-None-Match: W/\"etag-header-value\"\r\n", "\r\n"], Config), + {_Packet2, 304} = raw_resp([ + "GET /resetags?type=tuple-strong HTTP/1.1\r\n", + "Host: localhost\r\n", "Connection: close\r\n", + "If-None-Match: \"etag-header-value\"\r\n", "\r\n"], Config), + %% Backwards compatible return values from generate_etag/2. + {_Packet3, 304} = raw_resp([ + "GET /resetags?type=binary-weak-quoted HTTP/1.1\r\n", + "Host: localhost\r\n", "Connection: close\r\n", + "If-None-Match: W/\"etag-header-value\"\r\n", "\r\n"], Config), + {_Packet4, 304} = raw_resp([ + "GET /resetags?type=binary-strong-quoted HTTP/1.1\r\n", + "Host: localhost\r\n", "Connection: close\r\n", + "If-None-Match: \"etag-header-value\"\r\n", "\r\n"], Config) + end(). diff --git a/test/rest_resource_etags.erl b/test/rest_resource_etags.erl new file mode 100644 index 0000000..b21aa9f --- /dev/null +++ b/test/rest_resource_etags.erl @@ -0,0 +1,30 @@ +-module(rest_resource_etags). +-export([init/3, generate_etag/2, content_types_provided/2, get_text_plain/2]). + +init(_Transport, _Req, _Opts) -> + {upgrade, protocol, cowboy_http_rest}. + +generate_etag(Req, State) -> + case cowboy_http_req:qs_val(<<"type">>, Req) of + %% Correct return values from generate_etag/2. + {<<"tuple-weak">>, Req2} -> + {{weak, <<"etag-header-value">>}, Req2, State}; + {<<"tuple-strong">>, Req2} -> + {{strong, <<"etag-header-value">>}, Req2, State}; + %% Backwards compatible return values from generate_etag/2. + {<<"binary-weak-quoted">>, Req2} -> + {<<"W/\"etag-header-value\"">>, Req2, State}; + {<<"binary-strong-quoted">>, Req2} -> + {<<"\"etag-header-value\"">>, Req2, State}; + %% Invalid return values from generate_etag/2. + {<<"binary-strong-unquoted">>, Req2} -> + {<<"etag-header-value">>, Req2, State}; + {<<"binary-weak-unquoted">>, Req2} -> + {<<"W/etag-header-value">>, Req2, State} + end. + +content_types_provided(Req, State) -> + {[{{<<"text">>, <<"plain">>, []}, get_text_plain}], Req, State}. + +get_text_plain(Req, State) -> + {<<"This is REST!">>, Req, State}. -- cgit v1.2.3