-
Notifications
You must be signed in to change notification settings - Fork 279
Introduce new metadata provider initialize method #2101
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
Conversation
… rusamant/trying_new_converter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments. Will try to test out locally and will give more feedback if applicable.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me. Will approve after these 3 comments + pending comments from vaibhav and Sean are addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. the other test cases will be added as part of a follow up pr.
src/Core/Services/MetadataProviders/Converters/ObjectConverter.cs
Outdated
Show resolved
Hide resolved
src/Service.Tests/Unittests/SerializationDeserializationTests.cs
Outdated
Show resolved
Hide resolved
src/Service.Tests/Unittests/SerializationDeserializationTests.cs
Outdated
Show resolved
Hide resolved
src/Service.Tests/Unittests/SerializationDeserializationTests.cs
Outdated
Show resolved
Hide resolved
src/Service.Tests/Unittests/SerializationDeserializationTests.cs
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
… rusamant/trying_new_converter
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing feedback!
Why make this change?
GraphQL engine supports multi-tenancy (handles call for different tenants/artifacts in the same process). In Graphql workload, we are using dab as a nuget and every time we get a query request from a tenant, we use the config file of that tenant to set up the dab classes to respond to that particular request.
In each request we call metadataprovider.initializeAsync() which is an expensive call and adds overhead in graphql workload for query execution.
What is this change?
This code adds a new initialize method which takes in the necessary objects which are required for query execution. GraphQL workload will now call this new method in its query flow.
We also have added different converters to serialize and deserialize the different objects that get set, as Graphql workload will be storing these values in serialized format on its end.
Use of System.Text.Json serializer and deserializer
I am using System.Text.Json serializer- deserializer as compared to NewtonSoft, as System.Text.Json focuses primarily on performance, security and standards compliance. Migrate from Newtonsoft.Json to System.Text.Json
System.Text.Json doesnot support TypeNameHandling.All, that helps in handling abstract classes and its objects serialization and deserialization. Adding TypeName property on DatabaseObject level to differentiate different types of DatabaseObject as Dictionary EntityToDatabaseObject consists of different DatabaseObject objects : DatabaseTable, DatabaseView etc and eventually we need to deserialize into DatabaseObject.
System.Test.Json does not support direct serialization and deserialization of System.Type, one needs to add a customer converter to support this. Currently DatabaseObject contains SourceDefinition which contains ColumnDefinition, ColumnDefinition has property of type System.Type hence need added TypeConveter to explicitly handle serialization and deserialization.

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/docs/ThreatModel.md#mitigation-3
Similarly for object type - ColumnDefinition property Default Value I had to explicitly create an converter. Lets say there is a default value of type string is set of an column: firstname. The default value set is John. If I do not add an explicit deserialization then instead of "('John')" in original EntityDatabaseObject the deserialized value was looking as ValueKind = String : "('John')" which threw exeception.
How did I solve my initial infinite loop issue?
I was passing wrong element in line "DatabaseObject)JsonSerializer.Deserialize(document, concreteType, options)!;" of converter. I was passing root. Hence on correcting the object being passed it solved the infinite loop. My earlier issues discussed in first run, were not due to lack of converter, but were related to not correct object being passed, some properties on the Database have private set, hence during deserialization they were not getting set.( solution add : JsonInclude tag on property, https://learn.microsoft.com/en-us/dotnet/api/system.text.json.serialization.jsonincludeattribute?view=net-8.0)
How was this tested?
manually running local instance of DAB :