docstore: fixes to improve handling of byte arrays and non-pointer protocol buffers #2926
Conversation
| if b, ok := d.AsBytes(); ok { | ||
| v.SetBytes(b) | ||
| if v.Kind() == reflect.Slice { | ||
| v.SetBytes(b) |
There was a problem hiding this comment.
This would previously panic with reflect: call of reflect.Value.SetBytes on array Value.
There was a problem hiding this comment.
The check for slice is good, but if false I think you want to fall through to the general list-handling code on line 536/551. It should handle copying into arrays of various size, not just the same size. Currently you will not assign if the destination array is a different size, which is a bug.
There was a problem hiding this comment.
Hmm, that does not work. If I let it fall through, the call to d.ListLen() below fails.
There was a problem hiding this comment.
OK, keep your code and return an error if the byte array is a different size, maybe with a TODO that it should work like the general array-copying code below.
There was a problem hiding this comment.
I modified it to call prepareLen it if it's an array, I think this does the right thing but PTAL.
Codecov Report
@@ Coverage Diff @@
## master #2926 +/- ##
==========================================
- Coverage 69.52% 69.37% -0.15%
==========================================
Files 111 111
Lines 11500 11557 +57
==========================================
+ Hits 7995 8018 +23
- Misses 2850 2884 +34
Partials 655 655
Continue to review full report at Codecov.
|
vangent
changed the title
| if b, ok := d.AsBytes(); ok { | ||
| v.SetBytes(b) | ||
| if v.Kind() == reflect.Slice { | ||
| v.SetBytes(b) |
There was a problem hiding this comment.
The check for slice is good, but if false I think you want to fall through to the general list-handling code on line 536/551. It should handle copying into arrays of various size, not just the same size. Currently you will not assign if the destination array is a different size, which is a bug.
|
Thanks for dealing with this, Robert. I missed my mention on the original issue. |
|
Thanks for the review @jba, PTAL. |
3 participants
Fixes #2920
Fixes #2791 (I think)
@jba I'm not familiar with those code and am not sure this is correct. Please advise.
The newly added tests panic without the changes in
codec.go.