fix: handle changes/<change>/patch JSON marshalling error #182
Conversation
The changes/<change>/revisions/<revision>/patch endpoint in gerrit returns a base64 encoded diff by default, which cannot be marshalled as JSON. Creating a unit test for the patch endpoint results in the 'invalid character <some random char> looking for beginning of value` error (same as andygrunwald#166). This change fixes the issue by providing an input to Client.Do() which implements the Writer interface to avoid the JSON marshalling by Client.Do() and allow the caller to handle the response appropriately.
|
Thank you. I will have a look at this, but it may need a bit of time. Note for myself: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-patch @dmitshur If you have time to look into it, I would appreciate it. |
There was a problem hiding this comment.
I left some review comments from a quick look, I hope they're helpful. I haven't tried to test the changes locally so I may have missed something.
In general, the test seems to be useful, but I think the implementation of the fixed behavior has room for improvement. See inline comments for suggestions.
|
|
||
| // *(spw.Target) gives us the actual string value. | ||
| // We append the new data (converted from []byte to string) to it. | ||
| *(spw.Target) = *(spw.Target) + string(p) |
There was a problem hiding this comment.
Appending a string to a string causes a copy and and allocation, so when you have a need to append, it's generally better to stick with []byte which can append without copying and allocating whenever there's enough capacity in the slice.
| // StringPointerWriter is a new type based on *string. | ||
| // We would like it to implement Writer interface so that we can avoid | ||
| // unmarshalling of non JSON responses by Client.Do | ||
| type StringPointerWriter struct { |
There was a problem hiding this comment.
This is an implementation detail of this package, it shouldn't be made exported. Once it's exported, it can't be removed without breaking API compatibility.
| Target *string | ||
| } | ||
|
|
||
| func NewStringPointerWriter(target *string) (*StringPointerWriter, error) { |
There was a problem hiding this comment.
Similarly here, this shouldn't be exported.
| v := new(string) | ||
| resp, err := s.client.Do(req, v) | ||
| strVal := "" | ||
| var v *string = &strVal |
There was a problem hiding this comment.
Can you help clarify what's the motivation to change v := new(string) into the following code?
strVal := ""
var v *string = &strVal
| return nil, nil, err | ||
| } | ||
|
|
||
| resp, err := s.client.Do(req, stringWriter) |
There was a problem hiding this comment.
I suspect it's better to go with another way to achieve the outcome you want.
Consider the similar GetCommitContent method which uses getStringResponseWithoutOptions:
Lines 49 to 56 in 31687dc
As another example, see the approach used by another Gerrit client library here:
The changes//revisions//patch endpoint in gerrit returns a base64 encoded diff by default, which cannot be marshalled as JSON. Creating a unit test for the patch endpoint results in the 'invalid character looking for beginning of value` error (same as #166).
This change fixes the issue by providing an input to Client.Do() which implements the Writer interface to avoid the JSON marshalling by Client.Do() and allow the caller to handle the response appropriately.