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

[Bug][Serialization][AdditionalProperties Phase 1] Exception will be thrown when the model accepts AdditionalProperties with a value type of non-BinaryData #4128

Closed
Tracked by #4465
ArcturusZhang opened this issue Jan 13, 2024 · 1 comment · Fixed by #4510
Assignees
Labels
DPG/RLC v2.1 Post Gallium work DPG Mgmt This issue is related to a management-plane library. v3 Version 3 of AutoRest C# generator.

Comments

@ArcturusZhang
Copy link
Member

ArcturusZhang commented Jan 13, 2024

when a model defined with string typed additional properties property, we have the following deserialization code:

internal static ExtendsStringAdditionalProperties DeserializeExtendsStringAdditionalProperties(JsonElement element, ModelReaderWriterOptions options = null)
{
    options ??= new ModelReaderWriterOptions("W");

    if (element.ValueKind == JsonValueKind.Null)
    {
        return null;
    }
    string name = default;
    IDictionary<string, string> additionalProperties = default;
    Dictionary<string, string> additionalPropertiesDictionary = new Dictionary<string, string>();
    foreach (var property in element.EnumerateObject())
    {
        if (property.NameEquals("name"u8))
        {
            name = property.Value.GetString();
            continue;
        }
        additionalPropertiesDictionary.Add(property.Name, property.Value.GetString());
    }
    additionalProperties = additionalPropertiesDictionary;
    return new ExtendsStringAdditionalProperties(name, additionalProperties);
}

if we use this json:

{
	"name": "the name",
	"addtional1": "1",
	"addtional2": 2
}

as input to deserialize the model, we will get a JsonException on this line:

                additionalPropertiesDictionary.Add(property.Name, property.Value.GetString());

when we try to consume "addtional2" property in the json (because it is a number).

Furthermore, the public serialization feature requires the models to be roundtrip when serializing and deserializing using the J format, in this case, it will never be roundtrip because no property or field could take the number value additional property additional2.

@ArcturusZhang ArcturusZhang added the v3 Version 3 of AutoRest C# generator. label Jan 13, 2024
@ArcturusZhang ArcturusZhang self-assigned this Jan 13, 2024
@ArcturusZhang ArcturusZhang added Mgmt This issue is related to a management-plane library. DPG labels Jan 13, 2024
@ArcturusZhang
Copy link
Member Author

Proposal:

  1. we should have a value kind check when the additional properties property only accept a specific type.
  2. We should always have the serializedAdditionalRawData field when the value type of public additional properties property is not BinaryData or object

@ArcturusZhang ArcturusZhang added the DPG/RLC v2.1 Post Gallium work label Jan 23, 2024
@ArcturusZhang ArcturusZhang changed the title [Bug][Serialization] Exception will be thrown when the model accepts AdditionalProperties with a value type of non-BinaryData [Bug][Serialization][AdditionalProperties Phase 1] Exception will be thrown when the model accepts AdditionalProperties with a value type of non-BinaryData Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DPG/RLC v2.1 Post Gallium work DPG Mgmt This issue is related to a management-plane library. v3 Version 3 of AutoRest C# generator.
Projects
None yet
1 participant