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

Extra junk in url's produced by UriMatch.match #16

Open
Andersmholmgren opened this issue Jul 6, 2014 · 5 comments
Open

Extra junk in url's produced by UriMatch.match #16

Andersmholmgren opened this issue Jul 6, 2014 · 5 comments

Comments

@Andersmholmgren
Copy link
Contributor

  final p = new UriParser(new UriTemplate('/ui'));
  final match = p.match(Uri.parse('http://localhost:8080/ui/foo'));
  print(match.rest);

Expected

/foo

Actual

///foo?#

Cause
UriMatch builds (via UriBuilder) a uri with '' rather than null in fragment and host, plus an empty map for queryParameters.
Uri.toString() assume all those are null or have something interesting in them.

Note: I would consider it a bug in Uri to required null queryParameters as empty collections are normally considered better practice than null

@justinfagnani
Copy link
Contributor

Good report. I think you might have a point about the behavior of Uri, but that might be a backwards breaking change. We need to fix this here.

I see a few options:

  • UriBuilder could convert empty parts to null, though this means a difference in behavior between it and Uri. build() could take some parameters to control this behavior.
  • UriBuilder could not initialize parts. This might hurt usability, since it's nice not to have to check nor null.
  • UriParser.match could convert empty parts to null, and we could add to the UriPattern documentation that this is recommended practice for implementors.

Opinions and patches welcome here.

@justinfagnani
Copy link
Contributor

I think this is actually a breaking change from Uri in 1.6. The tests are failing on the dev channel, and setting the parts to null fixes it.

@justinfagnani
Copy link
Contributor

ok, I have a fix, which is the first option. The output from the above code is now foo instead of ///foo?#

@Andersmholmgren
Copy link
Contributor Author

Cool thanks. I'll back out the workaround in shelf_route when it's ready.

On 17 Jul 2014, at 12:44 pm, Justin Fagnani notifications@github.com
wrote:

ok, I have a fix, which is the first option. The output from the above code
is not foo instead of ///foo?#


Reply to this email directly or view it on GitHub
#16 (comment).

@Andersmholmgren
Copy link
Contributor Author

did this ever get released?

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