From cac31abb7f3643a95d9c8ebb4b7b348b5a67c633 Mon Sep 17 00:00:00 2001 From: Ville Aikas <11279988+vaikas@users.noreply.github.com> Date: Wed, 27 Nov 2019 13:13:22 -0800 Subject: [PATCH] Change URIFromDestinationV1 to return apis.URL instead of string (#909) --- apis/url.go | 17 +++ apis/url_test.go | 143 ++++++++++++++++++++++++++ resolver/addressable_resolver.go | 22 ++-- resolver/addressable_resolver_test.go | 10 +- 4 files changed, 176 insertions(+), 16 deletions(-) diff --git a/apis/url.go b/apis/url.go index b05990756..55c9260d5 100644 --- a/apis/url.go +++ b/apis/url.go @@ -109,3 +109,20 @@ func (u *URL) URL() *url.URL { url := url.URL(*u) return &url } + +// ResolveReference calls the underlying ResolveReference method +// and returns an apis.URL +func (u *URL) ResolveReference(ref *URL) *URL { + if ref == nil { + return u + } + // Turn both u / ref to url.URL + uRef := url.URL(*ref) + uu := url.URL(*u) + + newU := uu.ResolveReference(&uRef) + + // Turn new back to apis.URL + ret := URL(*newU) + return &ret +} diff --git a/apis/url_test.go b/apis/url_test.go index 79ff05cb0..f06d4141f 100644 --- a/apis/url_test.go +++ b/apis/url_test.go @@ -460,3 +460,146 @@ func TestURLString(t *testing.T) { }) } } + +// These are lifted from the net/url url_test.go +var resolveReferenceTests = []struct { + base, rel, expected string +}{ + // Absolute URL references + {"http://foo.com?a=b", "https://bar.com/", "https://bar.com/"}, + {"http://foo.com/", "https://bar.com/?a=b", "https://bar.com/?a=b"}, + {"http://foo.com/", "https://bar.com/?", "https://bar.com/?"}, + {"http://foo.com/bar", "mailto:foo@example.com", "mailto:foo@example.com"}, + + // Path-absolute references + {"http://foo.com/bar", "/baz", "http://foo.com/baz"}, + {"http://foo.com/bar?a=b#f", "/baz", "http://foo.com/baz"}, + {"http://foo.com/bar?a=b", "/baz?", "http://foo.com/baz?"}, + {"http://foo.com/bar?a=b", "/baz?c=d", "http://foo.com/baz?c=d"}, + + // Multiple slashes + {"http://foo.com/bar", "http://foo.com//baz", "http://foo.com//baz"}, + {"http://foo.com/bar", "http://foo.com///baz/quux", "http://foo.com///baz/quux"}, + + // Scheme-relative + {"https://foo.com/bar?a=b", "//bar.com/quux", "https://bar.com/quux"}, + + // Path-relative references: + + // ... current directory + {"http://foo.com", ".", "http://foo.com/"}, + {"http://foo.com/bar", ".", "http://foo.com/"}, + {"http://foo.com/bar/", ".", "http://foo.com/bar/"}, + + // ... going down + {"http://foo.com", "bar", "http://foo.com/bar"}, + {"http://foo.com/", "bar", "http://foo.com/bar"}, + {"http://foo.com/bar/baz", "quux", "http://foo.com/bar/quux"}, + + // ... going up + {"http://foo.com/bar/baz", "../quux", "http://foo.com/quux"}, + {"http://foo.com/bar/baz", "../../../../../quux", "http://foo.com/quux"}, + {"http://foo.com/bar", "..", "http://foo.com/"}, + {"http://foo.com/bar/baz", "./..", "http://foo.com/"}, + // ".." in the middle (issue 3560) + {"http://foo.com/bar/baz", "quux/dotdot/../tail", "http://foo.com/bar/quux/tail"}, + {"http://foo.com/bar/baz", "quux/./dotdot/../tail", "http://foo.com/bar/quux/tail"}, + {"http://foo.com/bar/baz", "quux/./dotdot/.././tail", "http://foo.com/bar/quux/tail"}, + {"http://foo.com/bar/baz", "quux/./dotdot/./../tail", "http://foo.com/bar/quux/tail"}, + {"http://foo.com/bar/baz", "quux/./dotdot/dotdot/././../../tail", "http://foo.com/bar/quux/tail"}, + {"http://foo.com/bar/baz", "quux/./dotdot/dotdot/./.././../tail", "http://foo.com/bar/quux/tail"}, + {"http://foo.com/bar/baz", "quux/./dotdot/dotdot/dotdot/./../../.././././tail", "http://foo.com/bar/quux/tail"}, + {"http://foo.com/bar/baz", "quux/./dotdot/../dotdot/../dot/./tail/..", "http://foo.com/bar/quux/dot/"}, + + // Remove any dot-segments prior to forming the target URI. + // http://tools.ietf.org/html/rfc3986#section-5.2.4 + {"http://foo.com/dot/./dotdot/../foo/bar", "../baz", "http://foo.com/dot/baz"}, + + // Triple dot isn't special + {"http://foo.com/bar", "...", "http://foo.com/..."}, + + // Fragment + {"http://foo.com/bar", ".#frag", "http://foo.com/#frag"}, + {"http://example.org/", "#!$&%27()*+,;=", "http://example.org/#!$&%27()*+,;="}, + + // Paths with escaping (issue 16947). + {"http://foo.com/foo%2fbar/", "../baz", "http://foo.com/baz"}, + {"http://foo.com/1/2%2f/3%2f4/5", "../../a/b/c", "http://foo.com/1/a/b/c"}, + {"http://foo.com/1/2/3", "./a%2f../../b/..%2fc", "http://foo.com/1/2/b/..%2fc"}, + {"http://foo.com/1/2%2f/3%2f4/5", "./a%2f../b/../c", "http://foo.com/1/2%2f/3%2f4/a%2f../c"}, + {"http://foo.com/foo%20bar/", "../baz", "http://foo.com/baz"}, + {"http://foo.com/foo", "../bar%2fbaz", "http://foo.com/bar%2fbaz"}, + {"http://foo.com/foo%2dbar/", "./baz-quux", "http://foo.com/foo%2dbar/baz-quux"}, + + // RFC 3986: Normal Examples + // http://tools.ietf.org/html/rfc3986#section-5.4.1 + {"http://a/b/c/d;p?q", "g:h", "g:h"}, + {"http://a/b/c/d;p?q", "g", "http://a/b/c/g"}, + {"http://a/b/c/d;p?q", "./g", "http://a/b/c/g"}, + {"http://a/b/c/d;p?q", "g/", "http://a/b/c/g/"}, + {"http://a/b/c/d;p?q", "/g", "http://a/g"}, + {"http://a/b/c/d;p?q", "//g", "http://g"}, + {"http://a/b/c/d;p?q", "?y", "http://a/b/c/d;p?y"}, + {"http://a/b/c/d;p?q", "g?y", "http://a/b/c/g?y"}, + {"http://a/b/c/d;p?q", "#s", "http://a/b/c/d;p?q#s"}, + {"http://a/b/c/d;p?q", "g#s", "http://a/b/c/g#s"}, + {"http://a/b/c/d;p?q", "g?y#s", "http://a/b/c/g?y#s"}, + {"http://a/b/c/d;p?q", ";x", "http://a/b/c/;x"}, + {"http://a/b/c/d;p?q", "g;x", "http://a/b/c/g;x"}, + {"http://a/b/c/d;p?q", "g;x?y#s", "http://a/b/c/g;x?y#s"}, + {"http://a/b/c/d;p?q", "", "http://a/b/c/d;p?q"}, + {"http://a/b/c/d;p?q", ".", "http://a/b/c/"}, + {"http://a/b/c/d;p?q", "./", "http://a/b/c/"}, + {"http://a/b/c/d;p?q", "..", "http://a/b/"}, + {"http://a/b/c/d;p?q", "../", "http://a/b/"}, + {"http://a/b/c/d;p?q", "../g", "http://a/b/g"}, + {"http://a/b/c/d;p?q", "../..", "http://a/"}, + {"http://a/b/c/d;p?q", "../../", "http://a/"}, + {"http://a/b/c/d;p?q", "../../g", "http://a/g"}, + + // RFC 3986: Abnormal Examples + // http://tools.ietf.org/html/rfc3986#section-5.4.2 + {"http://a/b/c/d;p?q", "../../../g", "http://a/g"}, + {"http://a/b/c/d;p?q", "../../../../g", "http://a/g"}, + {"http://a/b/c/d;p?q", "/./g", "http://a/g"}, + {"http://a/b/c/d;p?q", "/../g", "http://a/g"}, + {"http://a/b/c/d;p?q", "g.", "http://a/b/c/g."}, + {"http://a/b/c/d;p?q", ".g", "http://a/b/c/.g"}, + {"http://a/b/c/d;p?q", "g..", "http://a/b/c/g.."}, + {"http://a/b/c/d;p?q", "..g", "http://a/b/c/..g"}, + {"http://a/b/c/d;p?q", "./../g", "http://a/b/g"}, + {"http://a/b/c/d;p?q", "./g/.", "http://a/b/c/g/"}, + {"http://a/b/c/d;p?q", "g/./h", "http://a/b/c/g/h"}, + {"http://a/b/c/d;p?q", "g/../h", "http://a/b/c/h"}, + {"http://a/b/c/d;p?q", "g;x=1/./y", "http://a/b/c/g;x=1/y"}, + {"http://a/b/c/d;p?q", "g;x=1/../y", "http://a/b/c/y"}, + {"http://a/b/c/d;p?q", "g?y/./x", "http://a/b/c/g?y/./x"}, + {"http://a/b/c/d;p?q", "g?y/../x", "http://a/b/c/g?y/../x"}, + {"http://a/b/c/d;p?q", "g#s/./x", "http://a/b/c/g#s/./x"}, + {"http://a/b/c/d;p?q", "g#s/../x", "http://a/b/c/g#s/../x"}, + + // Extras. + {"https://a/b/c/d;p?q", "//g?q", "https://g?q"}, + {"https://a/b/c/d;p?q", "//g#s", "https://g#s"}, + {"https://a/b/c/d;p?q", "//g/d/e/f?y#s", "https://g/d/e/f?y#s"}, + {"https://a/b/c/d;p#s", "?y", "https://a/b/c/d;p?y"}, + {"https://a/b/c/d;p?q#s", "?y", "https://a/b/c/d;p?y"}, +} + +func TestResolveReference(t *testing.T) { + mustParse := func(url string) *URL { + u, err := ParseURL(url) + if err != nil { + t.Fatalf("Parse(%q) got err %v", url, err) + } + return u + } + + for _, tc := range resolveReferenceTests { + apisURL := mustParse(tc.base) + apisRel := mustParse(tc.rel) + if got := apisURL.ResolveReference(apisRel).String(); got != tc.expected { + t.Errorf("URL(%q).ResolveReference(%q)\ngot %q\nwant %q", tc.base, tc.rel, got, tc.expected) + } + } +} diff --git a/resolver/addressable_resolver.go b/resolver/addressable_resolver.go index 52591c974..8d64f30d8 100644 --- a/resolver/addressable_resolver.go +++ b/resolver/addressable_resolver.go @@ -89,7 +89,7 @@ func (r *URIResolver) URIFromDestination(dest duckv1beta1.Destination, parent in if dest.URI.URL().IsAbs() { return "", errors.New("absolute URI is not allowed when Ref or [apiVersion, kind, name] exists") } - return url.URL().ResolveReference(dest.URI.URL()).String(), nil + return url.ResolveReference(dest.URI).String(), nil } return url.URL().String(), nil } @@ -97,7 +97,7 @@ func (r *URIResolver) URIFromDestination(dest duckv1beta1.Destination, parent in if dest.URI != nil { // IsAbs check whether the URL has a non-empty scheme. Besides the non non-empty scheme, we also require dest.URI has a non-empty host if !dest.URI.URL().IsAbs() || dest.URI.Host == "" { - return "", fmt.Errorf("URI is not absolute(both scheme and host should be non-empty): %v", dest.URI.String()) + return "", fmt.Errorf("URI is not absolute(both scheme and host should be non-empty): %q", dest.URI.String()) } return dest.URI.String(), nil } @@ -105,31 +105,31 @@ func (r *URIResolver) URIFromDestination(dest duckv1beta1.Destination, parent in return "", errors.New("destination missing Ref, [apiVersion, kind, name] and URI, expected at least one") } -// URIFromDestinationV1 resolves a v1.Destination into a URI string. -func (r *URIResolver) URIFromDestinationV1(dest duckv1.Destination, parent interface{}) (string, error) { +// URIFromDestinationV1 resolves a v1.Destination into a URL. +func (r *URIResolver) URIFromDestinationV1(dest duckv1.Destination, parent interface{}) (*apis.URL, error) { if dest.Ref != nil { url, err := r.URIFromObjectReference(dest.Ref, parent) if err != nil { - return "", err + return nil, err } if dest.URI != nil { if dest.URI.URL().IsAbs() { - return "", errors.New("absolute URI is not allowed when Ref or [apiVersion, kind, name] exists") + return nil, errors.New("absolute URI is not allowed when Ref or [apiVersion, kind, name] exists") } - return url.URL().ResolveReference(dest.URI.URL()).String(), nil + return url.ResolveReference(dest.URI), nil } - return url.URL().String(), nil + return url, nil } if dest.URI != nil { // IsAbs check whether the URL has a non-empty scheme. Besides the non non-empty scheme, we also require dest.URI has a non-empty host if !dest.URI.URL().IsAbs() || dest.URI.Host == "" { - return "", fmt.Errorf("URI is not absolute(both scheme and host should be non-empty): %v", dest.URI.String()) + return nil, fmt.Errorf("URI is not absolute(both scheme and host should be non-empty): %q", dest.URI.String()) } - return dest.URI.String(), nil + return dest.URI, nil } - return "", errors.New("destination missing Ref and URI, expected at least one") + return nil, errors.New("destination missing Ref and URI, expected at least one") } // URIFromObjectReference resolves an ObjectReference to a URI string. diff --git a/resolver/addressable_resolver_test.go b/resolver/addressable_resolver_test.go index 4384b7aac..215e38c8f 100644 --- a/resolver/addressable_resolver_test.go +++ b/resolver/addressable_resolver_test.go @@ -82,14 +82,14 @@ func TestGetURI_DestinationV1Beta1(t *testing.T) { Host: "example.com", }, }, - wantErr: fmt.Sprintf("URI is not absolute(both scheme and host should be non-empty): %v", "//example.com"), + wantErr: fmt.Sprintf("URI is not absolute(both scheme and host should be non-empty): %q", "//example.com"), }, "URI with no host": { dest: duckv1beta1.Destination{ URI: &apis.URL{ Scheme: "http", }, }, - wantErr: fmt.Sprintf("URI is not absolute(both scheme and host should be non-empty): %v", "http:"), + wantErr: fmt.Sprintf("URI is not absolute(both scheme and host should be non-empty): %q", "http:"), }, "Ref and [apiVersion, kind, name] both exists": { objects: []runtime.Object{ @@ -382,14 +382,14 @@ func TestGetURI_DestinationV1(t *testing.T) { Host: "example.com", }, }, - wantErr: fmt.Sprintf("URI is not absolute(both scheme and host should be non-empty): %v", "//example.com"), + wantErr: fmt.Sprintf("URI is not absolute(both scheme and host should be non-empty): %q", "//example.com"), }, "URI with no host": { dest: duckv1.Destination{ URI: &apis.URL{ Scheme: "http", }, }, - wantErr: fmt.Sprintf("URI is not absolute(both scheme and host should be non-empty): %v", "http:"), + wantErr: fmt.Sprintf("URI is not absolute(both scheme and host should be non-empty): %q", "http:"), }, "happy ref": { objects: []runtime.Object{ @@ -533,7 +533,7 @@ func TestGetURI_DestinationV1(t *testing.T) { t.Errorf("unexpected error: %v", gotErr.Error()) } } - if diff := cmp.Diff(tc.wantURI, uri); diff != "" { + if diff := cmp.Diff(tc.wantURI, uri.String()); diff != "" { t.Errorf("unexpected object (-want, +got) = %v", diff) } })