fix JSON deserialization of peer.AddrInfo #177
Conversation
|
Can we turn off / reconfigure Codecov? It's ridiculous that it's failing here. |
Why is it ridiculous? This PR fixes an issue with deserialization that wasn't caught because it wasn't tested. There's no test case verifying this fix, which makes it entirely possible for this to be an issue in the future if it's left untested. Codecov configuration may be off, but I think it's a completely valid failure here. |
There was no test case for the deserialization in the first place, and there's no test case now, so this PR doesn't make anything worse. It's just modifying untested code. |
|
My short response here is that a test should be added verifying this fix does what it says it does. It's low effort, low impact on CI times and prevents regressions of this bug. Regardless of the existence of code coverage we should be validating patches with tests. |
|
Fair enough, I added a test case testing basic encoding / decoding of |
peer/addrinfo_serde.go
Outdated
| pid, err := IDB58Decode(data["ID"].(string)) | ||
| if err != nil { | ||
| return err | ||
| if id, ok := data["ID"]; ok { |
There was a problem hiding this comment.
Shouldn't this error if there's no ID? I'd expect addrInfo.UnmarshalJSON([]byte(`{"Addrs":[]}`)) or addrInfo.UnmarshalJSON([]byte(`{"foo":"bar"}`)) to error instead of returning nil. A test validating the bad path expectation would be great. No addresses is fine, but no ID isn't.
There was a problem hiding this comment.
Ok, added. PTAL.
marten-seemann
force-pushed
the
fix-addrinfo-json-deserialization
branch
from
c361c19
to
43d94f1
Compare
| if addrsEntry, ok := data["Addrs"]; ok { | ||
| if addrs, ok := addrsEntry.([]interface{}); ok { | ||
| for _, a := range addrs { | ||
| pi.Addrs = append(pi.Addrs, ma.StringCast(a.(string))) |
There was a problem hiding this comment.
PR looks good, the only outstanding issue I see is that ma.StringCast will panic if the multiaddr is malformed or we don't know one of the codecs in the multiaddr. Given this might contain an unknown codec perhaps we should just ignore the particular addr and continue trying to unmarshal the others?
|
How about #178? This lets go do all the error/type checking for us. |
|
Closing in favor of #178. |
No description provided.