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

Jump to bottom

Support decimal precision for Document.FromJson and ToJson by using System.Text.Json instead of LitJson #3534

Merged
merged 4 commits into from
Nov 6, 2024
Merged

Support decimal precision for Document.FromJson and ToJson by using System.Text.Json instead of LitJson #3534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92f492b
Switch Document.FromJson and ToJson to use System.Text.Json instead o…
ashovlin Aug 19, 2024
360a35a
Fix changelog
normj Oct 30, 2024
f0ea6e7
Fix up DynamoDB JSON tests
normj Oct 30, 2024
b46db9e
Modify unit test to avoid tripping the key scanner in CI
normj Oct 31, 2024
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
11 changes: 11 additions & 0 deletions generator/.DevConfigs/f4c592db-5554-4e25-9d2d-8efbe169b757.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"services": [
{
"serviceName": "DynamoDBv2",
"type": "patch",
"changeLogMessages": [
"Switch Document.FromJson and ToJson to use System.Text.Json instead of LitJson. This supports additional precision for decimal values."
]
}
]
}
21 changes: 11 additions & 10 deletions sdk/src/Services/DynamoDBv2/Custom/DocumentModel/DocumentExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
using System;
using System.Collections.Generic;
using System.Collections.Generic;
using System.IO;
using System.Text;
using ThirdParty.Json.LitJson;
using System.Text.Json;

namespace Amazon.DynamoDBv2.DocumentModel
{
Expand Down Expand Up @@ -59,11 +59,12 @@ public static string ToJsonPretty(this IEnumerable<Document> documents)

private static string SerializeEnumerable(IEnumerable<Document> documents, bool prettyPrint)
{
var sb = new StringBuilder();
var writer = new JsonWriter(sb);
writer.PrettyPrint = prettyPrint;
using var stream = new MemoryStream();
using var writer = new Utf8JsonWriter(stream, new JsonWriterOptions {
Indented = prettyPrint
});

writer.WriteArrayStart();
writer.WriteStartArray();
if (documents != null)
{
foreach (var document in documents)
Expand All @@ -74,10 +75,10 @@ private static string SerializeEnumerable(IEnumerable<Document> documents, bool
}
}
}
writer.WriteArrayEnd();
writer.WriteEndArray();

var jsonText = sb.ToString();
return jsonText;
writer.Flush();
return Encoding.UTF8.GetString(stream.ToArray());
}
}
}
164 changes: 92 additions & 72 deletions sdk/src/Services/DynamoDBv2/Custom/DocumentModel/JsonUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,9 @@
using System;
using System.Collections.Generic;
using System.Globalization;
using System.Text;

using Amazon.DynamoDBv2.Model;
using Amazon.Util;
using ThirdParty.Json.LitJson;
using System.IO;
using System.Text;
using System.Text.Json;

namespace Amazon.DynamoDBv2.DocumentModel
{
Expand All @@ -40,11 +37,18 @@ internal static class JsonUtils
/// <returns></returns>
public static Document FromJson(string jsonText)
{
var json = JsonMapper.ToObject(jsonText);
if (!json.IsObject)
var json = JsonDocument.Parse(jsonText, new JsonDocumentOptions
{
AllowTrailingCommas = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

In your description you say
"A slight change in behavior is the JSON being past into the FromJson is more strictly parsed. Meaning it doesn't allow extra , for the last property in an object. LitJson would allow that. Given that isn't valid JSON this seems an acceptable change for V4."

but here you are allowing trailing commas. Did you set AllowTrailingCommas to true on purpose to keep the same behavior as litJson or did you mean to set this to false?

Copy link
Member Author

@normj normj Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh yeah. I copied the comment from Alex's original PR he must have added setting that property afterwards. I'll remove that comment from the PR description.

CommentHandling = JsonCommentHandling.Skip
});

if (json.RootElement.ValueKind != JsonValueKind.Object)
{
throw new InvalidOperationException("Expected object at JSON root.");
}

var document = ToEntry(json, DynamoDBEntryConversion.V2) as Document;
var document = ToEntry(json.RootElement, DynamoDBEntryConversion.V2) as Document;
if (document == null)
throw new InvalidOperationException();

Expand All @@ -58,15 +62,15 @@ public static Document FromJson(string jsonText)
/// <returns>An <see cref="IEnumerable{T}"/> of type <see cref="Document"/></returns>
public static IEnumerable<Document> FromJsonArray(string jsonText)
{
var json = JsonMapper.ToObject(jsonText);
if (!json.IsArray)
var json = JsonDocument.Parse(jsonText);
if (json.RootElement.ValueKind != JsonValueKind.Array)
throw new InvalidOperationException("Expected array at JSON root.");

var array = new List<Document>();
for(int i=0;i<json.Count;i++)

foreach (var element in json.RootElement.EnumerateArray())
{
var item = json[i];
var entry = ToEntry(item, DynamoDBEntryConversion.V2) as Document;
var entry = ToEntry(element, DynamoDBEntryConversion.V2) as Document;
if (entry == null)
throw new InvalidOperationException();
array.Add(entry);
Expand All @@ -83,19 +87,16 @@ public static IEnumerable<Document> FromJsonArray(string jsonText)
/// <returns></returns>
public static string ToJson(Document document, bool prettyPrint)
{
var sb = new StringBuilder();
var writer = new JsonWriter(sb);
writer.PrettyPrint = prettyPrint;
using var stream = new MemoryStream();
using var writer = new Utf8JsonWriter(stream, new JsonWriterOptions
{
Indented = prettyPrint
});

WriteJson(document, writer, DynamoDBEntryConversion.V2);

// Trim everything before the first '{' character
var jsonIndex = FirstIndex(sb, '{');
if (jsonIndex > 0)
sb.Remove(0, jsonIndex);

var jsonText = sb.ToString();
return jsonText;
writer.Flush();
return Encoding.UTF8.GetString(stream.ToArray());
}


Expand Down Expand Up @@ -227,70 +228,79 @@ private static bool TryDecodeBase64(string base64Data, out byte[] bytes)
}
}

// Returns a DynamoDB entry for the given JSON data
private static DynamoDBEntry ToEntry(JsonData data, DynamoDBEntryConversion conversion)
/// <summary>
/// Returns a DynamoDB entry for the given JSON data
/// </summary>
private static DynamoDBEntry ToEntry(JsonElement data, DynamoDBEntryConversion conversion)
{
if (data == null)
if (data.ValueKind == JsonValueKind.Null)
return new DynamoDBNull();

if (data.IsObject)
if (data.ValueKind == JsonValueKind.Object)
{
var document = new Document();
foreach (var propertyName in data.PropertyNames)
foreach (var property in data.EnumerateObject())
{
var nestedData = data[propertyName];
var entry = ToEntry(nestedData, conversion);
document[propertyName] = entry;
var entry = ToEntry(property.Value, conversion);
document[property.Name] = entry;
}
return document;
}

if (data.IsArray)
if (data.ValueKind == JsonValueKind.Array)
{
var list = new DynamoDBList();
for(int i=0;i<data.Count;i++)
foreach (var property in data.EnumerateArray())
{
var item = data[i];
var entry = ToEntry(item, conversion);
var entry = ToEntry(property, conversion);
list.Add(entry);
}
return list;
}

if (data.IsBoolean)
return new UnconvertedDynamoDBEntry((bool)data).Convert(conversion);

if (data.IsDouble)
return new UnconvertedDynamoDBEntry((double)data).Convert(conversion);

if (data.IsInt)
return new UnconvertedDynamoDBEntry((int)data).Convert(conversion);
if (data.ValueKind == JsonValueKind.False)
return new UnconvertedDynamoDBEntry(false).Convert(conversion);

if (data.IsUInt)
return new UnconvertedDynamoDBEntry((uint)data).Convert(conversion);
if (data.ValueKind == JsonValueKind.True)
return new UnconvertedDynamoDBEntry(true).Convert(conversion);

if (data.IsLong)
return new UnconvertedDynamoDBEntry((long)data).Convert(conversion);

if (data.IsULong)
return new UnconvertedDynamoDBEntry((ulong)data).Convert(conversion);
if (data.ValueKind == JsonValueKind.Number)
{
if (data.TryGetDecimal(out decimal decimalValue))
return new UnconvertedDynamoDBEntry(decimalValue).Convert(conversion);
if (data.TryGetDouble(out double doubleValue))
return new UnconvertedDynamoDBEntry(doubleValue).Convert(conversion);
if (data.TryGetInt32(out int intValue))
return new UnconvertedDynamoDBEntry(intValue).Convert(conversion);
if (data.TryGetUInt32(out uint uintValue))
return new UnconvertedDynamoDBEntry(uintValue).Convert(conversion);
if (data.TryGetInt64(out long int64Value))
return new UnconvertedDynamoDBEntry(int64Value).Convert(conversion);
if (data.TryGetUInt64(out ulong uint64Value))
return new UnconvertedDynamoDBEntry(uint64Value).Convert(conversion);

// This preserves existing fallback behavior we had with LitJson
return new UnconvertedDynamoDBEntry(default(long)).Convert(conversion);
}

if (data.IsString)
return new UnconvertedDynamoDBEntry((string)data).Convert(conversion);
if (data.ValueKind == JsonValueKind.String)
return new UnconvertedDynamoDBEntry(data.ToString()).Convert(conversion);

throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture,
"Unable to convert JSON data of type {0} to DynamoDB type.", data.GetJsonType()));
"Unable to convert JSON data of type {0} with value {1} to DynamoDB type.", data.ValueKind, data.GetRawText()));
}

// Writes a JSON representation of the given DynamoDBEntry
internal static void WriteJson(DynamoDBEntry entry, JsonWriter writer, DynamoDBEntryConversion conversion)
/// <summary>
/// Writes a JSON representation of the given DynamoDBEntry
/// </summary>
internal static void WriteJson(DynamoDBEntry entry, Utf8JsonWriter writer, DynamoDBEntryConversion conversion)
{
entry = entry.ToConvertedEntry(conversion);

var document = entry as Document;
if (document != null)
{
writer.WriteObjectStart();
writer.WriteStartObject();

// Both item attributes and entries in M type are unordered, so sorting by key
// http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/DataModel.html#DataModel.DataTypes
Expand All @@ -302,7 +312,7 @@ internal static void WriteJson(DynamoDBEntry entry, JsonWriter writer, DynamoDBE
writer.WritePropertyName(name);
WriteJson(value, writer, conversion);
}
writer.WriteObjectEnd();
writer.WriteEndObject();
return;
}

Expand All @@ -320,71 +330,81 @@ internal static void WriteJson(DynamoDBEntry entry, JsonWriter writer, DynamoDBE
{
var itemType = primitiveList.Type;

writer.WriteArrayStart();
writer.WriteStartArray();
foreach (var item in primitiveList.Entries)
{
var itemValue = item.Value;
WritePrimitive(writer, itemType, itemValue);
}
writer.WriteArrayEnd();
writer.WriteEndArray();
return;
}

var ddbList = entry as DynamoDBList;
if (ddbList != null)
{
writer.WriteArrayStart();
foreach(var item in ddbList.Entries)
writer.WriteStartArray();
foreach (var item in ddbList.Entries)
{
WriteJson(item, writer, conversion);
}
writer.WriteArrayEnd();
writer.WriteEndArray();
return;
}

var ddbBool = entry as DynamoDBBool;
if (ddbBool != null)
{
writer.Write(ddbBool.Value);
writer.WriteBooleanValue(ddbBool.Value);
return;
}

var ddbNull = entry as DynamoDBNull;
if (ddbNull != null)
{
writer.Write((string)null);
writer.WriteNullValue();
return;
}

throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture,
"Unable to convert entry of type {0} to JSON", entry.GetType().FullName));
}

// Write the contents of a Primitive object as JSON data
private static void WritePrimitive(JsonWriter writer, DynamoDBEntryType type, object value)
/// <summary>
/// Write the contents of a Primitive object as JSON data
/// </summary>
private static void WritePrimitive(Utf8JsonWriter writer, DynamoDBEntryType type, object value)
{
var stringValue = value as string;

switch (type)
{
case DynamoDBEntryType.Numeric:
writer.WriteRaw(stringValue);
break;
{
#if NETCOREAPP3_1 // WriteRawValue was added in .NET 6, but we need to write out Number values without quotes
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Does this mean for the .NET Framework target we'd try to use WriteRawValue too (which is only available in .NET 6)? Or is it fine because we have System.Text.Json as a dependency for net472?

Copy link
Member

@ashovlin ashovlin Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's available in 4.7.2. At the time 3.1 was the only target we needed this workaround for.

using var document = JsonDocument.Parse(stringValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is an alternative, simpler solution here which is to use something like:

var number = Convert.ToDouble(stringValue);
writer.WriteNumberValue(number);

This way, we don't need to have a preprocessor directive. But maybe I'm missing something and there is a reason why it was done this way.😅

Copy link
Contributor

@peterrsongg peterrsongg Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this what the user was complaining about for losing decimal precision? If that's the case then nvm!

Copy link
Member Author

Choose a reason for hiding this comment

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

We would potentially lose precision trying to convert the string version of the number to the best fit .NET numeric type. Overall I don't think that will simplify the logic.

document.WriteTo(writer);
#else
writer.WriteRawValue(stringValue);
#endif
break;
}
case DynamoDBEntryType.String:
writer.Write(stringValue);
writer.WriteStringValue(stringValue);
break;
case DynamoDBEntryType.Binary:
var bytes = value as byte[];
var base64 = Convert.ToBase64String(bytes);
writer.Write(base64);
writer.WriteBase64StringValue(bytes);
break;
default:
throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture,
"Unsupport DynamoDBEntryType: {0}", type));
}
}

// Finds first instance of a character
/// <summary>
/// Finds first instance of a character
/// </summary>
private static int FirstIndex(StringBuilder sb, char toMatch)
{
for(int i=0;i<sb.Length;i++)
Expand Down
Loading