Skip to content

Fixed dictionary item value serialization issue - Part 2 #445

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

Merged
merged 8 commits into from
Dec 20, 2022
Merged

Fixed dictionary item value serialization issue - Part 2 #445

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3059678
Fixes after review
tjoubert Nov 30, 2022
b32f42c
Fixed tests
tjoubert Nov 30, 2022
e69c3a9
Added custom ApiClientSerializationOptions parameter to PostCreateVie…
tjoubert Dec 5, 2022
0e98898
Merge branch 'master' into feature-3.8/DE-283-PostCreateViewAsync-Lin…
tjoubert Dec 5, 2022
e43e662
Removed serializationOptions parameter from PostCreateViewAsync(). Ap…
tjoubert Dec 6, 2022
194010d
Added ignoreNullValuesOnSerialization parameter to PostCreateViewAsyn…
tjoubert Dec 6, 2022
5ae3706
Reformatted code file
tjoubert Dec 6, 2022
43cee7b
Fixed comments
tjoubert Dec 20, 2022
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
36 changes: 17 additions & 19 deletions arangodb-net-standard.Test/Serialization/JsonNetApiClientSerializationTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,7 @@ public void Serialize_ShouldNotCamelCaseBindVars_WhenSerializingPostCursorBody()

byte[] jsonBytes = serialization.Serialize(body, new ApiClientSerializationOptions(
useCamelCasePropertyNames: true,
ignoreNullValues: true,
camelCasePropertyNamesOfObjectValuesInDictionaries: false));
ignoreNullValues: true));

string jsonString = Encoding.UTF8.GetString(jsonBytes);

Expand All @@ -173,28 +172,28 @@ public void Serialize_ShouldNotCamelCaseBindVars_WhenSerializingPostCursorBody()
}

[Fact]
public void Serialize_ShouldCamelCaseBindVars_WhenSerializingPostCursorBody()
public void Serialize_ShouldCamelCaseBindVars_WhenSerializingPostCursorBodyWithDictionaryOption()
{
var body = new PostCursorBody
{
BindVars = new Dictionary<string, object>
{
["DontCamelCaseKey"] = new { DontCamelCaseMe = true }
["CamelCaseKey"] = new { CamelCaseMe = true }
}
};
var serialization = new JsonNetApiClientSerialization();

byte[] jsonBytes = serialization.Serialize(body, new ApiClientSerializationOptions(
useCamelCasePropertyNames: true,
ignoreNullValues: true,
camelCasePropertyNamesOfObjectValuesInDictionaries: true));
ignoreNullValues: true,
applySerializationOptionsToObjectValuesInDictionaries: true));

string jsonString = Encoding.UTF8.GetString(jsonBytes);

Assert.Contains("DontCamelCaseKey", jsonString);
Assert.DoesNotContain("dontCamelCaseKey", jsonString);
Assert.Contains("dontCamelCaseMe", jsonString);
Assert.DoesNotContain("DontCamelCaseMe", jsonString);
Assert.Contains("CamelCaseKey", jsonString);
Assert.DoesNotContain("camelCaseKey", jsonString);
Assert.Contains("camelCaseMe", jsonString);
Assert.DoesNotContain("CamelCaseMe", jsonString);
}

[Fact]
Expand All @@ -212,8 +211,7 @@ public void Serialize_ShouldNotCamelCaseParams_WhenSerializingPostTransactionBod

byte[] jsonBytes = serialization.Serialize(body, new ApiClientSerializationOptions(
useCamelCasePropertyNames: true,
ignoreNullValues: true,
camelCasePropertyNamesOfObjectValuesInDictionaries: false));
ignoreNullValues: true));

string jsonString = Encoding.UTF8.GetString(jsonBytes);

Expand All @@ -225,13 +223,13 @@ public void Serialize_ShouldNotCamelCaseParams_WhenSerializingPostTransactionBod


[Fact]
public void Serialize_ShouldCamelCaseParams_WhenSerializingPostTransactionBody()
public void Serialize_ShouldCamelCaseParams_WhenSerializingPostTransactionBodyWithDictionaryOption()
{
var body = new PostTransactionBody
{
Params = new Dictionary<string, object>
{
["DontCamelCaseKey"] = new { DontCamelCaseMe = true }
["CamelCaseKey"] = new { CamelCaseMe = true }
}
};

Expand All @@ -240,14 +238,14 @@ public void Serialize_ShouldCamelCaseParams_WhenSerializingPostTransactionBody()
byte[] jsonBytes = serialization.Serialize(body, new ApiClientSerializationOptions(
useCamelCasePropertyNames: true,
ignoreNullValues: true,
camelCasePropertyNamesOfObjectValuesInDictionaries: true));
applySerializationOptionsToObjectValuesInDictionaries: true));

string jsonString = Encoding.UTF8.GetString(jsonBytes);

Assert.Contains("DontCamelCaseKey", jsonString);
Assert.DoesNotContain("dontCamelCaseKey", jsonString);
Assert.Contains("dontCamelCaseMe", jsonString);
Assert.DoesNotContain("DontCamelCaseMe", jsonString);
Assert.Contains("CamelCaseKey", jsonString);
Assert.DoesNotContain("camelCaseKey", jsonString);
Assert.Contains("camelCaseMe", jsonString);
Assert.DoesNotContain("CamelCaseMe", jsonString);
}

[Fact]
Expand Down
2 changes: 1 addition & 1 deletion arangodb-net-standard/Serialization/ApiClientSerialization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public abstract class ApiClientSerialization : IApiClientSerialization
ignoreNullValues: true,
useStringEnumConversion: false,
ignoreMissingMember: true,
camelCasePropertyNamesOfObjectValuesInDictionaries: false);
applySerializationOptionsToObjectValuesInDictionaries: false);

/// <summary>
/// Deserializes the data structure contained by the specified stream
Expand Down
15 changes: 8 additions & 7 deletions arangodb-net-standard/Serialization/ApiClientSerializationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,13 @@ public class ApiClientSerializationOptions
public bool UseStringEnumConversion { get; set; }

/// <summary>
/// True to camel case the names of properties of object values
/// True to apply serialization options to object values
/// in dictionaries (i.e. CamelCaseMe => "camelCaseMe").
/// False to leave the names of properties of object values
/// in dictionaries as they are (i.e. DontCamelCaseMe => "DontCamelCaseMe")
/// False to not apply serialization options to object values
/// in dictionaries (i.e. leave the names of properties of object values
/// in dictionaries as they are: DontCamelCaseMe => "DontCamelCaseMe")
/// </summary>
public bool CamelCasePropertyNamesOfObjectValuesInDictionaries { get; set; }
public bool ApplySerializationOptionsToObjectValuesInDictionaries { get; set; }

/// <summary>
/// Create serialization options.
Expand All @@ -42,19 +43,19 @@ public class ApiClientSerializationOptions
/// <param name="ignoreNullValues">Whether null values should be ignored - i.e. not defined at all in the serialized string.</param>
/// <param name="useStringEnumConversion">Whether to serialize enum values to a string value instead of an integer.</param>
/// <param name="ignoreMissingMember">Whether missing members should be ignored.</param>
/// <param name="camelCasePropertyNamesOfObjectValuesInDictionaries">Whether to camel case the names of properties of object values in dictionaries.</param>
/// <param name="applySerializationOptionsToObjectValuesInDictionaries">Whether to apply serialization options to object values in dictionaries.</param>
public ApiClientSerializationOptions(
bool useCamelCasePropertyNames,
bool ignoreNullValues,
bool useStringEnumConversion = false,
bool ignoreMissingMember = true,
bool camelCasePropertyNamesOfObjectValuesInDictionaries = false)
bool applySerializationOptionsToObjectValuesInDictionaries = false)
{
UseCamelCasePropertyNames = useCamelCasePropertyNames;
IgnoreNullValues = ignoreNullValues;
UseStringEnumConversion = useStringEnumConversion;
IgnoreMissingMember = ignoreMissingMember;
CamelCasePropertyNamesOfObjectValuesInDictionaries = camelCasePropertyNamesOfObjectValuesInDictionaries;
ApplySerializationOptionsToObjectValuesInDictionaries = applySerializationOptionsToObjectValuesInDictionaries;
}
}
}
2 changes: 1 addition & 1 deletion arangodb-net-standard/Serialization/DictionaryValueConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public override void WriteJson(JsonWriter writer, object value, JsonSerializer s
{
// Use a local serializer for writing instead of the passed-in serializer
JsonSerializer mySerializer;
if (_serializationOptions != null && _serializationOptions.CamelCasePropertyNamesOfObjectValuesInDictionaries)
if (_serializationOptions != null && _serializationOptions.ApplySerializationOptionsToObjectValuesInDictionaries)
{
mySerializer = new JsonSerializer
{
Expand Down
5 changes: 3 additions & 2 deletions arangodb-net-standard/ViewApi/IViewsApiClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using ArangoDBNetStandard.Serialization;
using ArangoDBNetStandard.ViewApi.Models;
using System.Threading;
using System.Threading.Tasks;
Expand All @@ -24,10 +25,10 @@ Task<GetAllViewsResponse> GetAllViewsAsync(
/// POST /_api/view
/// </summary>
/// <param name="body">The body of the request containing required properties.</param>
/// <param name="serializationOptions">Custom serialization options to be used.</param>
/// <param name="token">A CancellationToken to observe while waiting for the task to complete or to cancel the task.</param>
/// <returns></returns>
Task<ViewResponse> PostCreateViewAsync(ViewDetails body,
CancellationToken token = default);
Task<ViewResponse> PostCreateViewAsync(ViewDetails body, ApiClientSerializationOptions serializationOptions = null, CancellationToken token = default);

/// <summary>
/// Delete / drop a view
Expand Down
13 changes: 8 additions & 5 deletions arangodb-net-standard/ViewApi/ViewsApiClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,18 @@ public virtual async Task<GetAllViewsResponse> GetAllViewsAsync(
/// Create a new View
/// POST /_api/view
/// </summary>
/// <param name="body">The body of the request containing required properties.</param>
/// <param name="body">The body of the request containing required properties.</param>
/// <param name="serializationOptions">Custom serialization options to be used.</param>
/// <param name="token">A CancellationToken to observe while waiting for the task to complete or to cancel the task.</param>
/// <returns></returns>
public virtual async Task<ViewResponse> PostCreateViewAsync(ViewDetails body,
CancellationToken token = default)
public virtual async Task<ViewResponse> PostCreateViewAsync(ViewDetails body, ApiClientSerializationOptions serializationOptions = null, CancellationToken token = default)
{
string uri = _apiPath;
var content = GetContent(body, new ApiClientSerializationOptions(true, true));
using (var response = await _transport.PostAsync(uri, content, token: token).ConfigureAwait(false))
var content = GetContent(body,
serializationOptions ?? new ApiClientSerializationOptions(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think serializationOptions must be exposed to callers of this API method. ArangoDB Web API accepts a JSON with camelCase, we don't want callers to change the standard JSON that the library/driver sends to ArangoDB.

Instead we should turn on the option, like so:

var content = GetContent(body, new ApiClientSerializationOptions(
    useCamelCasePropertyNames: true,
    ignoreNullValues: true,
    applySerializationOptionsToObjectValuesInDictionaries: true));

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed that you asked about this in your previous comment: #445 (comment)

I missed that.

From what I understand, the option should be hardcoded by us in the API method because we don't want to allow other options to be modified.
One gotchat is that null properties provided in LinkProperties will be ignored, because our serialization options for PostCreateViewAsync ignore null values. Is that acceptable? I don't understand enough about views and fields to know if it is gonna be a problem.

I see the following mentioned on LinksProperties.IncludeAllFields:

        /// If set to true, then process all document attributes.
        /// Otherwise, only consider attributes mentioned in fields.
        /// Attributes not explicitly specified in fields will be
        /// processed with default link properties, i.e. null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think serializationOptions must be exposed to callers of this API method. ArangoDB Web API accepts a JSON with camelCase, we don't want callers to change the standard JSON that the library/driver sends to ArangoDB.

Instead we should turn on the option, like so:

var content = GetContent(body, new ApiClientSerializationOptions(
    useCamelCasePropertyNames: true,
    ignoreNullValues: true,
    applySerializationOptionsToObjectValuesInDictionaries: true));

OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed that you asked about this in your previous comment: #445 (comment)

I missed that.

From what I understand, the option should be hardcoded by us in the API method because we don't want to allow other options to be modified. One gotchat is that null properties provided in LinkProperties will be ignored, because our serialization options for PostCreateViewAsync ignore null values. Is that acceptable? I don't understand enough about views and fields to know if it is gonna be a problem.

I see the following mentioned on LinksProperties.IncludeAllFields:

        /// If set to true, then process all document attributes.
        /// Otherwise, only consider attributes mentioned in fields.
        /// Attributes not explicitly specified in fields will be
        /// processed with default link properties, i.e. null.

If IncludeAllFields is set to true, then all document attributes are processed. The ones not specified in Fields are processed with a default value of null.

  1. When IncludeAllFields == true and ignoreNullValues == true or false, it's not a problem. null fields specified by the caller and removed by the serialization are replaced and processed as null by the server by default.
  2. When IncludeAllFields == false and ignoreNullValues == true, it could cause a problem for null attributes specified by the caller, because the caller clearly intends to process the attributes as null, but the serialization will remove/ignore it.
  3. When IncludeAllFields == false and ignoreNullValues == false, it will not cause any problem because null attributes specified by the caller will be processed as null as intended by the caller.
    So, I think it makes sense to set ignoreNullValues to false. @DiscoPYF , your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting ignoreNullValues to false, causes a lot of errors with our existing tests because several property values are not specified. I think this will break a lot of existing code that are not specifying the whole view body. For better backward compatibility, I've added a new parameter ignoreNullValuesOnSerialization which defaults to true. @DiscoPYF , your thoughts please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will take a closer look in the coming days. 👍

Copy link
Collaborator

@DiscoPYF DiscoPYF Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

At a glance, some thoughts:

What you did makes sense to me (adding a new method parameter ignoreNullValuesOnSerialization to PostCreateViewAsync and others), letting the caller decide based on what they are providing.
I'm not able to review fully right now (limited time) but will do in coming days.

Alternatives:

We can allow specifying the dictionary options individually, letting us cover all cases. For example: add a new property in ApiClientSerializationOptions:

public class ApiClientSerializationOptions
{
    /*...*/

    /// <summary>
    /// Whether dictionary values should be serialized with the same options
    /// as normal objects.
    /// </summary>
    /// <remarks>
    /// If false, you can use <see cref="DictionaryOptions"/>
    /// to control individual options for dictionary serialization.
    /// </remarks>
    public bool ApplySerializationOptionsToDictionaryValues { get; set; }

    /// <summary>
    /// Serialization options to apply specifically to dictionary values.
    /// </summary>
    /// <remarks>
    /// If <see cref="ApplySerializationOptionsToDictionaryValues"/> is true,
    /// this property is ignored.
    /// If <see cref="ApplySerializationOptionsToDictionaryValues"/> is false and
    /// <see cref="DictionaryOptions"/> is null, then dictionary values are serialized
    /// as provided (untouched).
    /// </remarks>
    public ApiClientSerializationOptions DictionaryOptions { get; set; }
}

Perhaps create a new type for dictionary options:

public interface ISerializationOptions // For convenience to have a source of truth for available options.
{
    public bool UseCamelCasePropertyNames { get; set; }
    public bool IgnoreNullValues { get; set; }
    public bool IgnoreMissingMember { get; set; }
    public bool UseStringEnumConversion { get; set; }
}

public class ApiClientSerializationOptions : ISerializationOptions
{
    /*...*/
    public bool ApplySerializationOptionsToDictionaryValues { get; set; }
    public DictionaryValuesSerializationOptions DictionaryOptions { get; set; }
}

public class DictionaryValuesSerializationOptions : ISerializationOptions
{
    /*...*/
}

Maybe ApplySerializationOptionsToDictionaryValues can be considered redundant then. It would be more of a convenience for any consumer that want to align dictionary serialization without having to specify all options again.

With this new property, we (as the library) can do the following inside PostCreateViewAsync:

var content = GetContent(body, new ApiClientSerializationOptions(
    useCamelCasePropertyNames: true,
    ignoreNullValues: true,
    DictionaryOptions: new DictionaryValuesSerializationOptions()
    {
        useCamelCasePropertyNames: true,
        ignoreNullValues: IncludeAllFields // not sure if it makes sense
    }));

I don't know if this also covers advanced scenarios such as nesting of links, e.g. what happens when someone sends:

new ViewDetails()
{
    Links = new Dictionary<string, LinksProperties>()
    {
        ["Files"] = new LinksProperties()
        {
            Fields = Dictionary<string, LinksProperties>()
            {
                ["Properties"] = new LinksProperties()
                {
                    /*...*/
                }
            }
        }
    }
}

Not sure if this is even valid, I don't know enough about the view feature. Perhaps best to leave it to the caller like you did.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had another look and this approach could be used together with the parameter ignoreNullValuesOnSerialization so that it is only applied to links and fields. We could rename this parameter and do something like:

public virtual async Task<ViewResponse> PostCreateViewAsync(
    ViewDetails body,
    bool ignoreNullLinksAndFields = false,
    CancellationToken token = default)
{
    var content = GetContent(body, new ApiClientSerializationOptions(
        useCamelCasePropertyNames: true,
        ignoreNullValues: true,
        DictionaryOptions = new DictionaryValuesSerializationOptions
        {
            useCamelCasePropertyNames: true,
            ignoreNullValues: ignoreNullLinksAndFields
        }));
}

Not blocker, just a thought.

useCamelCasePropertyNames: true,
ignoreNullValues: true));
using (var response = await _transport.PostAsync(uri, content).ConfigureAwait(false))
{
if (response.IsSuccessStatusCode)
{
Expand Down