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

Add BinaryData.ToDynamicFromJson() #53642

Closed
terrajobst opened this issue Jun 2, 2021 · 18 comments
Closed

Add BinaryData.ToDynamicFromJson() #53642

terrajobst opened this issue Jun 2, 2021 · 18 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Memory
Milestone

Comments

@terrajobst
Copy link
Member

terrajobst commented Jun 2, 2021

Background and Motivation

We designed BinaryData as a simple one-stop shop converting data from and to binary data. This type is especially useful for the Azure SDK where many technologies store blobs of data but the consumer typically uses strings or JSON encoded data.

We've recently added JsonNode for .NET 6 which includes the ability to treat a JSON object as dynamic so that one can "dot" into the type without having to use the indexer syntax. The Azure SDK has asked me whether we could add a convenience method to BinaryData to directly convert a blob to a dynamic object.

Proposed API

namespace System
{
    public class BinaryData
    {
        public dynamic ToDynamicFromJson(JsonSerializerOptions? options = null);
    }
}

Usage Examples

Response response = client.SomeMethod("s1", 5, RequestContent.Create(model));
dynamic result = response.Content.ToDynamicFromJson();
Console.WriteLine(result.Baz);

Alternative Designs

The Azure SDK could define their own extension method, but that feels wrong.

Risks

None; BinaryData already depends on System.Text.Json. This new API doesn't change the dependency matrix.

/cc @steveharter @layomia @KrzysztofCwalina

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json labels Jun 2, 2021
@ghost
Copy link

ghost commented Jun 2, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

We designed BinaryData as a simple one-stop shop converting data from and to binary data. This type is especially useful for the Azure SDK where many technologies store blobs of data but the consumer typically uses strings or JSON encoded data.

We've recently added JsonNode for .NET 6 which includes the ability to treat a JSON object as dynamic so that one can "dot" into the type without having to use the indexer syntax. The Azure SDK has asked me whether we could add a convenience method to BinaryData to directly convert a blob to a dynamic object.

Proposed API

namespace System
{
    public class BinaryData
    {
        public T ToDynamicFromJson<T>(JsonSerializerOptions? options = null);
    }
}

Usage Examples

Response response = client.SomeMethod("s1", 5, RequestContent.Create(model));
dynamic result = response.Content.ToDynamicFromJson();
Console.WriteLine(result.Baz);

Alternative Designs

The Azure SDK could define their own extension method, but that feels wrong.

Risks

None; BinaryData already depends on System.Text.Json. This new API doesn't change the dependency matrix.

/cc @steveharter @layomia @KrzysztofCwalina

Author: terrajobst
Assignees: -
Labels:

api-suggestion, area-System.Text.Json

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 2, 2021
@terrajobst terrajobst added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 2, 2021
@terrajobst terrajobst added this to the 6.0.0 milestone Jun 2, 2021
@stephentoub
Copy link
Member

stephentoub commented Jun 2, 2021

public T ToDynamicFromJson<T>(JsonSerializerOptions? options = null);

What is the T here?

@stephentoub
Copy link
Member

This would also depend on a specific answer to the questions asked in #53195, yes?

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Jun 2, 2021

What is the T here?

It should return dynamic, i.e.

public dynamic ToDynamicFromJson() {
    dynamic result = JsonNode.Parse(this.ToMemory());
    return result;
}

@stephentoub
Copy link
Member

public dynamic ToDynamicFromJson<T>(JsonSerializerOptions? options = null) {

And the <T>? Is that just a typo?

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Jun 2, 2021

And the <T>? Is that just a typo?

fixed above (i.e. removed T)

I also removed the options. They are not used by JsonNode.Parse. We could add overloads to take the JsonNode.Parse options, but I don't think any of these make sense on when reading JSON.

These are the JsonNode.Parse options:

public struct JsonNodeOptions {
    public bool PropertyNameCaseInsensitive { get; set; }
}

public struct JsonDocumentOptions {
    public bool AllowTrailingCommas { get; set; }
    public JsonCommentHandling CommentHandling { get; set; }
    public int MaxDepth { get; set; }
}

@terrajobst
Copy link
Member Author

terrajobst commented Jun 2, 2021

@stephentoub

public T ToDynamicFromJson<T>(JsonSerializerOptions? options = null);

What is the T here?

That's just me being dumb

@terrajobst
Copy link
Member Author

@KrzysztofCwalina

fixed above (i.e. removed T)

I think you forgot to hit save :-)

I also removed the options. They are not used by JsonNode.Parse. We could add overloads to take the JsonNode.Parse options, but I don't think any of these make sense on when reading JSON.

Hmm.... I thought we'd need at least the ability to change naming conventions but you probably also want support for converters and reference handling, no?

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Jun 2, 2021

I think you forgot to hit save :-)

I just fixed my sample, not yours :-)

I thought we'd need at least the ability to change naming conventions but you probably also want support for converters and reference handling, no?

I might be misunderstanding what the options do. I dont see how they support converters. Your API proposal takes serializer options, but the implementation of this method will use JsonNode.Parse, which does not take serializer options.

@terrajobst
Copy link
Member Author

terrajobst commented Jun 3, 2021

I might be misunderstanding what the options do. I dont see how they support converters. Your API proposal takes serializer options, but the implementation of this method will use JsonNode.Parse, which does not take serializer options.

Ah, I was misremembering. I thought JsonNode.Parse() would take JsonSerializerOptions, but you're correct -- it does not. In that case I propose to change the method to match JsonNode.Parse() although the nullable annotations feel a bit odd.

namespace System
{
    public class BinaryData
    {
        public dynamic ToDynamicFromJson(JsonNodeOptions? nodeOptions = default, JsonDocumentOptions documentOptions = default);
    }
}

Note: both @steveharter and @layomia are currently out, but they would have to take a look at this.

@terrajobst terrajobst removed the untriaged New issue has not been triaged by the area owner label Jun 21, 2021
@steveharter
Copy link
Member

Per discussion in #53195 about removing support for "dynamic", this API issue may be put on hold pending API discussion.

@terrajobst terrajobst modified the milestones: 6.0.0, Future Jun 29, 2021
@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 29, 2021
@terrajobst
Copy link
Member Author

Given that it's unclear whether or we're supporting dynamic from JSON it's unclear whether this API would make sense, so I moved it to Future as a suggestion.

@Rayzbam
Copy link

Rayzbam commented Jul 22, 2022

I don't really understand how it could be "unclear".
When you're communicating between two services or 2 microservices which don't have any clue of what version is deployed for both
Example : When a consumer wants to be resilient on all properties passed in a JSON, dynamic is a good option. Trying to deserialize it in a custom object might be hard and won't allow any mistake, whereas dynamic/ExpandoObject will.

Imo, it's a must have for some architectures which need to be resilient and use dynamic to.
Tbf, i have some issue to send an object in BinaryData and deserialize it in an ExpandoObject. I mean the BinaryData just serialized my object but won't allow to deserialize it in a dynamic. seems a bit weird.

@terrajobst
Copy link
Member Author

terrajobst commented Jul 25, 2022

I don't really understand how it could be "unclear".

The concern is that dynamic is a very heavyweight API. For most cases, especially JSON, this is completely overkill.

@ghost
Copy link

ghost commented Sep 2, 2022

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

We designed BinaryData as a simple one-stop shop converting data from and to binary data. This type is especially useful for the Azure SDK where many technologies store blobs of data but the consumer typically uses strings or JSON encoded data.

We've recently added JsonNode for .NET 6 which includes the ability to treat a JSON object as dynamic so that one can "dot" into the type without having to use the indexer syntax. The Azure SDK has asked me whether we could add a convenience method to BinaryData to directly convert a blob to a dynamic object.

Proposed API

namespace System
{
    public class BinaryData
    {
        public dynamic ToDynamicFromJson(JsonSerializerOptions? options = null);
    }
}

Usage Examples

Response response = client.SomeMethod("s1", 5, RequestContent.Create(model));
dynamic result = response.Content.ToDynamicFromJson();
Console.WriteLine(result.Baz);

Alternative Designs

The Azure SDK could define their own extension method, but that feels wrong.

Risks

None; BinaryData already depends on System.Text.Json. This new API doesn't change the dependency matrix.

/cc @steveharter @layomia @KrzysztofCwalina

Author: terrajobst
Assignees: -
Labels:

api-suggestion, area-System.Memory, area-System.Text.Json

Milestone: Future

@KrzysztofCwalina
Copy link
Member

The concern is that dynamic is a very heavyweight API. For most cases, especially JSON, this is completely overkill.

@jaredpar, any possibility of getting the "lightweight dynamic" feature in the next version of C#? i.e. a specially attributed indexer (string to dynamic map) that the compiler would call when dynamic properties are accessed.

@jaredpar
Copy link
Member

jaredpar commented Sep 2, 2022

@jaredpar, any possibility of getting the "lightweight dynamic" feature in the next version of C#? i.e. a specially attributed indexer (string to dynamic map) that the compiler would call when dynamic properties are accessed.

There is no concrete design for the said feature, only a series of discussion points. There is not universal agreement between the interested parties on exactly what properties the "lightweight dynamic" feature should have.

@adamsitnik
Copy link
Member

With our main focus on cloud native, where we compete mostly against languages that are AOTed (not JITted) I don't see us investing more in dynamic based APIs in System.Memory.

The Azure SDK could define their own extension method, but that feels wrong.

This sounds like a reasonable workaround.

any possibility of getting the "lightweight dynamic" feature in the next version of C#

If it was made AOT-friendly at the same time, it sounds like the proper solution that should be pursued.

@adamsitnik adamsitnik closed this as not planned Won't fix, can't repro, duplicate, stale Oct 20, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Memory
Projects
None yet
Development

No branches or pull requests

8 participants