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.

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

documents utils #545

Merged
merged 3 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
275 changes: 275 additions & 0 deletions src/PinguApps.Appwrite.Shared/Converters/DocumentGenericConverter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,275 @@
using System;
using System.Collections.Generic;
using System.Text.Json;
using System.Text.Json.Serialization;
using PinguApps.Appwrite.Shared.Responses;
using PinguApps.Appwrite.Shared.Utils;

namespace PinguApps.Appwrite.Shared.Converters;

public class DocumentGenericConverter<TData> : JsonConverter<Document<TData>>
where TData : class, new()
{
public override Document<TData> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
string? id = null;
string? collectionId = null;
string? databaseId = null;
DateTime? createdAt = null;
DateTime? updatedAt = null;
List<Permission>? permissions = null;
TData data = new();

var dataProperties = new Dictionary<string, object?>();

var dateTimeConverter = new MultiFormatDateTimeConverter();
var permissionListConverter = new PermissionListConverter();

if (reader.TokenType is not JsonTokenType.StartObject)
{
throw new JsonException("Expected StartObject token");
Copy link

Choose a reason for hiding this comment

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

category Error Handling

Log exceptions before throwing.

Tell me more

Consider logging the exception before throwing it. This can help with debugging and monitoring. You could add a logging statement before throwing the exception, like this:

logger.LogError("Expected StartObject token");
throw new JsonException("Expected StartObject token");

Make sure to inject and use an appropriate logging mechanism.

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

}

while (reader.Read())
{
if (reader.TokenType == JsonTokenType.EndObject)
{
break;
}

var propertyName = reader.GetString()!;

reader.Read();

switch (propertyName)
{
case "$id":
id = reader.GetString();
break;
case "$collectionId":
collectionId = reader.GetString();
break;
case "$databaseId":
databaseId = reader.GetString();
break;
case "$createdAt":
createdAt = dateTimeConverter.Read(ref reader, typeof(DateTime), options);
break;
case "$updatedAt":
updatedAt = dateTimeConverter.Read(ref reader, typeof(DateTime), options);
break;
case "$permissions":
permissions = permissionListConverter.Read(ref reader, typeof(List<Permission>), options);
break;
default:
var value = ReadValue(ref reader, options);
dataProperties[propertyName] = value;
break;
}
}

if (id is null)
{
throw new JsonException("Unable to find a value for Id");
}

if (collectionId is null)
{
throw new JsonException("Unable to find a value for CollectionId");
}

if (databaseId is null)
{
throw new JsonException("Unable to find a value for DatabaseId");
}

if (createdAt is null)
{
throw new JsonException("Unable to find a value for CreatedAt");
}

if (updatedAt is null)
{
throw new JsonException("Unable to find a value for UpdatedAt");
}

if (permissions is null)
{
throw new JsonException("Unable to find a value for Permissions");
}

// Deserialize the remaining properties into TData
var dataJson = JsonSerializer.Serialize(dataProperties, options);
data = JsonSerializer.Deserialize<TData>(dataJson, options) ?? new TData();

return new Document<TData>(id, collectionId, databaseId, createdAt.Value, updatedAt.Value, permissions, data);
}

Check notice on line 107 in src/PinguApps.Appwrite.Shared/Converters/DocumentGenericConverter.cs

View check run for this annotation

codefactor.io / CodeFactor

src/PinguApps.Appwrite.Shared/Converters/DocumentGenericConverter.cs#L13-L107

Complex Method
internal object? ReadValue(ref Utf8JsonReader reader, JsonSerializerOptions options)
{
switch (reader.TokenType)
{
case JsonTokenType.String:
var str = reader.GetString();

if ((DateTime.TryParse(str, out var dateTime)))
{
return dateTime;
}
return str;

case JsonTokenType.Number:
if (reader.TryGetInt64(out var longValue))
{
return longValue;
}
return reader.GetSingle();
Copy link

Choose a reason for hiding this comment

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

category Functionality

Use reader.GetDouble() instead of reader.GetSingle() to avoid precision loss.

Tell me more

In the ReadValue method, you're currently using reader.GetSingle() for non-integer numeric values. This may lead to precision loss for decimal numbers. Consider using reader.GetDouble() instead to support a wider range of decimal values with higher precision. This change will ensure that the JSON deserialization process accurately represents decimal numbers in the resulting object.

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.


case JsonTokenType.True:
case JsonTokenType.False:
return reader.GetBoolean();

case JsonTokenType.Null:
return null;

case JsonTokenType.StartArray:
return ReadArray(ref reader, options);

case JsonTokenType.StartObject:
return ReadObject(ref reader, options);

default:
throw new JsonException($"Unsupported token type: {reader.TokenType}");
Copy link

Choose a reason for hiding this comment

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

category Error Handling

Consider logging the error before throwing the exception.

Tell me more

Before throwing this exception, consider logging the error. This can provide valuable information for debugging and monitoring. You could add a logging statement like this:

logger.LogError($"Unsupported token type encountered: {reader.TokenType}");
throw new JsonException($"Unsupported token type: {reader.TokenType}");

Ensure you have a proper logging mechanism in place.

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

}
}

private IReadOnlyCollection<object?> ReadArray(ref Utf8JsonReader reader, JsonSerializerOptions options)
{
var list = new List<object?>();

while (reader.Read())
{
if (reader.TokenType is JsonTokenType.EndArray)
{
break;
}

var item = ReadValue(ref reader, options);
list.Add(item);
}

return list;
}

private Dictionary<string, object?> ReadObject(ref Utf8JsonReader reader, JsonSerializerOptions options)
{
var dict = new Dictionary<string, object?>();

while (reader.Read())
{
if (reader.TokenType is JsonTokenType.EndObject)
{
break;
}

var propertyName = reader.GetString()!;

reader.Read();

var value = ReadValue(ref reader, options);

dict[propertyName] = value;
}

return dict;
}

public override void Write(Utf8JsonWriter writer, Document<TData> value, JsonSerializerOptions options)
{
writer.WriteStartObject();

writer.WriteString("$id", value.Id);
writer.WriteString("$collectionId", value.CollectionId);
writer.WriteString("$databaseId", value.DatabaseId);

// Use MultiFormatDateTimeConverter for DateTime properties
var dateTimeConverter = new MultiFormatDateTimeConverter();

writer.WritePropertyName("$createdAt");
dateTimeConverter.Write(writer, value.CreatedAt, options);

writer.WritePropertyName("$updatedAt");
dateTimeConverter.Write(writer, value.UpdatedAt, options);

writer.WritePropertyName("$permissions");
JsonSerializer.Serialize(writer, value.Permissions, options);

// Serialize the Data property
if (value.Data is not null)
{
var dataProperties = JsonSerializer.SerializeToElement(value.Data, options);
foreach (var property in dataProperties.EnumerateObject())
{
writer.WritePropertyName(property.Name);
WriteValue(writer, property.Value, options);
}
}

writer.WriteEndObject();
}

internal void WriteValue(Utf8JsonWriter writer, JsonElement element, JsonSerializerOptions options)
{
var dateTimeConverter = new MultiFormatDateTimeConverter();

switch (element.ValueKind)
{
case JsonValueKind.String:
var stringValue = element.GetString();
if (DateTime.TryParse(stringValue, out var dateTimeValue))
{
// Write DateTime using the MultiFormatDateTimeConverter
dateTimeConverter.Write(writer, dateTimeValue, options);
}
else
{
writer.WriteStringValue(stringValue);
}
break;
case JsonValueKind.Number:
if (element.TryGetInt32(out var intValue))
writer.WriteNumberValue(intValue);
else if (element.TryGetInt64(out var longValue))
writer.WriteNumberValue(longValue);
else if (element.TryGetDouble(out var doubleValue))
writer.WriteNumberValue(doubleValue);
break;
Comment on lines +239 to +246
Copy link

Choose a reason for hiding this comment

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

category Functionality

Consider adding support for additional numeric types.

Tell me more

In the WriteValue method, you're currently handling int, long, and double numeric types. However, you might want to consider adding support for other numeric types such as decimal, float, and byte to ensure comprehensive handling of various numeric data. This would involve adding additional checks and using the appropriate writer methods for these types. For example, you could add else if (element.TryGetDecimal(out var decimalValue)) writer.WriteNumberValue(decimalValue); for handling decimal values.

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

case JsonValueKind.True:
case JsonValueKind.False:
writer.WriteBooleanValue(element.GetBoolean());
break;
case JsonValueKind.Null:
writer.WriteNullValue();
break;
case JsonValueKind.Array:
writer.WriteStartArray();
foreach (var item in element.EnumerateArray())
{
WriteValue(writer, item, options);
}
writer.WriteEndArray();
break;
case JsonValueKind.Object:
writer.WriteStartObject();
foreach (var property in element.EnumerateObject())
{
writer.WritePropertyName(property.Name);
WriteValue(writer, property.Value, options);
}
writer.WriteEndObject();
break;
case JsonValueKind.Undefined:
throw new JsonException("Cannot serialize undefined JsonElement");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
using System;
using System.Text.Json;
using System.Text.Json.Serialization;
using PinguApps.Appwrite.Shared.Responses;

namespace PinguApps.Appwrite.Shared.Converters;
public class DocumentGenericConverterFactory : JsonConverterFactory
{
public override bool CanConvert(Type typeToConvert)
{
// Ensure the type is a generic type of Document<>
return typeToConvert.IsGenericType && typeToConvert.GetGenericTypeDefinition() == typeof(Document<>);
}

public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options)
{
// Extract the TData type from Doocument<TData>
Type dataType = typeToConvert.GetGenericArguments()[0];

// Create a specific generic converter for Doocument<TData>
var converterType = typeof(DocumentGenericConverter<>).MakeGenericType(dataType);
return (JsonConverter?)Activator.CreateInstance(converterType);
Copy link

Choose a reason for hiding this comment

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

category Functionality

Include error handling in the CreateConverter method.

Tell me more

The CreateConverter method currently doesn't include any error handling. Consider wrapping the Activator.CreateInstance call in a try-catch block to handle potential exceptions that might occur during the creation of the converter instance. This will improve the robustness of the code and prevent unexpected crashes.

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

Copy link

Choose a reason for hiding this comment

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

category Error Handling

Wrap Activator.CreateInstance() in a try-catch block.

Tell me more

The Activator.CreateInstance() method can throw various exceptions, such as MissingMethodException, TargetInvocationException, or InvalidOperationException. It's recommended to wrap this call in a try-catch block to handle potential exceptions gracefully. This will improve the robustness of your code and provide better error handling.

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

}
}
14 changes: 7 additions & 7 deletions src/PinguApps.Appwrite.Shared/Responses/Document.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ namespace PinguApps.Appwrite.Shared.Responses;
/// <param name="Data">Document data</param>
[JsonConverter(typeof(DocumentConverter))]
public record Document(
[property: JsonPropertyName("$id")] string Id,
[property: JsonPropertyName("$collectionId")] string CollectionId,
[property: JsonPropertyName("$databaseId")] string DatabaseId,
[property: JsonPropertyName("$createdAt"), JsonConverter(typeof(MultiFormatDateTimeConverter))] DateTime CreatedAt,
[property: JsonPropertyName("$updatedAt"), JsonConverter(typeof(MultiFormatDateTimeConverter))] DateTime UpdatedAt,
[property: JsonPropertyName("$permissions"), JsonConverter(typeof(PermissionReadOnlyListConverter))] IReadOnlyList<Permission> Permissions,
string Id,
string CollectionId,
string DatabaseId,
DateTime CreatedAt,
DateTime UpdatedAt,
IReadOnlyList<Permission> Permissions,
[property: JsonExtensionData] Dictionary<string, object?> Data
)
) : DocumentBase(Id, CollectionId, DatabaseId, CreatedAt, UpdatedAt, Permissions)
{
/// <summary>
/// Extract document data by key
Expand Down
26 changes: 26 additions & 0 deletions src/PinguApps.Appwrite.Shared/Responses/DocumentBase.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
using System;
using System.Collections.Generic;
using System.Text.Json.Serialization;
using PinguApps.Appwrite.Shared.Converters;
using PinguApps.Appwrite.Shared.Utils;

namespace PinguApps.Appwrite.Shared.Responses;

/// <summary>
/// An Appwrite Document object
/// </summary>
/// <param name="Id">Document ID</param>
/// <param name="CollectionId">Collection ID</param>
/// <param name="DatabaseId">Database ID</param>
/// <param name="CreatedAt">Document creation date in ISO 8601 format</param>
/// <param name="UpdatedAt">Document update date in ISO 8601 format</param>
/// <param name="Permissions">Document permissions. <see href="https://appwrite.io/docs/permissions">Learn more about permissions</see></param>
[JsonConverter(typeof(DocumentConverter))]
public abstract record DocumentBase(
Copy link

Choose a reason for hiding this comment

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

category Functionality

Clarify usage of the abstract DocumentBase class.

Tell me more

The DocumentBase class is marked as abstract, which is good for defining a base structure. However, it's not clear how this class should be extended or used in practice. Consider adding a comment or example showing how to create a concrete implementation of this class. This will help other developers understand how to properly use this base class in their implementations.

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

[property: JsonPropertyName("$id")] string Id,
[property: JsonPropertyName("$collectionId")] string CollectionId,
[property: JsonPropertyName("$databaseId")] string DatabaseId,
[property: JsonPropertyName("$createdAt"), JsonConverter(typeof(MultiFormatDateTimeConverter))] DateTime CreatedAt,
[property: JsonPropertyName("$updatedAt"), JsonConverter(typeof(MultiFormatDateTimeConverter))] DateTime UpdatedAt,
[property: JsonPropertyName("$permissions"), JsonConverter(typeof(PermissionReadOnlyListConverter))] IReadOnlyList<Permission> Permissions
);
29 changes: 29 additions & 0 deletions src/PinguApps.Appwrite.Shared/Responses/DocumentGeneric.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
using System;
using System.Collections.Generic;
using System.Text.Json.Serialization;
using PinguApps.Appwrite.Shared.Converters;
using PinguApps.Appwrite.Shared.Utils;

namespace PinguApps.Appwrite.Shared.Responses;

/// <summary>
/// An Appwrite Document object
/// </summary>
/// <param name="Id">Document ID</param>
/// <param name="CollectionId">Collection ID</param>
/// <param name="DatabaseId">Database ID</param>
/// <param name="CreatedAt">Document creation date in ISO 8601 format</param>
/// <param name="UpdatedAt">Document update date in ISO 8601 format</param>
/// <param name="Permissions">Document permissions. <see href="https://appwrite.io/docs/permissions">Learn more about permissions</see></param>
/// <param name="Data">Document data</param>
[JsonConverter(typeof(DocumentGenericConverterFactory))]
public record Document<TData>(
string Id,
string CollectionId,
string DatabaseId,
DateTime CreatedAt,
DateTime UpdatedAt,
IReadOnlyList<Permission> Permissions,
TData Data
) : DocumentBase(Id, CollectionId, DatabaseId, CreatedAt, UpdatedAt, Permissions)
where TData : class, new();
Loading