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

Regression in set_path for 2.1.1 vs 2.1.0 #579

Closed
weiznich opened this issue Jan 14, 2020 · 6 comments · Fixed by #603
Closed

Regression in set_path for 2.1.1 vs 2.1.0 #579

weiznich opened this issue Jan 14, 2020 · 6 comments · Fixed by #603

Comments

@weiznich
Copy link

weiznich commented Jan 14, 2020

Diesels test suits fail since the release of url 2.1.1. I've narrowed down the failure to the following minimal test case:

let pg_url = "postgres://postgres@localhost/";
let mut db_url = url::Url::parse(pg_url).unwrap();
db_url.set_path("diesel_foo");
println!("{}", db_url);

With url 2.1.0 this results in postgres://postgres@localhost/diesel_foo being printed, while 2.1.1 outputs postgres://postgres@localhostdiesel_foo.

(Sounds similar to #577, but I cannot see how this is related.)

weiznich added a commit to weiznich/diesel that referenced this issue Jan 14, 2020
Caused by the release of url 2.1.1
See servo/rust-url#579 for details
@tmccombs
Copy link
Contributor

It's probably somewhat related. I think in order to pass new tests for changes to the WHATWG standard, the behaviour of the setters for "non-special" protocol/schemes (basically those that aren't used by browsers) changed.

weiznich added a commit to weiznich/diesel that referenced this issue Jan 14, 2020
* Caused by the release of url 2.1.1
  See servo/rust-url#579 for details

* A mysql release seems to have broken the macos setup, so use a
absulute path there
weiznich added a commit to weiznich/diesel that referenced this issue Jan 14, 2020
* Caused by the release of url 2.1.1
  See servo/rust-url#579 for details

* A mysql release seems to have broken the macos setup, so use a
absulute path there
weiznich added a commit to weiznich/diesel that referenced this issue Jan 14, 2020
* Caused by the release of url 2.1.1
  See servo/rust-url#579 for details

* A mysql release seems to have broken the macos setup, so use a
absulute path there
@tmccombs
Copy link
Contributor

Maybe this is a bug in WHATWG? I think this line of the parse algorithm is the cause (in path start state):

If c is not U+002F (/), then decrease pointer by one.

This seems odd to me, especially in the context of setting the pathname, since in that case pointer is already pointing at the beginning of input.

@tmccombs
Copy link
Contributor

btw, I think that set_path("/diesel_foo") would work for you

@alercah
Copy link

alercah commented Apr 24, 2020

I agree with you that the spec is a bit buggy there, but it shouldn't make a difference in this testcase . The only difference it would make here if the base URL had a special scheme would be that a leading \ would be a validation error, as far as I can tell. It would still attempt to move c back by one beyond the leading edge of the string at step 1.3 of path start state.

But it doesn't really matter because the pointer is to the input "diesel_foo" here, not the base URL or the URL being worked on. Moving backwards off the front is just ill-defined, and best interpreted as not moving at all. If the Rust code is somehow treating this like a string operation, and acting on the separator between the host and path, that's a clear misinterpretation of the algorithm.

This seems like the closest applicable WPT (it does not trigger the special case for # because https is special) and it confirms this.

@alercah
Copy link

alercah commented Apr 25, 2020

Filed whatwg/url#480 about the spec bug. It seems the intent is to step back by one so that when the state machine loops, it advances back to the start of the path (since looping is defined to increment the pointer).

@alercah
Copy link

alercah commented May 4, 2020

If whatwg/url#488 is merged, it will clarify the intent as per the above, making clear that this is a bug in the library.

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

Successfully merging a pull request may close this issue.

3 participants