sylabs / json-resp Goto Github PK

Expose this code as a Go Module.

Switch linter fom golang.org/x/lint to github.com/golangci/golangci-lint.

I think it might be wise to remove JSONErrorUnauthorized. It's not obvious (to me anyways) how one would use it directly with any of the rest of the json-resp API.

The jsonresp.Error type is intended to allow a status code and optional message to be included in a JSON-encoded response. The idea is that the entity sending the JSON-encoded response calls WriteError() with the status code and message. The recipient, upon receiving a non-successful HTTP status code, can use ReadError() to retrieve a jsonresp.Error that includes the status code and message.

JSONErrorUnauthorized isn't really useful to pass to WriteError(), since it takes an error string and code int rather than a jsonresp.Error. You can't compare it to any error you receive from ReadError() easily. #30 would open this up as a possibility, but in practice I'm not sure errors.Is(err, JSONErrorUnauthorized) is as useful as doing errors.Is(err, &Error{Code: http.StatusUnauthorized}). The former will only match errors where the Message is exactly "Unauthorized", whereas the latter would match any unauthorized response.

@EmmEff, would love your input here. I know there's some use of this in the Sylabs code base, and I guess I'm curious whether JSONErrorUnauthorized can/should be moved somewhere internal where it's used?

Update CI to verify this module is (and continues to be) compatible with Go v1.13. The cache_go_mod step should be unnecessary, with the default module proxy serving a similar purpose.

We should implement the Is() method introduced in Go 1.13 for the jsonresp.Error type in order to make it more usable. I propose the implementation takes the approach similar to the example in the Working with Errors in Go 1.13 blog post, comparing non-zero fields.

This would allow succinct code such as the following:

if errors.Is(err, &jsonresp.Error{Code: http.StatusUnauthorized}) {
	// Do some stuff...
}

Dependabot has been acquired by GitHub, and as a result has made some changes that will require a bit of work to migrate. Details available here.

https://github.com/sylabs/scs-key-client has been migrated, and it went smoothly other than a minor issue with the badge (sylabs/scs-key-client#37). I'd suggest we hold off migrating this repo until that niggle is sorted. Progress on the badge niggle will likely be reported in dependabot/dependabot-core#1912.

This package gives the ability to provide detailed error messages, but in practice this is somewhat awkward to use with the current API. Consider the case where a caller gets a non-OK status code back, but is unsure whether a detailed error is present. In this case, the caller must differentiate a failure of jsonresp.ReadResponse to unmarshal versus a detailed error itself. In the case of a failure to unmarshal, it is often desirable to report other more relevant information such as the HTTP status code. This can currently be accomplished with logic such as:

	if res.StatusCode != http.StatusOK {
		if err := jsonresp.ReadResponse(res.Body, nil); err != nil {
			if err, ok := err.(*jsonresp.Error); ok {
				return err
			}
		}
		return jsonresp.NewError(res.StatusCode, "")
	}

This is not very straight forward. To help with clarity, I propose introducing a func ReadError(r io.Reader) error function, that will return the error detail when one is possible in r, and nil otherwise. Then, the logic is more readable:

	if res.StatusCode != http.StatusOK {
		if err := jsonresp.ReadError(res.Body); err != nil {
			return err
		}
		return jsonresp.NewError(res.StatusCode, "")
	}

Run go fmt and go vet via golangci-lint.

There are some recommendations about how best to pass around source code and dependencies on the CircleCI blog. Use checkout to retrieve source, and restore_cache to fetch cached Go modules.

Report code coverage via codecov.

As @mikegray points out, json-resp has no external dependencies, so it is not necessary to use dep for this package.

Remove dep, and all references to it in the README, CircleCI config, and .gitignore file.

https://godoc.org/ will eventually be replaced by https://pkg.go.dev/. We should update our documentation badge to the new location when appropriate.

Only minor blocker I can see at the moment is golang/go#36982. Suggest progressing once that is sorted.

I'm a little concerned that in practice, usage of SafeWriteResponse() always results in a subtle bug.

As I understand it, SafeWriteResponse() is intended to address the following scenario:

1. A user calls WriteResponse(), which calls WriteResponsePage(), which fails to encode JSON.
2. The caller receives an error.
3. The caller now wishes http.StatusInternalServerError to be set in the HTTP response.

SafeWriteResponse combines these three steps.

This leads me to where I think there is an issue. According to the http.ResponseWriter docs, the response code is written to the caller on the first call to (http.ResponseWriter).WriteError() or (http.ResponseWriter).Write() (whichever comes first.) In the above example, this always occurs in step 1, and thus any future call to set an HTTP response code is ignored.

I do think there is merit to the above behaviour. We could make this the default in WriteResponse() if we encoded the JSON to a buffer to ensure it could be encoded before writing it to the response. Thoughts?

Add markdownlint to CircleCI workflow.