Go: cmd/go: go mod download -json does not output json when sumdb fails

[marwan-at-work]

Summary

go mod download -json still returns a valid json when the command fails.

For example, if you run go mod download -json github.com/alsdkfjjsdkfjk/[email protected] then Stdout receives a valid json with the error message saying "module not found".

However, say you have a private repo you'd like to go mod download -json, but you haven't configured GOPRIVATE/GONOSUMDB correctly, then the command fails as expected, but the output is not json.

Digging a little more, I see that most sumdb-checking functions just call base.Fatalf and not actually return an error.

Here's what I had to do to fix it: https://github.com/marwan-at-work/go/commit/f143bcec673f8e63055707a63d79f62573afd0f7

I'd be happy to submit the commit above as a CL if it looks okay as a fix

What version of Go are you using (go version)?

1.13

Does this issue reproduce with the latest release?

Yes

What did you do?

GONOSUMDB='' go mod download -json <private-repo>@<version>

What did you expect to see?

A valid json with the Error field describing that the sumdb check has failed.

What did you see instead?

A non-valid-json with the expected error message

[bcmills]

That seems reasonable to me, but I'd like to confirm with @FiloSottile or @rsc .

(CC @jayconrod)

[rsc]

There are always going to be ways the go command fails and prints to standard error without producing JSON on standard output. Why not treat this failure like any other such failure? Why does it have to be encoded in JSON on standard output?

[rsc]

Another option is maybe base.Fatalf should be printing the JSON during -json mode. That would be a much smaller patch.

[marwan-at-work]

There are always going to be ways the go command fails and prints to standard error without producing JSON on standard output

Does that mean that go proxies should always try to parse json from stdout (on failure and success) and if the parsing fails they need to read stderr as the alternate failure message?

Why not treat this failure like any other such failure? Why does it have to be encoded in JSON on standard output?

That's kind of what I expected. "Any other such failure" does produce valid json, therefore I expected the sumdb failure to also behave similarly.

For example the following command fails, but produces valid json:

go mod download -json github.com/asdfasdf/[email protected]
go: finding github.com/asdfasdf/asdfasdfasdf v1.1.0
go: finding github.com/asdfasdf/asdfasdfasdf v1.1.0
{
    "Path": "github.com/asdfasdf/asdfasdfasdf",
    "Version": "v1.1.0",
    "Error": "github.com/asdfasdf/[email protected]: invalid version: unknown revision v1.1.0"
}

While the same exact command fails (for a different reason), but does not produce a valid json:

go mod download -json github.com/marwan-at-work/umlaut@latest
go: finding github.com/marwan-at-work/umlaut latest
verifying github.com/marwan-at-work/[email protected]/go.mod: github.com/marwan-at-work/[email protected]/go.mod: reading https://sum.golang.org/lookup/github.com/marwan-at-work/[email protected]: 410 Gone

umlaut is a test private repo in my github account.

Another option is maybe base.Fatalf should be printing the JSON during -json mode. That would be a much smaller patch.

The issue is that some json data will be lost or may not even be the same shape, for example: go mod download -json returns a json of this shape https://github.com/golang/go/blob/master/src/cmd/go/internal/modcmd/download.go#L59:L70

While go list can return either of these:
https://github.com/golang/go/blob/master///src/cmd/go/internal/load/pkg.go#L51:6
https://github.com/golang/go/blob/master///src/cmd/go/internal/modinfo/info.go#L12:6

So I imagine base.Fatalf will probably have its own json shape that describes the error and potential extra information.

That sounds good but the issue is that at this moment, running go mod download -json and running go list -m may "fail" through a base.Fatalf but may also fail through a propagated return err. Which means go proxies will have to continue guessing as to what the json shape will be.

If the patch is to change all errors to use base.Fatalf shape, then that will be consistent, but I imagine that's an equally large change to the codebase.

But maybe you had something else in mind for how the go proxies should deal with go mod download -json and go list -m failures?

Thanks for the quick response!

[bcmills]

I agree that proxies should be robust to missing JSON output.

On the other hand, I suspect that we will need to convert many calls to base.Fatalf to return structured errors anyway, both so that they can be diagnosed more easily (the point of failure doesn't always have enough contextual information, such as the import or requirement path by which the failed module was reached), and so that explicitly error-tolerant tools such as gopls can degrade gracefully when working on incomplete or inconsistent code.

So I'm not sure that go mod download -json in particular is a good motivating example, but we may need to fix it incidentally anyway.

[heschik]

cc @hyangah, @katiehockman

[gopherbot]

Change https://golang.org/cl/197062 mentions this issue: cmd/go: consistent output for -json failures