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.

Sign up for GitHub

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

Jump to bottom

Uri.resolveUri does not work correctly for relative URIs. #27447

Closed
scheglov opened this issue Sep 28, 2016 · 12 comments
Closed

Uri.resolveUri does not work correctly for relative URIs. #27447

scheglov opened this issue Sep 28, 2016 · 12 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@scheglov
Copy link
Contributor 

main() {
  {
    Uri base = Uri.parse('a.dart');
    Uri contained = Uri.parse('../b.dart');
    var resolved = base.resolveUri(contained);
    print(resolved);
  }
  {
    Uri base = Uri.parse('a/b.dart');
    Uri contained = Uri.parse('../../c.dart');
    var resolved = base.resolveUri(contained);
    print(resolved);
  }
}

I expected that this code would print ../b.dart and ../c.dart.
But it prints b.dart and c.dart.

Moreover, if I step into _SimpleUri.resolveUri and invoke _toNonSimple().resolveUri(reference), I get the expected results. So, I guess that the _SimpleUri's implementation is broken.

@stereotype441 @lrhn
@scheglov scheglov added P1 A high priority bug; for example, a single project is unusable or has many test failures area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Sep 28, 2016
scheglov added a commit that referenced this issue Sep 28, 2016
@scheglov
Add tests for resolveRelativeUri() and a workaround.
3f6afd8 
The actual problem should be fixed in SDKs Uri implementation.
#27447

R=paulberry@google.com
BUG=

Review URL: https://codereview.chromium.org/2376073003 .
@lrhn
Copy link
Member

lrhn commented Sep 29, 2016

We have special casing for resolving a relative URI reference against another relative URI reference (the URI spec assumes always resolving against a non-relative URI, and it's behavior isn't really useful, but technically we are not following the specification).
It seems the simple-URI implementation fails the special casing here for some reason.

lrhn added a commit that referenced this issue Sep 29, 2016
@lrhn
Fix bug in _SimpleUri.resolve.
9bd4406 
In some cases it didn't follow our non-RFC behavior for resolving a relative
path on top of another relative path.

Fixes issue #27447
BUG= http://dartbug.com/27447
R=eernst@google.com

Review URL: https://codereview.chromium.org/2374253004 .
@mit-mit
Copy link
Member

mit-mit commented Sep 29, 2016

@scheglov is this really P1?

@scheglov
Copy link
Contributor Author

This causes invalid analysis (and possibly DDC compilation) when using summaries.
This seems pretty serious to me.
But I'm not an expert in priorities.

@mit-mit
Copy link
Member

mit-mit commented Sep 29, 2016

In what real-world context are we using a relative base?

@stereotype441
Copy link
Member

Analyzer summary logic uses a relative base so that it can create portable summaries. For example, assuming user Joe has the following file structure:

/home/joe/proj/foo/bar/baz.dart
  import '../qux.dart';
  class C extends B {}
/home/joe/proj/foo/qux.dart
  export '../thud.dart';
/home/joe/proj/thud.dart
  class B {}

In the summary for /home/joe/proj/foo/bar/baz.dart we want to record that class C extends class B, and that class B came originally from the file /home/joe/proj/thud.dart. But we don't want to encode the absolute path /home/joe/proj/thud.dart in the summary file, because that would make the summary unusable if Joe decided to share this code with some other user. So instead we use the relative path from /home/joe/proj/foo/bar/baz.dart to /home/joe/proj/thud.dart, which is ../../thud.dart. The easiest way to compute this path is to use Uri.resolveUri to resolve ../thud.dart relative to ../qux.dart.

@lrhn
Copy link
Member

lrhn commented Sep 29, 2016

It should be fixed on by 9bd4406 now.

@lrhn lrhn closed this as completed Sep 29, 2016
@lrhn
Copy link
Member

lrhn commented Sep 29, 2016 •
edited

Just for context:
Resolving a URI reference against another URI reference (and not a URI) is not specified by RFC 3986, but since we use the Uri class to represent both, we have to handle it.
We allow it and handle it reasonably because it's useful, e.g., for cases like this, so it should work consistently - but the only exception from the RFC 9386 algorithm is in the case where the base URI has neither scheme nor authority and a non-empty relative path.

@mit-mit
Copy link
Member

mit-mit commented Sep 29, 2016

The last full push to 1.20 happened earlier today; do we need to merge this?

CC @whesse

@stereotype441
Copy link
Member

Yes, that would be very helpful, since analyzer's workaround for this issue is incomplete, and we can't remove it safely until a version of the Dart SDK is published that contains the bug fix. So if we don't get this fix merged into 1.20, we'll have to wait until 1.21 is released before we can clean up analyzer.

@scheglov
Copy link
Contributor Author

For reference, this CL would remove the incomplete analyze workaround.

@whesse
Copy link
Contributor

whesse commented Sep 29, 2016

The fix for this was the last thing that made it into -dev.10.0, so no, we don't need to merge this.

@mit-mit
Copy link
Member

mit-mit commented Sep 30, 2016

Great, thanks for checking!

On Thu, Sep 29, 2016 at 4:28 PM, William Hesse notifications@github.com
wrote:

The fix for this was the last thing that made it into -dev.10.0, so no, we
don't need to merge this.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#27447 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ANAxitIUpO60GHdcCZ9wQGSR0XI1pvwEks5qvEmAgaJpZM4KJQYC
.

stereotype441 added a commit that referenced this issue Jan 30, 2017
@stereotype441
Remove analyzer workaround for #27447.
b3237d0 
This issue was fixed in the SDK sufficiently long ago that it doesn't
seem necessary to keep the workaround in analyzer.

R=scheglov@google.com

Review-Url: https://codereview.chromium.org/2662033002 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@stereotype441 @scheglov @lrhn @whesse @mit-mit