Enable nullable globally and fix issues #21
Conversation
There was a problem hiding this comment.
Let's clean up the null suppression before we close this off. As a library that parses raw (all technically nullable) API data into strongly-typed C#, we need to be validating parameters and return values to make sure they abide to our declared net-ipfs-core API surface.
I've left several examples of what to change, but didn't tag everything I saw. Let's do a sweep using the examples.
| DataSent = (ulong)o["Recv"], | ||
| BlocksExchanged = (ulong)o["Exchanged"] | ||
| Peer = (string)o["Peer"]!, | ||
| DataReceived = (ulong?)o["Sent"] ?? 0, |
There was a problem hiding this comment.
If the API returns a null value for these, we should bubble that up to the consuming application. Let's remove the fallback null coalescence values.
There was a problem hiding this comment.
These changes will require a new wave of nullable updates in IpfsCore, as the declarations of the data classes there are not nullable after the recent 0.0.5 updates. Will send a new PR from there.
There was a problem hiding this comment.
Wait, why does that require changes in IpfsCore? These are being null suppressed, I assumed the json models were already nullable. That's how it should be, as the data could go missing from the API surface if you use a new version of Kubo with breaking changes, or if JSON doesn't deserialize correctly for some reason.
For the models exposed to the consumer of net-ipfs-http-client, if the official specification says it's nullable or not, then the models in IpfsCore should reflect that. It's the underlying API model properties used for deserialization that need to be nullable. This is already the case, because we're manually grabbing fields using Newtonsoft (e.g. o["Peer"] is nullable).
We just need to do null check / throw here. Simple validation, so that suppressed null values aren't returned and passed around to other parts of the application.
| var json = await ipfs.UploadAsync("block/put", cancel, data, options.ToArray()); | ||
| var info = JObject.Parse(json); | ||
| Cid cid = (string)info["Key"]; | ||
|
|
||
| Cid cid = (string)info["Key"]!; |
There was a problem hiding this comment.
There's nothing stopping this suppressed null value from being returned and passed around to other parts of the application.
Instead of suppressing and returning null values in these methods, we should throw instead. Early validation makes for clean stack traces and faster debugging.
Let's do this for each method in our core implementations (not just this line), to make sure it doesn't return a supressed null value.
| return (Cid)(string)result["Hash"]; | ||
| return new Block | ||
| { | ||
| Size = (long?)info["Size"] ?? 0, |
There was a problem hiding this comment.
Unnecessary fallback value, the API can return null for size.
| Size = (long?)info["Size"] ?? 0, | |
| Size = (long?)info["Size"], | |
| return new Block | ||
| { | ||
| Size = (long?)info["Size"] ?? 0, | ||
| Id = (string)info["Key"]! |
There was a problem hiding this comment.
Use parameter validation here instead of null suppression.
|
This PR has too many conflicts to be merged safely. We should do a fresh pass fixing nullables, but we need to make sure our exposed API surface matches the underlying Kubo API. |
For #20, utilizing a locally published version of the nullable IpfsShipyard.Ipfs.Core.dll from ipfs-shipyard/net-ipfs-core#21 . Remove some ArgumentNullException where nullability covers it to reduce runtime code (this is a change to public surface area, can be reverted - see changes in unit tests). Some opportunistic code cleanup ahead of #8 .