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

vanity import paths backed by GitHub repos are incorrectly redirected #579

Closed
myitcv opened this Issue Sep 6, 2018 · 13 comments

Comments

Projects
None yet
4 participants
@myitcv
Member

myitcv commented Sep 6, 2018

The link https://godoc.org/myitcv.io/highlightjs incorrectly redirects to https://godoc.org/github.com/myitcv/x/highlightjs.

But I can't tell why this is, because the <meta> information appears to be correct:

$ curl -s -i https://myitcv.io/highlightjs?go-get=1  | grep meta
        <meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>
        <meta name="go-import" content="myitcv.io git https://github.com/myitcv/x">
        <meta name="go-source" content="myitcv.io https://github.com/myitcv/x/wiki https://github.com/myitcv/x/tree/master{/dir} https://github.com/myitcv/x/blob/master{/dir}/{file}#L{line}">
        <meta http-equiv="refresh" content="0; url=https://godoc.org/myitcv.io/highlightjs">

The code beneath myitcv.io/... no longer uses import path checking, per golang/go#26367 (comment).

Perhaps https://godoc.org/ doesn't understand modules in such detail yet?

Any help much appreciated.

Thanks

@tav

This comment has been minimized.

Show comment
Hide comment
@tav

tav commented Sep 8, 2018

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 8, 2018

Member

Perhaps https://godoc.org/ doesn't understand modules in such detail yet?

gddo does not have any support for modules yet, the tracking issue for that is #567.

However, the redirect from godoc.org/myitcv.io/highlightjs to godoc.org/github.com/myitcv/x/highlightjs seems unexpected and likely a bug that we can fix. I suspect it might be a regression caused by 9d8ff1c.

I was about to say that it's strange that other vanity import paths aren't affected, but I realized they are as soon as you refresh that package. E.g., https://godoc.org/go4.org/sort did not redirect at first, but it does now after I refreshed it.

Sorry about not catching this sooner and thanks for reporting!

Member

dmitshur commented Sep 8, 2018

Perhaps https://godoc.org/ doesn't understand modules in such detail yet?

gddo does not have any support for modules yet, the tracking issue for that is #567.

However, the redirect from godoc.org/myitcv.io/highlightjs to godoc.org/github.com/myitcv/x/highlightjs seems unexpected and likely a bug that we can fix. I suspect it might be a regression caused by 9d8ff1c.

I was about to say that it's strange that other vanity import paths aren't affected, but I realized they are as soon as you refresh that package. E.g., https://godoc.org/go4.org/sort did not redirect at first, but it does now after I refreshed it.

Sorry about not catching this sooner and thanks for reporting!

@dmitshur dmitshur added the bug label Sep 8, 2018

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 8, 2018

Member

gddo/doc/builder.go

Lines 589 to 597 in 656e7e1

case bpkg.ImportComment == "" && dir.ResolvedGitHubPath != "":
// Make sure to redirect to GitHub's reported canonical casing when
// there's no import path comment.
if dir.ImportPath != dir.ResolvedGitHubPath {
return nil, gosrc.NotFoundError{
Message: "not at canonical import path",
Redirect: dir.ResolvedGitHubPath,
}
}

That case is too aggressive and is incorrectly redirecting vanity import paths. We should update it to only redirect if the import path starts with "github.com/" (case insensitively), I think that would resolve the issue.

Member

dmitshur commented Sep 8, 2018

gddo/doc/builder.go

Lines 589 to 597 in 656e7e1

case bpkg.ImportComment == "" && dir.ResolvedGitHubPath != "":
// Make sure to redirect to GitHub's reported canonical casing when
// there's no import path comment.
if dir.ImportPath != dir.ResolvedGitHubPath {
return nil, gosrc.NotFoundError{
Message: "not at canonical import path",
Redirect: dir.ResolvedGitHubPath,
}
}

That case is too aggressive and is incorrectly redirecting vanity import paths. We should update it to only redirect if the import path starts with "github.com/" (case insensitively), I think that would resolve the issue.

@dmitshur dmitshur changed the title from Incorrect redirect being applied? to vanity import paths backed by GitHub repos are incorrectly redirected Sep 8, 2018

@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv
Member

myitcv commented Sep 9, 2018

@Carpetsmoker

This comment has been minimized.

Show comment
Hide comment
@Carpetsmoker

Carpetsmoker Sep 9, 2018

Contributor

We should update it to only redirect if the import path starts with "github.com/" (case insensitively), I think that would resolve the issue.

Currently, that information isn't available in doc.newPackage(), as gosrc.getDynamic() reset the ImportPath here:

dir.ImportPath = importPath

So we'd either need to make that logic smarter, or add a new field to record the path the user requested.

Contributor

Carpetsmoker commented Sep 9, 2018

We should update it to only redirect if the import path starts with "github.com/" (case insensitively), I think that would resolve the issue.

Currently, that information isn't available in doc.newPackage(), as gosrc.getDynamic() reset the ImportPath here:

dir.ImportPath = importPath

So we'd either need to make that logic smarter, or add a new field to record the path the user requested.

@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv Sep 9, 2018

Member

Does it make sense to revert the commit that broke this in the first place, for now?

Member

myitcv commented Sep 9, 2018

Does it make sense to revert the commit that broke this in the first place, for now?

@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv Sep 9, 2018

Member

Also looks like we're going to need to do something to clear up the index too

screen shot 2018-09-09 at 15 28 39

Member

myitcv commented Sep 9, 2018

Also looks like we're going to need to do something to clear up the index too

screen shot 2018-09-09 at 15 28 39

@tav

This comment has been minimized.

Show comment
Hide comment
@tav

tav Sep 9, 2018

While this issue is getting fixed, would it also make sense to add basic support for Go modules, so that if anyone goes to the GitHub version of a repo which is using vanity import paths, e.g. https://godoc.org/github.com/myitcv/x/highlightjs, it would try to read a go.mod file at the repo root, and redirect to that subpath on godoc.org instead, i.e. https://godoc.org/myitcv.io/highlightjs ?

This should prevent people from seeing the non-canonical docs and import paths when projects have stopped using canonical import comments...

tav commented Sep 9, 2018

While this issue is getting fixed, would it also make sense to add basic support for Go modules, so that if anyone goes to the GitHub version of a repo which is using vanity import paths, e.g. https://godoc.org/github.com/myitcv/x/highlightjs, it would try to read a go.mod file at the repo root, and redirect to that subpath on godoc.org instead, i.e. https://godoc.org/myitcv.io/highlightjs ?

This should prevent people from seeing the non-canonical docs and import paths when projects have stopped using canonical import comments...

@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv Sep 10, 2018

Member

@tav - I think if this issue is fixed that will be fine for now. More important that resolving any sort of reverse mapping from VCS to module/custom import path would be to get full support for semantic import paths.

@ianthehat - is godoc.org something that could/should fall under the umbrella of golang/go#24661?

Member

myitcv commented Sep 10, 2018

@tav - I think if this issue is fixed that will be fine for now. More important that resolving any sort of reverse mapping from VCS to module/custom import path would be to get full support for semantic import paths.

@ianthehat - is godoc.org something that could/should fall under the umbrella of golang/go#24661?

@dmitshur dmitshur self-assigned this Sep 10, 2018

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 10, 2018

Member

Does it make sense to revert the commit that broke this in the first place, for now?

In this (rare) case, I've opted not to, because the problem and the path to resolution are well understood. The bottleneck to resolving this issue is doing the deploy.

The fix is simple: before considering to redirect to canonical GitHub case, check that the requested import path is a case-mismatch and not something else altogether. With that change in, these bad redirects shouldn't happen anymore.

I've sent CL 134355 that should address this issue, and added tests for newPackage redirection behavior, because we've gone far too long without them.

@tav I agree that adding some basic awareness of mod.go files would be helpful, but let's discuss that further in #567. It'd be helpful if you copied your comment there. This issue is for the specific vanity -> GitHub redirection bug.

Member

dmitshur commented Sep 10, 2018

Does it make sense to revert the commit that broke this in the first place, for now?

In this (rare) case, I've opted not to, because the problem and the path to resolution are well understood. The bottleneck to resolving this issue is doing the deploy.

The fix is simple: before considering to redirect to canonical GitHub case, check that the requested import path is a case-mismatch and not something else altogether. With that change in, these bad redirects shouldn't happen anymore.

I've sent CL 134355 that should address this issue, and added tests for newPackage redirection behavior, because we've gone far too long without them.

@tav I agree that adding some basic awareness of mod.go files would be helpful, but let's discuss that further in #567. It'd be helpful if you copied your comment there. This issue is for the specific vanity -> GitHub redirection bug.

@dmitshur

This comment has been minimized.

Show comment
Hide comment

@gopherbot gopherbot closed this in ee3b7b1 Sep 11, 2018

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 11, 2018

Member

The fix is merged and deployed, resolving this issue. Thanks again for the report @myitcv and @tav! Please let us know if something else comes up.

Member

dmitshur commented Sep 11, 2018

The fix is merged and deployed, resolving this issue. Thanks again for the report @myitcv and @tav! Please let us know if something else comes up.

@tav

This comment has been minimized.

Show comment
Hide comment
@tav

tav Sep 11, 2018

Thanks for the prompt fix @dmitshur — everything is working great! ❤️

tav commented Sep 11, 2018

Thanks for the prompt fix @dmitshur — everything is working great! ❤️

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