search

Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.19k stars 4.55k forks source link

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

Open Arithmomaniac opened 7 months ago

Arithmomaniac commented 7 months ago

https://github.com/Azure/azure-sdk-for-net/blob/07bb13aeed0904a284808ab826923446360aca7c/sdk/core/Azure.Core/src/Shared/TypeBinder.cs#L131-L143

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 commented 7 months ago

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

jsquire commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 5 months ago

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

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

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