From 5625faf4003df9894cecc1b4d0079268b8a15d1c Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Thu, 27 Feb 2020 17:04:58 -0500 Subject: [PATCH] Fix previous OTP-23 compat patches - Normalize path behaviour to always be "/" across versions - Normalize port visibility behaviour to match input string when appending - restore rebar_utils function to avoid breaking random plugins that peek into our libs for their stuff - Support URIOpts equivalent and protocol-based overrides; the behaviour there is not necessarily consistent across versions in terms of what ports are returned (http_uri always returned a port but uri_string only returns it if explicitly specified -- we choose the latter behaviour on newer releases), but the calls work to ensure URI parsing consistently works across versions The latter judgement call is a bit of an odd one; for consistency we could always mandate ports, but this would come at a performance penalty when appending URL paths (i.e. when fetching packages). The reason for this is that we standardize on the new uri_string behaviour for path appending (if the port wasn't specified, we don't add it), and with the http_uri rules, we need to do a kind of parsing round that checks if the port was included or not to make it equivalent. This is costly, and _not_ returning the port when it isn't specified lets us do this transparently. This allows to maintain backwards compat in older append functions, and nobody aside from us currently uses the new rebar_uri module so we can decide to introduce this potential inconsistency if we wish to. --- src/rebar_git_resource.erl | 9 +---- src/rebar_uri.erl | 81 ++++++++++++++++++++++++++++++++------ src/rebar_utils.erl | 4 ++ test/rebar_uri_SUITE.erl | 34 ++++++++-------- 4 files changed, 93 insertions(+), 35 deletions(-) diff --git a/src/rebar_git_resource.erl b/src/rebar_git_resource.erl index 808f1f8f..4a37a02b 100644 --- a/src/rebar_git_resource.erl +++ b/src/rebar_git_resource.erl @@ -100,13 +100,6 @@ compare_url(Dir, Url) -> ?DEBUG("Comparing git url ~p with ~p", [ParsedUrl, ParsedCurrentUrl]), ParsedCurrentUrl =:= ParsedUrl. --ifdef (OTP_RELEASE). - -if(?OTP_RELEASE >= 23). - -compile({nowarn_deprecated_function, [{http_uri, parse, 2}, - {http_uri, scheme_defaults, 0}]}). - -endif. --endif. - parse_git_url(Url) -> %% Checks for standard scp style git remote case re:run(Url, ?SCP_PATTERN, [{capture, [host, path], list}, unicode]) of @@ -116,7 +109,7 @@ parse_git_url(Url) -> parse_git_url(not_scp, Url) end. parse_git_url(not_scp, Url) -> - UriOpts = [{scheme_defaults, [{git, 9418} | http_uri:scheme_defaults()]}], + UriOpts = [{scheme_defaults, [{git, 9418} | rebar_uri:scheme_defaults()]}], case rebar_uri:parse(Url, UriOpts) of #{path := Path, host := Host} -> {ok, {Host, filename:rootname(Path, ".git")}}; diff --git a/src/rebar_uri.erl b/src/rebar_uri.erl index 3a6382b1..a52f0e02 100644 --- a/src/rebar_uri.erl +++ b/src/rebar_uri.erl @@ -2,26 +2,33 @@ -module(rebar_uri). -export([ - parse/1, + parse/1, parse/2, scheme_defaults/0, append_path/2 ]). --import(rebar_utils, [to_list/1]). - --ifdef (OTP_RELEASE). +-ifdef(OTP_RELEASE). -spec parse(URIString) -> URIMap when URIString :: uri_string:uri_string(), URIMap :: uri_string:uri_map() | uri_string:error(). parse(URIString) -> - uri_string:parse(URIString). + parse(URIString, []). + +parse(URIString, URIOpts) -> + case uri_string:parse(URIString) of + #{path := ""} = Map -> apply_opts(Map#{path => "/"}, URIOpts); + Map -> apply_opts(Map, URIOpts) + end. -else. -spec parse(URIString) -> URIMap when URIString :: iodata(), URIMap :: map() | {error, atom(), term()}. parse(URIString) -> - case http_uri:parse(URIString) of + parse(URIString, []). + +parse(URIString, URIOpts) -> + case http_uri:parse(URIString, URIOpts) of {error, Reason} -> %% no additional parser/term info available to us, %% e.g. see what uri_string returns in @@ -46,11 +53,11 @@ parse(URIString) -> -endif. %% OTP 21+ --ifdef (OTP_RELEASE). +-ifdef(OTP_RELEASE). append_path(Url, ExtraPath) -> case parse(Url) of #{path := Path} = Map -> - FullPath = filename:join(Path, ExtraPath), + FullPath = join(Path, ExtraPath), {ok, uri_string:recompose(maps:update(path, FullPath, Map))}; _ -> error @@ -58,14 +65,66 @@ append_path(Url, ExtraPath) -> -else. append_path(Url, ExtraPath) -> case parse(Url) of - #{scheme := Scheme, userinfo := UserInfo, host := Host, port := Port, path := Path, query := Query} -> + #{scheme := Scheme, userinfo := UserInfo, host := Host, + port := Port, path := Path, query := Query} -> + ListScheme = rebar_utils:to_list(Scheme), PrefixedQuery = case Query of [] -> []; Other -> lists:append(["?", Other]) end, - {ok, lists:append([to_list(Scheme), "://", UserInfo, Host, ":", to_list(Port), - filename:join(Path, ExtraPath), PrefixedQuery])}; + NormPath = case Path of + "" -> "/"; + _ -> Path + end, + {ok, maybe_port( + Url, lists:append([ListScheme, "://", UserInfo, Host]), + [$: | rebar_utils:to_list(Port)], + lists:append([join(NormPath, ExtraPath), PrefixedQuery]) + )}; _ -> error end. -endif. + +%% OTP 21+ +-ifdef(OTP_RELEASE). +scheme_defaults() -> + %% no scheme defaults here; just custom ones + []. +-else. +scheme_defaults() -> + http_uri:scheme_defaults(). +-endif. + +join(URI, "") -> URI; +join(URI, "/") -> URI; +join("/", [$/|_] = Path) -> Path; +join("/", Path) -> [$/ | Path]; +join("", [$/|_] = Path) -> Path; +join("", Path) -> [$/ | Path]; +join([H|T], Path) -> [H | join(T, Path)]. + + +-ifdef(OTP_RELEASE). +apply_opts(Map = #{port := _}, _) -> + Map; +apply_opts(Map = #{scheme := Scheme}, URIOpts) -> + SchemeDefaults = proplists:get_value(scheme_defaults, URIOpts, []), + %% Here is the funky bit: don't add the port number if it's in a default + %% to maintain proper default behaviour. + try lists:keyfind(list_to_existing_atom(Scheme), 1, SchemeDefaults) of + {_, Port} -> + Map#{port => Port}; + false -> + Map + catch + error:badarg -> % not an existing atom, not in the list + Map + end. +-else. +maybe_port(Url, Host, Port, PathQ) -> + case lists:prefix(Host ++ Port, Url) of + true -> Host ++ Port ++ PathQ; % port was explicit + false -> Host ++ PathQ % port was implicit + end. +-endif. diff --git a/src/rebar_utils.erl b/src/rebar_utils.erl index 6be50f14..0933bb6c 100644 --- a/src/rebar_utils.erl +++ b/src/rebar_utils.erl @@ -64,6 +64,7 @@ tup_find/2, line_count/1, set_httpc_options/0, + url_append_path/2, escape_chars/1, escape_double_quotes/1, escape_double_quotes_weak/1, @@ -938,6 +939,9 @@ normalise_proxy(Scheme, URI) -> _ -> URI end. +url_append_path(Url, ExtraPath) -> + rebar_uri:append_path(Url, ExtraPath). + %% escape\ as\ a\ shell\? escape_chars(Str) when is_atom(Str) -> escape_chars(atom_to_list(Str)); diff --git a/test/rebar_uri_SUITE.erl b/test/rebar_uri_SUITE.erl index 4db93690..a7fde27b 100644 --- a/test/rebar_uri_SUITE.erl +++ b/test/rebar_uri_SUITE.erl @@ -9,20 +9,20 @@ -include_lib("kernel/include/file.hrl"). all() -> - [parse]. + [parse, append_path]. parse(_Config) -> #{scheme := Scheme, host := Host, path := Path} = rebar_uri:parse("https://repo.hex.pm"), ?assertEqual("https", Scheme), ?assertEqual("repo.hex.pm", Host), - ?assert(lists:member(Path, ["", "/"])), + ?assertEqual(Path, "/"), % Normalize on OTP-23 behaviour. #{scheme := Scheme2, host := Host2, port := Port2, path := Path2, query := Query2} = rebar_uri:parse("https://repo.hex.pm:443?foo=bar"), ?assertEqual("https", Scheme2), ?assertEqual("repo.hex.pm", Host2), ?assertEqual(443, Port2), - ?assert(lists:member(Path2, ["", "/"])), + ?assertEqual(Path2, "/"), % Normalize on old http_uri behaviour ?assertEqual("foo=bar", Query2), #{scheme := Scheme3, host := Host3, path := Path3, query := Query3} = @@ -30,19 +30,21 @@ parse(_Config) -> ?assertEqual("https", Scheme3), ?assertEqual("repo.hex.pm", Host3), ?assertEqual("/over/here", Path3), - ?assertEqual("foo=bar", Query3). + ?assertEqual("foo=bar", Query3), + + %% override default port and get it parsed as such + ?assertMatch(#{port := 1337}, + rebar_uri:parse("https://repo.hex.pm/", + [{scheme_defaults, [{https,1337}]}])), + ok. append_path(_Config) -> - %% OTP version differences - {ok, Val1} = rebar_utils:append_path("https://repo.hex.pm", "/repos/org"), - ?assert(lists:member(Val1, [ - "https://repo.hex.pm/repos/org", - "https://repo.hex.pm:443/repos/org" - ])), - {ok, Val2} = rebar_utils:append_path("https://repo.hex.pm?foo=bar", "/repos/org"), - ?assert(lists:member(Val2, [ - "https://repo.hex.pm/repos/org?foo=bar", - "https://repo.hex.pm:443/repos/org?foo=bar" - ])), + %% Default port for the proto is omitted if not mentioned originally + {ok, Val1} = rebar_uri:append_path("https://repo.hex.pm/", "/repos/org"), + ?assertEqual("https://repo.hex.pm/repos/org", Val1), + %% QS elements come after the path + {ok, Val2} = rebar_uri:append_path("https://repo.hex.pm?foo=bar", "/repos/org"), + ?assertEqual("https://repo.hex.pm/repos/org?foo=bar", Val2), + %% If the port is explicitly mentioned, keep it. ?assertEqual({ok, "https://repo.hex.pm:443/repos/org?foo=bar"}, - rebar_utils:append_path("https://repo.hex.pm:443?foo=bar", "/repos/org")). + rebar_uri:append_path("https://repo.hex.pm:443?foo=bar", "/repos/org")).