TypeBinder.Deserialize<T> should have new() constraint

[Arithmomaniac]

azure-sdk-for-net/sdk/core/Azure.Core/src/Shared/TypeBinder.cs

Lines 131 to 143 in 07bb13a

	public T Deserialize<T>(TExchange source)
	{
	if (_isPrimitive)
	{
	if (!_binderImplementation.TryGet(null, source, out T result))
	{
	throw new InvalidOperationException($"Unable to deserialize into a primitive type {typeof(T)}");
	}
	return result;
	}
	T o = Activator.CreateInstance<T>();

This method should have a new() constraint, as it can only handle primitive types or types with a parameterless constructor. Propagating the resulting new() constraints s up the call chain (such as to Azure.Monitor.Query.LogsQueryClient.QueryWorkspaceAsync<T>) would prevent runtime errors that could be caught at compile time instead.

[jsquire]

Thank you for your feedback. We'll add this to the Azure.Core backlog for consideration.

[jsquire]

@AlexanderSher: Would you please take point for understanding the end-to-end scenario and making a recommendation on whether we should make this change?

[AlexanderSher]

Hi @Arithmomaniac

Adding new() constraint to the TypeBinder.Deserialize<T> would result in adding the same constraint to several public methods in the released libraries (e.g.: Azure.Data.Tables.TableClient.QueryAsync`), which would be a source breaking change for any code that has passes generic argument down the call stack.

@jsquire, @KrzysztofCwalina @tg-msft - do we allow this kind of source breaking change?

[jsquire]

@jsquire, @KrzysztofCwalina @tg-msft - do we allow this kind of source breaking change?

Not outside of special circumstances such as security issues. If this feature would introduce breaking changes to the public API for stable packages, then this does not look like a change that we would make. Unless I'm overlooking something, the benefit of this change would be convenience (better runtime safety) - which would not outweigh the breaking change, in my opinion.

[Arithmomaniac]

I figured this was probably the case. In which case, I would propose making an "audit" of upstream APIs and check which ones do and do not declare new() themselves. (I wouldn't mind putting this together). Then maybe do some combination of the following?

1. Add documentation remarks (exception XML doc and general documentation) to all such members
2. Have an internal Roslyn analyzer that checks whether this improper dependency is taken (errors could be suppressed for existing APIs, but new APIs wouldn't fall into this trap)
3. Add the new() constraint whenever the API change is breaking (or when a new API supersedes the old one) anyways
4. Adding fallback JSON-based deserialization for classes without parameterless constructors in the affected APIs. (For example, in LogsQueryResult, each row is a JSON array; in case of e.g. record Foo(int A, string B) and value [1, "2"], use JsonSerializer.Deserialize<Foo>("{\"A\": 1, \"B\": \"2\"}"))

[Arithmomaniac]

I noticed that TypeBinder is an "opt-in compilation" file, and is only enabled in the following SDK projects:

• Azure.Monitor.Query.csproj
• Azure.Data.Tables.csproj
• Microsoft.Azure.WebJobs.Extensions.Tables.csproj

And the Deserialize method only is used on the following paths:

• Azure.Monitor.Query.RowBinder.BindResults
• Microsoft.Azure.WebJobs.Extensions.Tables.TableEntityToPocoConverter.Convert
• Azure.Data.Tables.DictionaryTableExtensions.ToTableEntity
  • Azure.Data.Tables.DictionaryTableExtensions.ToTableEntityList
    • Azure.Data.Tables.TableClient.Query(Async)
  • Azure.Data.Tables.TableClient.GetEntityInternalAsync
    • Azure.Data.Tables.TableClient.GetEntity(IfExists)(Async)

Given its small footprint, I think we should be able to at least do 1) and/or 4).