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

Query parameters are not removed when resolving URLs #113

Closed
fidian opened this Issue Sep 3, 2013 · 5 comments

Comments

Projects
None yet
3 participants
@fidian
Contributor

fidian commented Sep 3, 2013

When navigating to "test.html" from "http://example.com/somewhere.html?a=b#c", I expect the URL to be "http://example.com/test.html", not "http://example.com/test.html?a=b". Here's simple node REPL code to replicate it.

var URI = require('./URI');
URI('asdf', 'http://example.com/somewhere.html?a=b#c').toString();
@rodneyrehm

This comment has been minimized.

Show comment
Hide comment
@rodneyrehm

rodneyrehm Sep 3, 2013

Member

Looking at examples in the RFC most of the following tests fail:

test("absoluteTo", function() {
    // http://tools.ietf.org/html/rfc3986#section-5.4
    var uri;
    var base = 'http://a/b/c/d;p?q';
    var map = {
        // normal
        // "g:h"           :  "g:h", // identified as URN
        "g"             :  "http://a/b/c/g",
        "./g"           :  "http://a/b/c/g",
        "g/"            :  "http://a/b/c/g/",
        "/g"            :  "http://a/g",
        "//g"           :  "http://g",
        "?y"            :  "http://a/b/c/d;p?y",
        "g?y"           :  "http://a/b/c/g?y",
        "#s"            :  "http://a/b/c/d;p?q#s",
        "g#s"           :  "http://a/b/c/g#s",
        "g?y#s"         :  "http://a/b/c/g?y#s",
        ";x"            :  "http://a/b/c/;x",
        "g;x"           :  "http://a/b/c/g;x",
        "g;x?y#s"       :  "http://a/b/c/g;x?y#s",
        ""              :  "http://a/b/c/d;p?q",
        "."             :  "http://a/b/c/",
        "./"            :  "http://a/b/c/",
        ".."            :  "http://a/b/",
        "../"           :  "http://a/b/",
        "../g"          :  "http://a/b/g",
        "../.."         :  "http://a/",
        "../../"        :  "http://a/",
        "../../g"       :  "http://a/g",
        // abnormal
        "../../../g"    :  "http://a/g",
        "../../../../g" :  "http://a/g"
    };

    for (var key in map) {
        uri = URI(key).absoluteTo(base);
        equal(r + "", map[key], 'reference resolution ' + key);
    }
});

@djcsdy have you got anything to say about query+hash in absoluteTo()? otherwise I'd just remove them…

Member

rodneyrehm commented Sep 3, 2013

Looking at examples in the RFC most of the following tests fail:

test("absoluteTo", function() {
    // http://tools.ietf.org/html/rfc3986#section-5.4
    var uri;
    var base = 'http://a/b/c/d;p?q';
    var map = {
        // normal
        // "g:h"           :  "g:h", // identified as URN
        "g"             :  "http://a/b/c/g",
        "./g"           :  "http://a/b/c/g",
        "g/"            :  "http://a/b/c/g/",
        "/g"            :  "http://a/g",
        "//g"           :  "http://g",
        "?y"            :  "http://a/b/c/d;p?y",
        "g?y"           :  "http://a/b/c/g?y",
        "#s"            :  "http://a/b/c/d;p?q#s",
        "g#s"           :  "http://a/b/c/g#s",
        "g?y#s"         :  "http://a/b/c/g?y#s",
        ";x"            :  "http://a/b/c/;x",
        "g;x"           :  "http://a/b/c/g;x",
        "g;x?y#s"       :  "http://a/b/c/g;x?y#s",
        ""              :  "http://a/b/c/d;p?q",
        "."             :  "http://a/b/c/",
        "./"            :  "http://a/b/c/",
        ".."            :  "http://a/b/",
        "../"           :  "http://a/b/",
        "../g"          :  "http://a/b/g",
        "../.."         :  "http://a/",
        "../../"        :  "http://a/",
        "../../g"       :  "http://a/g",
        // abnormal
        "../../../g"    :  "http://a/g",
        "../../../../g" :  "http://a/g"
    };

    for (var key in map) {
        uri = URI(key).absoluteTo(base);
        equal(r + "", map[key], 'reference resolution ' + key);
    }
});

@djcsdy have you got anything to say about query+hash in absoluteTo()? otherwise I'd just remove them…

@fidian

This comment has been minimized.

Show comment
Hide comment
@fidian

fidian Sep 3, 2013

Contributor

I believe the query should stay the same if we are just resolving against a hash. See the "#s" example in your code.

Contributor

fidian commented Sep 3, 2013

I believe the query should stay the same if we are just resolving against a hash. See the "#s" example in your code.

@djcsdy

This comment has been minimized.

Show comment
Hide comment
@djcsdy

djcsdy Sep 4, 2013

Contributor

@rodneyrehm Those tests derived from the spec look pretty complete.

The query component of a base URL should be retained if and only if:

  • The relative URL to be resolved is the empty string (""); OR
  • The relative URL to be resolved contains a hash component but no other components ("#foo").

Otherwise it should be discarded.

Contributor

djcsdy commented Sep 4, 2013

@rodneyrehm Those tests derived from the spec look pretty complete.

The query component of a base URL should be retained if and only if:

  • The relative URL to be resolved is the empty string (""); OR
  • The relative URL to be resolved contains a hash component but no other components ("#foo").

Otherwise it should be discarded.

@rodneyrehm

This comment has been minimized.

Show comment
Hide comment
@rodneyrehm

rodneyrehm Sep 4, 2013

Member

I agree. I'll look into fixing absoluteTo() this week. It will break relativeTo() tests - they'll have to be adjusted as well.

Member

rodneyrehm commented Sep 4, 2013

I agree. I'll look into fixing absoluteTo() this week. It will break relativeTo() tests - they'll have to be adjusted as well.

@rodneyrehm

This comment has been minimized.

Show comment
Hide comment
@rodneyrehm

rodneyrehm Sep 4, 2013

Member

fixed in master, will be included in the next release

Member

rodneyrehm commented Sep 4, 2013

fixed in master, will be included in the next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment