golang / gddo

Go Doc Dot Org
https://godoc.org
BSD 3-Clause "New" or "Revised" License
1.1k stars 265 forks source link

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

Closed myitcv closed 6 years ago

myitcv commented 6 years ago

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 https://github.com/golang/go/issues/26367#issuecomment-405666468.

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

Any help much appreciated.

Thanks

tav commented 6 years ago

I'm having the same issue, with https://godoc.org/peerbase.net/go/process getting redirected to https://godoc.org/github.com/peerbase/peerbase/process

dmitshur commented 6 years ago

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 9d8ff1c67be5c2da6321c656913e1fe355991138.

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 commented 6 years ago

https://github.com/golang/gddo/blob/656e7e11a75c6bf4ba73628e7384a0d4c08cc537/doc/builder.go#L589-L597

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.

myitcv commented 6 years ago

cc @Carpetsmoker @shantuo

arp242 commented 6 years ago

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:

https://github.com/golang/gddo/blob/656e7e11a75c6bf4ba73628e7384a0d4c08cc537/gosrc/gosrc.go#L411

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

myitcv commented 6 years ago

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

myitcv commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 https://github.com/golang/go/issues/24661?

dmitshur commented 6 years ago

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 commented 6 years ago

That CL can be tested (temporarily) at https://20180910t145226-dot-godoc-org.appspot.com/.

E.g., these work as expected:

dmitshur commented 6 years ago

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 commented 6 years ago

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