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 DynamicJson type #12843

Merged
merged 20 commits into from
Jun 22, 2020
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -160,4 +160,7 @@ dist/
node_modules/

# MSBuild binary log files
msbuild.binlog
msbuild.binlog

# BenchmarkDotNet
BenchmarkDotNet.Artifacts
2 changes: 2 additions & 0 deletions eng/Packages.Data.props
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@
<!-- Test only packages -->
<ItemGroup Condition="('$(IsTestProject)' == 'true') OR ('$(IsTestSupportProject)' == 'true')">

<PackageReference Update="Microsoft.CSharp" Version="4.6" />

<!-- Extension packages -->
<PackageReference Update="Microsoft.AspNetCore.Server.Kestrel" Version="2.2.0" />
<PackageReference Update="Microsoft.AspNetCore.Server.WebListener" Version="1.0.2" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,62 @@ public readonly partial struct BinaryData
public override string ToString() { throw null; }
public string ToString(System.Text.Encoding encoding) { throw null; }
}
public partial class DynamicJson : System.Dynamic.IDynamicMetaObjectProvider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also implement IDictionary sooner than later because I think it'll help force some of the naming choices below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you feel strongly about having IDictionary and why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's such a well known, easy to use thing. This is getting to be a pretty hefty API at this point and if I'm trying to get a quick job done, seeing IDictionary may let me shortcut all the rest of it. All of https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.idictionary-2?view=netcore-3.1#extension-methods as well.

Copy link
Contributor Author

@pakrym pakrym Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem with implementing IDictionary<string, DJ> is that I would also need to implement IList if the current value is an array. Then we get 2 IEnumerable interfaces on the same object and that's a bit weird.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're jamming an object and an array and primitives into the same type. It's already weird. 😄

{
public DynamicJson(string json) { }
public DynamicJson(System.Text.Json.JsonElement element) { }
public Azure.Core.DynamicJson this[int arrayIndex] { get { throw null; } set { } }
public Azure.Core.DynamicJson this[string propertyName] { get { throw null; } set { } }
public static Azure.Core.DynamicJson Array() { throw null; }
public static Azure.Core.DynamicJson Array(params Azure.Core.DynamicJson[] values) { throw null; }
public static Azure.Core.DynamicJson Array(System.Collections.Generic.IEnumerable<Azure.Core.DynamicJson> values) { throw null; }
public static Azure.Core.DynamicJson Create(System.Text.Json.JsonElement element) { throw null; }
public System.Threading.Tasks.Task<T> DeserializeAsync<T>(Azure.Core.ObjectSerializer serializer) { throw null; }
public T Deserialize<T>(Azure.Core.ObjectSerializer serializer) { throw null; }
public T Deserialize<T>(System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
public System.Collections.Generic.IEnumerable<Azure.Core.DynamicJson> EnumerateArray() { throw null; }
public System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, Azure.Core.DynamicJson>> EnumerateObject() { throw null; }
public int GetArrayLength() { throw null; }
public bool GetBoolean() { throw null; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this convert the current value to a boolean? As might be a better prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't really expecting that. When I see this API, I wonder why we don't just add a couple of extension methods to JsonElement like Serialize<T>, AsDynamic, etc. I was thinking this type would be much smaller and focused exclusively on objects to avoid all these other accessors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type can't be focused just on objects, services like digitaltwins support taking arbitrary JSON so the root can be an object or an array maybe even a primitive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And maybe we're trying to solve too much here? Maybe we should tell folks if you want to use primitives or arrays with DigitalTwins that you need to work with raw JSON elements. I don't think I'd want to use this type as it exists right now for Search because it's way too general and I think would confuse what's already a tricky part of the Search API.

public double GetDouble() { throw null; }
public float GetFloat() { throw null; }
public int GetIn32() { throw null; }
public long GetLong() { throw null; }
public Azure.Core.DynamicJson GetProperty(string name) { throw null; }
public string? GetString() { throw null; }
public static Azure.Core.DynamicJson Object() { throw null; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the signature, I don't really know what this API does. Should this just be Create()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Any idea how to name the one that creates an array?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create vs CreateArray?

public static Azure.Core.DynamicJson Object(System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, Azure.Core.DynamicJson>> values) { throw null; }
public static explicit operator bool (Azure.Core.DynamicJson json) { throw null; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love a virtual I could override with domain specific conversion logic. We'd probably need that for Search.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would also require us to have a factory func so the child DynamicJson objects we create are of the right type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and I'd like a virtual one of those as well please. SearchDocument turns nested objects into more SearchDocuments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you get an object nested into array via SearchDocument?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do that when parsing JSON. But if you just set a property, we leave your object type and let the JSON serializer deal with it when turning it back into JSON.

public static explicit operator double (Azure.Core.DynamicJson json) { throw null; }
public static explicit operator int (Azure.Core.DynamicJson json) { throw null; }
public static explicit operator long (Azure.Core.DynamicJson json) { throw null; }
public static explicit operator bool? (Azure.Core.DynamicJson json) { throw null; }
public static explicit operator double? (Azure.Core.DynamicJson json) { throw null; }
public static explicit operator int? (Azure.Core.DynamicJson json) { throw null; }
public static explicit operator long? (Azure.Core.DynamicJson json) { throw null; }
public static explicit operator float? (Azure.Core.DynamicJson json) { throw null; }
public static explicit operator float (Azure.Core.DynamicJson json) { throw null; }
public static explicit operator string (Azure.Core.DynamicJson json) { throw null; }
public static implicit operator Azure.Core.DynamicJson (bool value) { throw null; }
public static implicit operator Azure.Core.DynamicJson (double value) { throw null; }
public static implicit operator Azure.Core.DynamicJson (int value) { throw null; }
public static implicit operator Azure.Core.DynamicJson (long value) { throw null; }
public static implicit operator Azure.Core.DynamicJson (bool? value) { throw null; }
public static implicit operator Azure.Core.DynamicJson (double? value) { throw null; }
public static implicit operator Azure.Core.DynamicJson (int? value) { throw null; }
public static implicit operator Azure.Core.DynamicJson (long? value) { throw null; }
public static implicit operator Azure.Core.DynamicJson (float? value) { throw null; }
public static implicit operator Azure.Core.DynamicJson (float value) { throw null; }
public static implicit operator Azure.Core.DynamicJson (string? value) { throw null; }
public static Azure.Core.DynamicJson Parse(string json) { throw null; }
public static System.Threading.Tasks.Task<Azure.Core.DynamicJson> SerializeAsync<T>(T value, Azure.Core.ObjectSerializer serializer) { throw null; }
public static Azure.Core.DynamicJson Serialize<T>(T value, Azure.Core.ObjectSerializer serializer) { throw null; }
public static Azure.Core.DynamicJson Serialize<T>(T value, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
System.Dynamic.DynamicMetaObject System.Dynamic.IDynamicMetaObjectProvider.GetMetaObject(System.Linq.Expressions.Expression parameter) { throw null; }
public System.Text.Json.JsonElement ToJsonElement() { throw null; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would Serialize<object>() do the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand. Serialize is a static method that takes an object and turns it into DJ.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, should have said Deserialize. It feels like this is a short cut for calling Serialize<object>(this).

public override string ToString() { throw null; }
public void WriteTo(System.Text.Json.Utf8JsonWriter writer) { }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not feeling strong about it. I was trying to have a lot of interop with S.T.J.

}
public partial class JsonObjectSerializer : Azure.Core.ObjectSerializer
{
public JsonObjectSerializer() { }
Expand Down
Loading