Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable nullable globally and fix issues #21

Closed
wants to merge 5 commits into from
+1,197 −1,156
Closed

Enable nullable globally and fix issues #21

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6b3ad42
Enable nullable globally and fix issues
erikmav Mar 31, 2023
d67f11c
Wave 2 with updated nullable IpfsShipyard.Ipfs.Core
erikmav Mar 31, 2023
c54af88
Remove ArgNullEx exception check in UT
erikmav Mar 31, 2023
acddd18
Make use of new ipfscore package. Various analyzer updates
erikmav Apr 17, 2023
7e13391
Update src/Block.cs
Arlodotexe Apr 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions Directory.Build.props
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project>

<PropertyGroup>
<Nullable>enable</Nullable>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<langversion>10.0</langversion>
</PropertyGroup>

</Project>
60 changes: 29 additions & 31 deletions src/Block.cs
View file
Original file line number Diff line number Diff line change
@@ -1,33 +1,33 @@
using System.IO;
using System;
using System.IO;
using System.Runtime.Serialization;

namespace Ipfs.Http
{
/// <inheritdoc />
[DataContract]
public class Block : IDataBlock
{
long? size;

/// <inheritdoc />
[DataMember]
public Cid Id { get; set; }

/// <inheritdoc />
[DataMember]
public byte[] DataBytes { get; set; }

/// <inheritdoc />
public Stream DataStream
{
get
{
return new MemoryStream(DataBytes, false);
}

namespace Ipfs.Http
{
/// <inheritdoc />
[DataContract]
public class Block : IDataBlock
{
private long? size;
private Cid? id;

/// <inheritdoc />
[DataMember]
public Cid Id
{
get => id ?? throw new InvalidDataException("Value mus be initialized");
set => id = value;
}

/// <inheritdoc />
[DataMember]
/// <inheritdoc />
[DataMember]
public byte[] DataBytes { get; set; } = Array.Empty<byte>();

/// <inheritdoc />
public Stream DataStream => new MemoryStream(DataBytes, false);

/// <inheritdoc />
[DataMember]
public long Size
{
get
@@ -43,7 +43,5 @@ public long Size
size = value;
}
}

}

}
}
}
65 changes: 32 additions & 33 deletions src/CoreApi/BitswapApi.cs
View file
Original file line number Diff line number Diff line change
@@ -1,58 +1,57 @@
using Ipfs.CoreApi;
using Newtonsoft.Json.Linq;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Newtonsoft.Json.Linq;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

namespace Ipfs.Http
{
class BitswapApi : IBitswapApi
{
IpfsClient ipfs;

internal BitswapApi(IpfsClient ipfs)
{
this.ipfs = ipfs;
namespace Ipfs.Http
{
class BitswapApi : IBitswapApi
{
private readonly IpfsClient ipfs;
internal BitswapApi(IpfsClient ipfs)
{
this.ipfs = ipfs;
}

public Task<IDataBlock> GetAsync(Cid id, CancellationToken cancel = default(CancellationToken))
public Task<IDataBlock> GetAsync(Cid id, CancellationToken cancel = default)
{
return ipfs.Block.GetAsync(id, cancel);
}

public async Task<IEnumerable<Cid>> WantsAsync(MultiHash peer = null, CancellationToken cancel = default(CancellationToken))
public async Task<IEnumerable<Cid>> WantsAsync(MultiHash? peer = null, CancellationToken cancel = default)
{
var json = await ipfs.DoCommandAsync("bitswap/wantlist", cancel, peer?.ToString());
var keys = (JArray)(JObject.Parse(json)["Keys"]);
// https://github.com/ipfs/go-ipfs/issues/5077
return keys
var keys = (JArray?)(JObject.Parse(json)["Keys"]);
// https://github.com/ipfs/go-ipfs/issues/5077
return keys
.Select(k =>
{
if (k.Type == JTokenType.String)
return Cid.Decode(k.ToString());
return Cid.Decode(k.ToString());
var obj = (JObject)k;
return Cid.Decode(obj["/"].ToString());
});
return Cid.Decode(obj["/"]!.ToString());
});
}

public async Task UnwantAsync(Cid id, CancellationToken cancel = default(CancellationToken))
public async Task UnwantAsync(Cid id, CancellationToken cancel = default)
{
await ipfs.DoCommandAsync("bitswap/unwant", cancel, id);
}

public async Task<BitswapLedger> LedgerAsync(Peer peer, CancellationToken cancel = default(CancellationToken))
public async Task<BitswapLedger> LedgerAsync(Peer peer, CancellationToken cancel = default)
{
var json = await ipfs.DoCommandAsync("bitswap/ledger", cancel, peer.Id.ToString());
var json = await ipfs.DoCommandAsync("bitswap/ledger", cancel, peer.Id?.ToString());
var o = JObject.Parse(json);
return new BitswapLedger
{
Peer = (string)o["Peer"],
DataReceived = (ulong)o["Sent"],
DataSent = (ulong)o["Recv"],
BlocksExchanged = (ulong)o["Exchanged"]
Peer = (string)o["Peer"]!,
DataReceived = (ulong?)o["Sent"] ?? 0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@Arlodotexe Arlodotexe Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

DataSent = (ulong?)o["Recv"] ?? 0,
BlocksExchanged = (ulong?)o["Exchanged"] ?? 0,
};
}
}

}
}
}
162 changes: 80 additions & 82 deletions src/CoreApi/BlockApi.cs
View file
Original file line number Diff line number Diff line change
@@ -1,113 +1,111 @@
using Ipfs.CoreApi;
using Newtonsoft.Json.Linq;
using System.Collections.Generic;
using Newtonsoft.Json.Linq;
using System.Collections.Generic;
using System.IO;
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;

namespace Ipfs.Http
{
class BlockApi : IBlockApi
{
IpfsClient ipfs;

internal BlockApi(IpfsClient ipfs)
{
this.ipfs = ipfs;
}

public async Task<IDataBlock> GetAsync(Cid id, CancellationToken cancel = default(CancellationToken))
{
var data = await ipfs.DownloadBytesAsync("block/get", cancel, id);
return new Block
{
DataBytes = data,
Id = id
};
}

public async Task<Cid> PutAsync(
namespace Ipfs.Http
{
class BlockApi : IBlockApi
{
private readonly IpfsClient ipfs;
internal BlockApi(IpfsClient ipfs)
{
this.ipfs = ipfs;
}
public async Task<IDataBlock> GetAsync(Cid id, CancellationToken cancel = default)
{
var data = await ipfs.DownloadBytesAsync("block/get", cancel, id);
return new Block
{
DataBytes = data,
Id = id
};
}
public async Task<Cid> PutAsync(
byte[] data,
string contentType = Cid.DefaultContentType,
string multiHash = MultiHash.DefaultAlgorithmName,
string encoding = MultiBase.DefaultAlgorithmName,
bool pin = false,
CancellationToken cancel = default(CancellationToken))
{
var options = new List<string>();
string contentType = Cid.DefaultContentType,
string multiHash = MultiHash.DefaultAlgorithmName,
string encoding = MultiBase.DefaultAlgorithmName,
bool pin = false,
CancellationToken cancel = default)
{
var options = new List<string>();
if (multiHash != MultiHash.DefaultAlgorithmName ||
contentType != Cid.DefaultContentType ||
encoding != MultiBase.DefaultAlgorithmName)
{
options.Add($"mhtype={multiHash}");
options.Add($"format={contentType}");
options.Add($"cid-base={encoding}");
}
}
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"]!;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (pin)
{
await ipfs.Pin.AddAsync(cid, recursive: false, cancel: cancel);
}

return cid;
}

public async Task<Cid> PutAsync(
}
return cid;
}
public async Task<Cid> PutAsync(
Stream data,
string contentType = Cid.DefaultContentType,
string multiHash = MultiHash.DefaultAlgorithmName,
string encoding = MultiBase.DefaultAlgorithmName,
bool pin = false,
CancellationToken cancel = default(CancellationToken))
{
var options = new List<string>();
string contentType = Cid.DefaultContentType,
string multiHash = MultiHash.DefaultAlgorithmName,
string encoding = MultiBase.DefaultAlgorithmName,
bool pin = false,
CancellationToken cancel = default)
{
var options = new List<string>();
if (multiHash != MultiHash.DefaultAlgorithmName ||
contentType != Cid.DefaultContentType ||
encoding != MultiBase.DefaultAlgorithmName)
{
options.Add($"mhtype={multiHash}");
options.Add($"format={contentType}");
options.Add($"cid-base={encoding}");
}
var json = await ipfs.UploadAsync("block/put", cancel, data, null, options.ToArray());
}
var json = await ipfs.UploadAsync("block/put", cancel, data, name: null, options.ToArray());
var info = JObject.Parse(json);
Cid cid = (string)info["Key"];

Cid cid = (string)info["Key"]!;
if (pin)
{
await ipfs.Pin.AddAsync(cid, recursive: false, cancel: cancel);
}

return cid;
}

public async Task<IDataBlock> StatAsync(Cid id, CancellationToken cancel = default(CancellationToken))
{
var json = await ipfs.DoCommandAsync("block/stat", cancel, id);
}
return cid;
}
public async Task<IDataBlock> StatAsync(Cid id, CancellationToken cancel = default)
{
var json = await ipfs.DoCommandAsync("block/stat", cancel, id);
var info = JObject.Parse(json);
return new Block
{
Size = (long)info["Size"],
Id = (string)info["Key"]
};
}

public async Task<Cid> RemoveAsync(Cid id, bool ignoreNonexistent = false, CancellationToken cancel = default(CancellationToken))
{
var json = await ipfs.DoCommandAsync("block/rm", cancel, id, "force=" + ignoreNonexistent.ToString().ToLowerInvariant());
if (json.Length == 0)
return null;
var result = JObject.Parse(json);
var error = (string)result["Error"];
if (error != null)
throw new HttpRequestException(error);
return (Cid)(string)result["Hash"];
return new Block
{
Size = (long?)info["Size"] ?? 0,
Copy link
Collaborator

@Arlodotexe Arlodotexe Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary fallback value, the API can return null for size.

Suggested change
Size = (long?)info["Size"] ?? 0,
Size = (long?)info["Size"],

Id = (string)info["Key"]!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use parameter validation here instead of null suppression.

};
}

}

}
public async Task<Cid?> RemoveAsync(Cid id, bool ignoreNonexistent = false, CancellationToken cancel = default)
{
var json = await ipfs.DoCommandAsync("block/rm", cancel, id, "force=" + ignoreNonexistent.ToString().ToLowerInvariant());
if (json.Length == 0)
return null;
var result = JObject.Parse(json);
var error = (string?)result["Error"];
if (error is not null)
throw new HttpRequestException(error);
return (Cid)(string)result["Hash"]!;
}
}
}
Loading