Handle Json ExtensionData in sql translation visitor

[onionhammer]

Describe the bug
Given an object like this:

class MyEntity
{
    public string MyTypedProperty { get; set; }

    [JsonExtensionData]
    public Dictionary<string, System.Text.Json.JsonElement>? ExtensionData { get; set; }
}

Querying against an entity with extensiondata is not possible within a LINQ expression

{l => (Convert(l.ExtensionData.get_Item("MyTest"), Object) == Convert(100, Object))}

Should convert to:

WHERE root["MyTest"] == 100

Instead converts to

WHERE root["ExtensionData"]["MyTest"] == 100

The underlying entity in Cosmos will serialize like this;

{
    "MyTest": 100
}

So it is impossible to query for the "MyTest" field without dropping down to writing raw QueryDefinitions, in which case you do not have the benefit of strongly-typed expressions for the rest of your query

[onionhammer]

Workaround: have your type extend IReadOnlyDictionary<>, however then you would have to provide your own custom JSON serialization/deserialization for this type in order to still serialize the additional (non-extension data) properties

[onionhammer]

Note: I would be happy to fix this, but I already have 3 open pull requests on this repository that have not been looked at. Is it worth my time to try to contribute?

[neildsh]

Hello @onionhammer my apologies that the ball was dropped on your other PRs. I will ping the people who were reviewing those PRs, and we will try to get them merged. We can take this particular issue forward once the other PRs have been resolved. Please note that the other PRs will likely not be merged before next week. Thanks.

[onionhammer]

@neildsh Any progress on this front? Again, I would definitely be willing to take a crack at this if it means getting it done sooner, but this PR approval process is pretty arduous.

[onionhammer]

@neildsh @adityasa @leminh98 Again happy to look into contributing this if I'll see traction on the PR reviews..

[adityasa]

Hey @onionhammer, I am taking a look at this item. I have looked and approved 2 of your other prs.

[onionhammer]

@adityasa ok, let me know if you want any help/tests written

[adityasa]

@onionhammer are you talking about Newtonsoft's JsonExtensionData attribute or System.Text.Json.Serialization.JsonExtensiondata?

[adityasa]

@onionhammer if you are open to making this change, we will be happy to review it. Let me know.

[onionhammer]

@onionhammer are you talking about Newtonsoft's JsonExtensionData attribute or System.Text.Json.Serialization.JsonExtensiondata?

Both should be handled.

I will look into how much effort it will be to implement later this week or early next week.

[adityasa]

@onionhammer fyi test below baselines current behavior which shows that Newtonsoft property is already honored:
#3830

[onionhammer]

@adityasa so could the code just not ignore the STJ attribute? Would that be the change?

[adityasa]

@onionhammer, I am not sure if it's that straightforward. The sdk uses Newtonsoft.JSON for serialization/deserialization between client and the backend. Newtonsoft library doesn't understand/honor STJ attributes. As a result, this logic needs to be custom implemented on top of existing logic, with due back-compat considerations.

[onionhammer]

@adityasa OK I misunderstood what you meant previously. The functionality proposed in this issue doesnt work for either STJ or Newtonsoft attributes - right? I'm not proposing any changes to the way serialization of objects is done, only the behavior of generating cosmos SQL from LINQ.

This would be a breaking change for users who are using Newtonsoft document serializer, but also decorating their classes with STJ [ExtensionData] attribute.

If we want to avoid this breaking change, we will need to add back the ability to utilize the custom serializer during LINQ -> SQL generation; something that needs to be done anyways.., but I would argue is beyond the scope of this issue (a pre-requisite to this issue, perhaps?)

How do you want to proceed?

[onionhammer]

@adityasa After looking into this, supporting this is trivial - just a slight modification to GetMemberName and VisitMemberAccess should be needed.

Supporting STJ is still necessary, but support could be split into a separate PR.

Here's a patch you can try https://gist.github.com/onionhammer/24f6f148324a4ecae6cc1bb8a89ea9a8

[onionhammer]

And then there were 4 (open PRs)