OData / AspNetCoreOData

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

Return null in OData operations, results in different errors #634

Open Sneedd opened 2 years ago

Sneedd commented 2 years ago

Hello, I encountered ome problems when returning null in OData operations (actions and functions). I cannot provide my actual code, so I made a sample project which result in similar errors.

Here the sample code using Microsoft.AspNetCore.OData 8.0.10 followed with some of the errors which I encountered with this sample project.

public class Startup
{
    public Startup(IConfiguration configuration)
    {
        Configuration = configuration;
    }

    public IConfiguration Configuration { get; }

    // This method gets called by the runtime. Use this method to add services to the container.
    public void ConfigureServices(IServiceCollection services)
    {
        services.AddControllers()
            .AddOData(options =>
            {
                var builder = new ODataConventionModelBuilder();
                builder.Namespace = "MyNamespace";

                var entityType = builder.EntitySet<Article>("Articles").EntityType.Collection;
                var function = entityType.Function("GetName");
                function.Returns<string>();

                var function2 = entityType.Function("GetEntity");
                function2.ReturnsFromEntitySet<Article>("Articles");

                var action1 = entityType.Action("GetEntityAction");
                action1.ReturnsFromEntitySet<Article>("Articles");

                options.AddRouteComponents("odata", builder.GetEdmModel());                    
            });
    }

    // This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
    public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
    {
        if (env.IsDevelopment())
        {
            app.UseDeveloperExceptionPage();
        }
        else
        {
            app.UseExceptionHandler("/Error");
        }

        app.UseODataRouteDebug();
        app.UseRouting();
        app.UseAuthorization();
        app.UseEndpoints(endpoints =>
        {
            endpoints.MapControllers();     
        });
    }
}

public class ArticlesController : ODataController
{
    [HttpGet]
    public IQueryable<Article> Get()
    {
        var articles = new List<Article> { new Article { Id = 100, Name = "First" } };
        return articles.AsQueryable();
    }

    [HttpGet]
    public string GetName() => null;

    [HttpGet]
    public Article GetEntity() => null;

    [HttpPost]
    public Article GetEntityAction(ODataActionParameters parameters) => null;
}

public class Article
{
    public int Id { get; set; }
    public string Name { get; set; }
}

Calling the GetEntity(): Article function results in the following exception and same with the GetEntityAction(): Article:

GET http://localhost:32345/odata/Articles/GetEntity()
GET http://localhost:5000/odata/Articles/GetEntityAction

System.Runtime.Serialization.SerializationException: Cannot serialize a null 'Resource'.
   at Microsoft.AspNetCore.OData.Formatter.Serialization.ODataResourceSerializer.WriteObjectInlineAsync(Object graph, IEdmTypeReference expectedType, ODataWriter writer, ODataSerializerContext writeContext)
   at Microsoft.AspNetCore.OData.Formatter.Serialization.ODataResourceSerializer.WriteObjectAsync(Object graph, Type type, ODataMessageWriter messageWriter, ODataSerializerContext writeContext)
   at Microsoft.AspNetCore.OData.Formatter.ODataOutputFormatterHelper.WriteToStreamAsync(Type type, Object value, IEdmModel model, ODataVersion version, Uri baseAddress, MediaTypeHeaderValue contentType, HttpRequest request, IHeaderDictionary requestHeaders, IODataSerializerProvider serializerProvider)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeResultFilters>g__Awaited|27_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|19_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.OData.Routing.ODataRouteDebugMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

Calling the GetName(): string function results in the following response:

http://localhost:32345/odata/Articles/GetName()

Response in IIS profile:
{
    "@odata.context": "http://localhost:32345/odata/$metadata#Edm.Null"

And no response in Kestrel profile but this exception inside the console:
Microsoft.OData.ODataException: Cannot write the value 'null' in top level property; return 204 instead.
   at Microsoft.OData.WriterValidator.ValidateNullPropertyValue(IEdmTypeReference expectedPropertyTypeReference, String propertyName, Boolean isTopLevel, IEdmModel model)
   at Microsoft.OData.JsonLight.ODataJsonLightPropertySerializer.WriteNullPropertyAsync(ODataPropertyInfo property)
   at Microsoft.OData.JsonLight.ODataJsonLightPropertySerializer.WritePropertyAsync(ODataProperty property, IEdmStructuredType owningType, Boolean isTopLevel, IDuplicatePropertyNameChecker duplicatePropertyNameChecker, ODataResourceMetadataBuilder metadataBuilder)
   at Microsoft.OData.JsonLight.ODataJsonLightPropertySerializer.<>c__DisplayClass9_0.<<WriteTopLevelPropertyAsync>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.OData.JsonLight.ODataJsonLightSerializer.WriteTopLevelPayloadAsync(Func`1 payloadWriterFunc)
   at Microsoft.OData.JsonLight.ODataJsonLightOutputContext.WritePropertyImplementationAsync(ODataProperty property)
   at Microsoft.OData.JsonLight.ODataJsonLightOutputContext.WritePropertyAsync(ODataProperty property)
   at Microsoft.AspNetCore.OData.Formatter.Serialization.ODataPrimitiveSerializer.WriteObjectAsync(Object graph, Type type, ODataMessageWriter messageWriter, ODataSerializerContext writeContext)
   at Microsoft.AspNetCore.OData.Formatter.ODataOutputFormatterHelper.WriteToStreamAsync(Type type, Object value, IEdmModel model, ODataVersion version, Uri baseAddress, MediaTypeHeaderValue contentType, HttpRequest request, IHeaderDictionary requestHeaders, IODataSerializerProvider serializerProvider)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeResultFilters>g__Awaited|27_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|19_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.OData.Routing.ODataRouteDebugMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Watch.BrowserRefresh.BrowserRefreshMiddleware.InvokeAsync(HttpContext context)
   at Microsoft.AspNetCore.Builder.Extensions.MapWhenMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application)

Note the missing end bracket? Also I would expect "value": null or like the exception said a 204. This happends when using the IISExpress profile. Using the Kestrel profile does not return a response, which is easily reproduced with WireShark.

Sneedd commented 2 years ago

The only workaround I found so far is returning nothing and react in my OData client accordingly, here the solution for the sample project.

// Override the services
options.AddRouteComponents("odata", builder.GetEdmModel(), (services) =>
{
    services.AddSingleton<ODataPrimitiveSerializer, ODataPrimitiveSerializer2>();
    services.AddSingleton<ODataResourceSerializer, ODataResourceSerializer2>();
});

internal class ODataResourceSerializer2 : ODataResourceSerializer
{
    public ODataResourceSerializer2(IODataSerializerProvider serializerProvider) : base(serializerProvider) { }

    public override Task WriteObjectAsync(object graph, Type type, ODataMessageWriter messageWriter, ODataSerializerContext writeContext)
    {
        if (graph == null)
        {
            return Task.CompletedTask;
        }
        return base.WriteObjectAsync(graph, type, messageWriter, writeContext);
    }
}

public class ODataPrimitiveSerializer2 : ODataPrimitiveSerializer
{
    public ODataPrimitiveSerializer2() { }

    public override Task WriteObjectAsync(object graph, Type type, ODataMessageWriter messageWriter, ODataSerializerContext writeContext)
    {
        if (graph == null)
        {
            return Task.CompletedTask;
        }            
        return base.WriteObjectAsync(graph, type, messageWriter, writeContext);
    }
}

This currently works for me, but the solution is not optimal.

Or changing everything to IActionResult which would be a major change for me.

Hope someone have a better workaround.

julealgon commented 2 years ago

@Sneedd do you have nullable reference types enabled in the project? Does it make a difference if you declare the return value as Article? instead of Article? Like:

.ReturnsFromEntitySet<Article?>("Articles");
Sneedd commented 2 years ago

@julealgon I tried it now with Article? and changed the Nullable setting of the project but it did not make a difference.

I also tried to find the example/documentation where I got this information (to send a actual value instead of IActionResult), but with no luck. So my question would be: Should both following examples work? And if not wouldn't it be better to throw an exception.

[HttpGet]
public string GetName() => null;  // Does not work 

[HttpGet]
public IActionResult GetName() => this.NoContent();  // Works

Btw, I started to use the IActionResult approach, because my previous mentioned workaround did not felt good after some more testing.

julealgon commented 2 years ago

@Sneedd I know this doesn't answer your original question, but is there a strong reason you want to return null, instead of returning a 404 which is the standard behavior for "when a resource is not found"? Are you using the null value with this semantic, and if so why?

julealgon commented 2 years ago

@Sneedd can you give this a shot?

.ReturnsFromEntitySet<Article>("Articles").ReturnNullable = true;
Sneedd commented 2 years ago

@julealgon Thanks for your reply and sorry for my late reply. I return 404 but only when working with the resource endpoints and an entity is missing. But the operation endpoints for OData action and functions are different, I only provide a abstraction a simple API for someone how want use this feature. Because only I am implementing new functions and actions, I wanted to give the other developers the freedom to send null. That said I already starting to change the underlying behavior to IActionResult return values.

.ReturnsFromEntitySet<Article>("Articles").ReturnNullable = true;

I already tried this and found out that ReturnsFromEntitySet() sets ReturnNullable already to true, so nothing different here.

Because the behavior in the initial comment cannot be correct and is easily reproduced, I thought I should report it. Like I said, I have a workaround with which I am happy.

julealgon commented 1 year ago

Thanks for clarifying @Sneedd . Now I wonder if this is an actually supported scenario or not. If ReturnNullable is already true by default, this could very well mean that entities just can't ever be null?

Will leave it up to someone on the team to check this.

Regardless, I think the user experience here is pretty bad. IMHO:

So even if there is no bug here, I think there are changes to be done.

mikepizzo commented 1 year ago

According to the OData V4 spec, the return value for a primitive (i.e., string) function returning a null value should be 204 (as per the error in the Kestrel example above). In OData V3, we returned the #Edm.Null value. There was a bug in early versions of the OData V4 library that we continued to return this invalid null value in OData V4 and we fixed it, but we added a LibraryCompatibilityFlag for backward compatibility to support existing services that may have clients receiving that value. So, I'm surprised this value is getting returned (even invalidly) without somehow setting that compatibility flag.

ReturnNullable=true means that the function/action should be able to return a null value. The way that the null value is represented in OData V4 is as a 204/404 (depending on whether or not it is primitive).

So, if I understand the issue, the request is that the controller method shouldn't have to understand the protocol -- it should just be able to return the null value and have the framework do the right thing (return 204). That seems a reasonable separation of having controller method implementing the data logic and the framework understanding how that is represented in the protocol.

julealgon commented 1 year ago

ReturnNullable=true means that the function/action should be able to return a null value. The way that the null value is represented in OData V4 is as a 204/404 (depending on whether or not it is primitive).

Interesting, thanks for sharing.

So, if I understand the issue, the request is that the controller method shouldn't have to understand the protocol -- it should just be able to return the null value and have the framework do the right thing (return 204).

Feels to me like this is an option, but one could also at least improve the exception message in the entity-based cases above. Contrary to the primitive value exception which has a very descriptive message that leads people into the correct action, the entity exception is extremely generic and has no explanation about what is happening.

Guiding people into using the proper http response by updating that exception message would result in less "magic" in the framework. That can sometimes be seen as a good thing too.