OData / AspNetCoreOData

ASP.NET Core OData: A server library built upon ODataLib and ASP.NET Core
Other
457 stars 157 forks source link

Batch handler doesn't work with source-generated models #977

Open CheloXL opened 1 year ago

CheloXL commented 1 year ago

Assemblies affected ASP.NET Core OData 8.2.0

Describe the bug Using the DefaultODataBatchHandler to handle batch requests, simply by changing the json serialization (in AddJsonOptions) to use source generated models, any request to any endpoint fails with a 500 error. No error is logged anywhere. Please note that I'm not using OData to do anything else. Just batch handling (no edm models, no DB access, etc).

This is my OData configuration:

.AddOData(options =>
{
    options.EnableContinueOnErrorHeader = true;
    options.AddRouteComponents("api", new EdmModel(), serviceCollection =>
    {
        serviceCollection.AddSingleton<ODataBatchHandler>(new DefaultODataBatchHandler());
        serviceCollection.AddSingleton<IStreamBasedJsonWriterFactory>(_ => new DefaultStreamBasedJsonWriterFactory(JavaScriptEncoder.UnsafeRelaxedJsonEscaping));
    } );
})

Data Model It doesn't matter.

EDM (CSDL) Model No EDM models are used.

Request/Response Sample request: {"requests":[{"body":{"operations":[{"op":"replace","path":"lastUpdate","value":"2023-07-01T01:22:02.892Z"},{"op":"replace","path":"stats/weight","value":78}]},"id":"user_cljjbiuj30000fke2p30c","method":"PATCH","url":"user/P9MhPnlnGUmOJza2i4EZ3h2UQ","headers":{"content-type":"application/json-patch+json"}}]}

Sample response: {"responses":[{"id":"user_cljjbiuj30000fke2p30c","status":500,"headers":{}}]}

Nothing is logged in the app, even in Debug mode.

Expected behavior It should work, as it works if I don't use source-generated models.

julealgon commented 1 year ago

Wouldn't you need to register the models in the EDM for this to work as expected?

How does the example without source generated models look like on your end?

CheloXL commented 1 year ago

@julealgon Not sure I'm following you. I don't have to register any EDM models for the batch handler to work. The endpoints are standard aspnetcore controller/actions, registered with HttpMethodAttribute/UseRouting.

This is already working without source generated serialization. Things break when I create a new XXSerializationContext : JsonSerializerContext, and then register that context into the default json serializer. If I call any of the enpoints, it works. If I call the endpoints via the batch handler, it doesn't work.

julealgon commented 1 year ago

@julealgon Not sure I'm following you. I don't have to register any EDM models for the batch handler to work.

Fair enough. I was just asking since I wasn't sure myself, thanks for clarifying.

This is already working without source generated serialization. Things break when I create a new XXSerializationContext : JsonSerializerContext, and then register that context into the default json serializer. If I call any of the enpoints, it works. If I call the endpoints via the batch handler, it doesn't work.

Doesn't the batch handler use a separate implementation of the serializer, vs the rest of the MVC pipeline, because it uses a completely different container instance behind the scenes? If so, I'd imagine you'd have to register your new context in the specific container for the OData route component and not via AddJsonOptions necessarily. The fact that OData doesn't rely on the "project-wide" standard serializer configuration is a well-known limitation.

I could be wrong, however.

CheloXL commented 1 year ago

@julealgon That's what's strange. If it uses a different serializer, it should work anyways. There is nothing in the DTO that states how the serialization should be. Regarding the OData serialization, I didn't see any options to register any context. As shown on the registration code above, you can only pass an enum to the ctor, and there are no properties that deals with the serialization context.

Probably I could solve that by implementing myself all the IStreamBasedJsonWriterFactory machinery, but I would prefer not to do so, as using the default json serializer should be the standard anyways. They already support the system.json serializer using the above code, but it seems they are using an internal set of options instead of getting them from the service provider.

gathogojr commented 1 year ago

@CheloXL Thank you for reaching out to us. In your issue description, you mentioned that you're not using any Edm model. That's not quite accurate. You're actually passing an Edm model to the AddRoutComponents method only that the model doesn't contain any elements. AFAIK, there's no overload of that method that doesn't accept an Edm model.

I'd like to understand your issue better, specifically the scenario that is working vs the one that is not. I'm not able to understand based on the information provided. Could you share a minimal repro for the working scenario and another for the non-working scenario - or both combined in one?

habbes commented 1 year ago

@CheloXL When you say that things work when you don't use source-generated models, are you also using OData batch in that scenario? From the configuration snippet you shared in the description, it seems that you are actually configuring an OData route with the /api prefix and an empty Edm model. I assume you're making the batch request to POST /api/$batch, is that correct?

What controller handles the PATCH user/{id} endpoint, that's the single request in your sample payload. What does that controller return?

OData routes are handled by OData routing conventions, which usually send requests to OData controllers and the responses are handled by custom OData formatters which implement OData-based serializations. IStreamBasedJsonWriterFactory allows you override the default OData JsonWriter with a custom one (in this case the DefaultStreamBasedJsonWriterFactory.Default). In short, this library does not use the default ASP.NET Core serializer, it does not use JsonSerializer to serialize responses. It uses a custom serializer that is "OData-aware" and serializes responses based on the OData specification and validation rules.

CheloXL commented 1 year ago

@habbes: 1) Correct. The message example I posted was sent to POST /api/$batch. 2) The action is under the /api/user/{id} endpoint. It's a standard webapi action that receives the id and a JsonPatchCollection as argument and returns a 204. The JsonPatchCollection is a standard class with some properties on it. Nothing fancy. 3) I understand that, and that's why I don't get how changes in the JsonSerializer could affect OData, but again, I have the system working fine (batch calls works, same as calling individual endpoints). If I set the TypeInfoResolver in the ConfigureHttpJsonOptions method with my JsonContexts, batch calls stop to work with a HttpStatus 500, whle individual endpoints continue to work.

The worst part of this is that I get nothing in the logs.

veselinbg commented 8 months ago

Hi, I have the same issue when trying to add JsonSerializerContext to my OdataApi. I received this error when I try to run the api.

['JsonSerializerOptions' instance associated with a 'JsonSerializerContext' instance cannot be mutated once the context has been instantiated.

I tried to remove any JsonSerializerOptions declaration but still there is no changes.

Is there any progress with this issue?