Rest client support for endpoint not returning a JSON

[nobe4]

Problem statement

While working on gh-not I started experimenting with Actors interacting with the GitHub API.

A very simple one would be to mark a notification as read, as per https://docs.github.com/en/rest/activity/notifications?apiVersion=2022-11-28#mark-a-thread-as-read.

Here's a simplified version of the code I wanted to use:

func main() {
    client, err := api.DefaultRESTClient()
    if err != nil {
        panic(err)
    }

    url := "https://api.github.com/notifications/threads/[REDACTED]"

    resp := []interface{}{}
    err = client.Patch(url, nil, &resp)
    if err != nil {
        panic(err)
    }

}

And I always get: panic: unexpected end of JSON input

Possible fix 1

The documentation specifies status codes, but no response content; expecting one is thus useless in this context.

I wonder if https://github.com/cli/go-gh/blob/dbd982eba2a041d88d398cce01cb708c4c3503f7/pkg/api/rest_client.go#L103-L111 could be modified in the following way:

    b, err := io.ReadAll(resp.Body)
    if err != nil {
        return err
    }

+        if len(b) > 0{
            err = json.Unmarshal(b, &response)
            if err != nil {
                return err
            }
+        }

This would prevent breaking on invalid JSON when none is expected.

POC implemented in https://github.com/cli/go-gh/pull/162

Possible fix 2

The 205 status code spec specifies:

a server MUST NOT generate content in a 205 response

So maybe a better fix would be:

    if resp.StatusCode == http.StatusNoContent {
        return nil
    }

+   if resp.StatusCode == http.StatusResetContent {
+       return nil
+   }

Implementation PR: https://github.com/cli/go-gh/pull/163

Current workaround

func main() {
    client, err := api.DefaultRESTClient()
    if err != nil {
        panic(err)
    }

    url := "https://api.github.com/notifications/threads/[REDACTED]"

    resp := []interface{}{}
    err = client.Patch(url, nil, &resp)
    if err != nil && err.Error() != "unexpected end of JSON input" {
        panic(err)
    }
        // continue as planned
}

Additional questions

• Is there another way to use go-gh's REST client for Patch requests if the endpoint doesn't return a valid JSON?
• Would there be any benefits in returning the response code from those requests? It seems that implementing custom logic solely based on JSON breaks as soon as no JSON is returned

[andyfeller]

I was curious about what the stated about RFC 7231 > 6.3.6 > "205 Reset Content", so thought I'd look it up:

The 205 (Reset Content) status code indicates that the server has fulfilled the request and desires that the user agent reset the "document view", which caused the request to be sent, to its original state as received from the origin server.

This response is intended to support a common data entry use case where the user receives content that supports data entry (a form, notepad, canvas, etc.), enters or manipulates data in that space, causes the entered data to be submitted in a request, and then the data entry mechanism is reset for the next entry so that the user can easily initiate another input action.

Since the 205 status code implies that no additional content will be provided, a server MUST NOT generate a payload in a 205 response. In other words, a server MUST do one of the following for a 205 response:

1. indicate a zero-length body for the response by including a Content-Length header field with a value of 0

2. indicate a zero-length payload for the response by including a Transfer-Encoding header field with a value of chunked and a message body consisting of a single chunk of zero-length

3. close the connection immediately after sending the blank line terminating the header section.

I think we all agree #163 is a clear ✅ as regardless of how the server is indicating to the client, there should be no content and we can just skip the rest.

My concern in #162 — skip unmarshaling the response if empty — is whether it hides a potential err from the caller.

[nobe4]

I think we all agree https://github.com/cli/go-gh/pull/163 is a clear ✅

I'm glad to hear, let's see on the PR how we can make the tests better. I'll open it up for review.

My concern in https://github.com/cli/go-gh/pull/162 — skip unmarshaling the response if empty — is whether it hides a potential err from the caller.

I agree with this assessment, there needs to be a better error handling. Currently, the only error we get is unexpected end of JSON input, which says little about the actual HTTP response.

I was considering requesting to also get additional HTTP information out of those methods, so that specific error codes could be dealt with by the client.

[williammartin]

My concern in https://github.com/cli/go-gh/pull/162 — skip unmarshaling the response if empty — is whether it hides a potential err from the caller.

Can you think of an example of a scenario in which a developer using go-gh would rather have a panic from inside go-gh than for the response struct to be nil?