[BUG] Azure.ResourceManager.KeyVault's KeyVaultData type not usable with JSON deserialization

AtOMiCNebula commented 1 year ago

Library name and version

Azure.ResourceManager.KeyVault 1.0.0

Describe the bug

The KeyVaultData class in the Azure.ResourceManager.KeyVault SDK has a single public constructor that throws an exception if a null KeyVaultProperties instance is passed in, which is what both Newtonsoft.Json and System.Text.Json do when trying to create an instance for deserialization purposes. This effectively prevents you from deserializing either of these types from JSON.

https://github.com/Azure/azure-sdk-for-net/blob/2cbcfd0af4ffab269ea8aafc664362fc39da2464/sdk/keyvault/Azure.ResourceManager.KeyVault/src/Generated/KeyVaultData.cs#L19-L28

This doesn't appear to be the case for other types in Azure.ResourceManager.* SDKs, but I don't understand why the code generation for the KeyVault management SDK is different:

Expected behavior

Structure deserialized, no exception

Actual behavior

Exception thrown:

System.ArgumentNullException
  HResult=0x80004003
  Message=Value cannot be null. (Parameter 'properties')
  Source=Azure.ResourceManager.KeyVault
  StackTrace:
   at Azure.ResourceManager.KeyVault.KeyVaultData..ctor(AzureLocation location, KeyVaultProperties properties)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObjectUsingCreatorWithParameters(JsonReader reader, JsonObjectContract contract, JsonProperty containerProperty, ObjectConstructor`1 creator, String id)
[...]

Reproduction Steps

Minimal example:

KeyVaultData data = JsonConvert.DeserializeObject<KeyVaultData>("{}");

Thing I was actually trying to do:

ArmClient client = new(credential);
TenantCollection tenants = client.GetTenants();
await foreach (TenantResource tenant in tenants)
{
    QueryContent query = new(@"
Resources
| where type == ""microsoft.keyvault/vaults""
| order by name asc
");
    QueryResponse response = await tenant.ResourcesAsync(query);
    IList<KeyVaultData> vaultData = response.Data.ToObjectFromJson<IList<KeyVaultData>>();
}

Environment

.NET 6.0, Visual Studio 17.4.1

jsquire commented 1 year ago

Thank you for your feedback. Tagging and routing to the team member best able to assist.

xboxeer commented 1 year ago

@AtOMiCNebula this is by design, the service team has defined its swagger here so the keyvaultdata ctor method has this properties parameter

AtOMiCNebula commented 1 year ago

@xboxeer, this still seems like a poor design choice, regardless. What's the right way to engage the KeyVault team on this? Open a new issue in the azure-rest-api-specs repo?

AtOMiCNebula commented 1 year ago

Actually, this is probably a closer fit to either #25699 or #25045.

For now, I'm hacking around this with KeyVaultData with the following (the Write implementation wasn't strictly needed, but why not 😄):

public class AzureJsonConverter<T> : JsonConverter<T>
{
    public override T? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        MethodInfo? deserializer = typeof(T).GetMethod($"Deserialize{typeof(T).Name}", BindingFlags.NonPublic | BindingFlags.Static);
        if (deserializer == null)
        {
            throw new NotImplementedException();
        }

        using JsonDocument document = JsonDocument.ParseValue(ref reader);
        object? result = deserializer.Invoke(null, new object?[] { document.RootElement });
        return (T)result!;
    }

    public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options)
    {
        MethodInfo? serializer = typeof(T).GetMethod("Azure.Core.IUtf8JsonSerializable.Write", BindingFlags.NonPublic | BindingFlags.Instance);
        if (serializer == null)
        {
            throw new NotImplementedException();
        }

        serializer.Invoke(value, new object?[] { writer });
    }
}

JsonSerializerOptions options = new() { Converters = { new AzureJsonConverter<KeyVaultData>() } };
IList<KeyVaultData> vaultData = response.Data.ToObjectFromJson<IList<KeyVaultData>>(options);

Azure / azure-sdk-for-net