-
Notifications
You must be signed in to change notification settings - Fork 357
OData Writer API and abstraction brainstorming #3260
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
base: main
Are you sure you want to change the base?
Conversation
|
||
Note: `$metadata` currently still defaults to XML. | ||
|
||
* If customers implement their own `JsonConverter` to customize serialization for specific types, they would also need to manually handle support for `$select`, `$expand`, annotations, dynamic properties, etc. This includes knowing which annotations to write and where. If they already have generic `JsonConverters`, they likely won't work out-of-the-box. They would need to write OData-specific `JsonConverters`. |
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.
I agree with your point here, it's super important that we try to abstract the odata details from callers. I think we also need to recognize that customers will need to know these things if they wish to customize certain behaviors. Again, though, I agree that the converter is not the layer where they should be expected to do those things correctly just to get some custom behavior.
interface IODataMessageWriter<TContext, TState> | ||
{ | ||
IODataWriter<TContext, TState, TValue> GetWriter<TValue>(ODataPayloadKind payloadKind); | ||
// Alternatively we could have different `GetWriter` methods for different |
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.
please do this; i think avoiding enums is really helpful for us to have visibility into breaking changes
.WithJsonOptions(jsonSerializationOptions) | ||
.WithODataUri(odataUri) | ||
.ConfigureResourceWriters(provider => provider.Add<Customer, CustomerResourceWriter>()) | ||
.ForCollectionOf<Customer>() |
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.
Let's remember that AGS wants to use our readers and writers, and doesn't have CLR types for the types represented in the model
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.
In the case of AGS they would inject custom writers for their input types JArray
, JObject
or the input stream. The build pattern here is to provide a discoverable way to configure the service, so there could be helper methods for different common use cases (including when it's not an IEnumerable<T>
), while also providing where to define the type directly for the less common use cases.
where: | ||
|
||
- `TContext`: represents an implementation-specific context type that encapsulates session-specific settings and format information (e.g. JSON options, ODataUri, Output stream, custom services) | ||
- `TState`: implementation-specific state type that represents the current state of the serialization. Unlike the context, this is expected to change per object or level (e.g. current type being serialized, nesting depth, etc.). I'm not yet sure if it makes sense to have this |
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.
I'm not yet sure if it makes sense to have this
I was writing an argument against having TContext
and TState
and I convinced myself otherwise. The use-case: TState
is "has a property already been written" so that we can avoid trailing ","; TContext
is a logger that the writer posts to when significant writes have occurred (like the end of an object); TValue
is the value currently being written
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.
TContext
to me is stuff that is not changing throughout the write, which TState
can change throughout the write, and TValue
is necessary be definition
{ | ||
public ODataUri ODataUri { get; } | ||
public IEdmModel Model { get; } | ||
public Ut8JsonWriter JsonWriter { get; } |
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.
based on the separation i see between tstate
and tcontext
, i think the writer goes into tstate
; please clarify what you're thinking the difference is
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.
The thinking here is that the same writer would be passed along throughout the entire serialization of the payload, the writer here is like a stand-in for the output stream, it's shared from the start to the end of the serialization session.
The `ODataOutputContext` does most of the heavy lifting and is the one that determines the output format. | ||
|
||
This means that if you wanted to create your own customer `ODataMessageWriter` from scratch (e.g. to output a different format other than JSON), you would | ||
need to provide custom `ODataOutputContext` and `ODataWriter` implementations. This is a non-trivial undertaking and I doubt there are any implementations |
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.
Issues
Related to issue #3236
Description
This proposal/brainstorming session to figure out the right public API and abstractions to expose for the new writer.