Bug with how AcceptedError is handled

[xibz]

Computing stats, ListContributorsStats, will return an AcceptedError and a body that is a JSON object and not a JSON array. However, it seems this will try to decode it as a JSON array causing a serialization error.

Commit that caused the bug... this seems to be a breaking change as well; I guess it is now obvious that it is:
88eb4e9

My suggestion would be to internally type alias the []ContributorStats and add a ImmediateAcceptedErrorReturn interface that can be casted during the client.Do function. If it is castable to an ImmediateAcceptedErrorReturn, it will immediately return the AcceptedError and not try to unmarshal the body. This makes it completely backwards compatible. I'll see if I can get some time to do this myself! :)

[gmlewis]

Thank you, @xibz, for reporting this issue.

@dsymonds (author of #878) may have some ideas about this issue and your proposal, and I would greatly appreciate his feedback when he has time.

[dsymonds]

Is the response body for AcceptedError documented anywhere? (I'm guessing not.)

If the problem is that certain APIs will return different body forms for 200 and 202 responses, how widespread is that? If it only happens in a small number of cases, there's two good options that come to mind:

1. Have those places provide json.RawMessage to Do. Then the JSON decoding will pass the bytes back, and the caller (ListContributorsStats, etc.) can do the unmarshaling themselves based on the error.
2. Have those places implement json.Unmarshaler on the types that get passed through, sniffing the bytes to detect the right type. This seems unclean (it'll affect everyone else using those types).

But to return to my original question: is the body of the AcceptedError in these cases wanted? What does it even mean for ListContributorsStats to fail in this way? For, say, CreateFork, an AcceptedError means that the creation request was accepted, it'll happen in the background, and the caller should poll to find out when it finishes. Does that same pattern apply to ListContributorsStats?

[xibz]

Yes, ListContributorsStats behaves the same way, where you have to poll until the process is done completing. However, I don't understand why the contents of the response is important for things like CreateFork? Once I understand that, I can make a good guess on whether it is good to have those contents also for ListContributorsStats.

If I am understanding 1, would this be another breaking change where AcceptedError will no longer do the unmarshaling of the body but instead contain the raw json?

[dsymonds]

For CreateFork, the 202 Accepted response includes most of the repository information (things like the new repo's name), which is necessary for polling using a different API (e.g. GetRepository). In its case, the 202 Accepted response is a subset of the 200 OK response, and the CreateFork Go API returns it to the caller, which can detect the 202 Accepted and use the response.

If I understand right, ListContributorStats returns some completely different response in the 202 Accepted case. Do you know what that is? What is the caller supposed to do with it? How would you change the ListContributorStats Go API to support that?

[xibz]

No documentation unfortunately on any of the stats accepted cases. It only returns the 202 on the first time it is called for the day, then every call after that will be a 200.

I would change the AcceptedError actually to contain a Raw field. If an accepted error was returned then allow for each call to handle how it should unmarshal the raw body contents. Things like CreateFork would need to be updated to handle the new Raw field. This would allow for the change to be backwards compatible.

I would even take this one step further. In CreateFork I would return a new error that would be something like CreateForkAcceptedError containing the metadata needed. This will allow for more flexible usage on the users end :). However, this would be a breaking change :(

Edited: got a little bit ahead of myself

[xibz]

Update: Looks like ListContributorsStats contains an empty JSON object when AcceptedError occurs.

[dsymonds]

Okay, so maybe ListContributorsStats should pass json.RawMessage to Do, and parse the data into the real response if Do doesn't return an error. That's not a breaking change, and it avoids complicating the lower level parts.

[xibz]

Hm, how do you feel about putting a Raw field on AcceptedError? I think that is more flexible and it becomes a little more obvious to what the API handling code is doing. If users want more metadata they can pull the Raw message themselves and do what they will with it.

I think passing json.RawMessage is an okay alternative, but I don't know if I like the controlling logic passing a json.RawMessage around and doesn't seem obvious if you are looking at the code. But if that is what's preferred over the solution above, I wont mind changing that.

[dsymonds]

Well, that would be a breaking change. It's not my call to make for this package; I'm just suggesting using json.RawMessage to remedy the issue you've hit without a breaking change.

[xibz]

I may not be seeing it, but what makes adding a Raw field breaking? The CreateFork will still behave the same as I made the changes to unmarshal the Raw field into the CreateFork output shape. My goal was to make any operation that utilizes the unmarshaling was to handle it in the API handler instead. That would prevent the breaking change, at least. I think the transfer repo also has a modeled 202. I can update the code to also unmarshal when calling transfer a repo. So there wouldn't be any breaking changes with the APIs that required modeled 202s.

Thoughts?

[dsymonds]

Oh, I didn't understand what you were suggesting. Yeah, that might be okay. Want to try that out and we can see how it looks?

[xibz]

Yea, awesome! Thanks @dsymonds!

[xibz]

Updated the PR #1023