OData / WebApi

OData Web API: A server library built upon ODataLib and WebApi
https://docs.microsoft.com/odata
Other
857 stars 473 forks source link

System.Text.Json support #2174

Closed josundt closed 1 year ago

josundt commented 4 years ago

The latest version of Microsoft.AspNetCore.OData (v7.4.1) still depends on NewtonSoft.Json for correct JSON serialization, e.g. when using $select.

Using OData with AspNetCore's current default serializer System.Text.Json will not serialize projection queries correctly.

Please see #1929 or #2038 for further details.

I'm happy that endpoint routing is finally supported. When can we expect System.Text.Json support, so that we can combine AspNetCore.OData with the latest AspNetCore default stack - without the "fallback to NewtonSoft workaround" using Microsoft.AspNetCore.Mvc.NewtonsoftJson?

gathogojr commented 4 years ago

@josundt We currently don't use NewtonSoft.Json for serialization/deserialization. We read and write the Json directly to the wire to take advantage of some important optimizations. We're adding support for reading/writing metadata as Json and we're using System.Text.Json for that

gathogojr commented 4 years ago

@josundt Can you describe what you're trying to do? Are you swapping the serializer with System.Text.Json

josundt commented 4 years ago

I made a small app to demonstrate the problem:

Please clone repository at https://github.com/josundt/odata-serialization-test. This is an AspNetCore App with one API controller and a single HTML page. The page executes API requests and displays the JSON response. The Controller Action Method uses the [EnableQuery] attribute for OData support.

  1. Run the application.
  2. Notice that the response from the projection ($select) query is wrong.
  3. Try uncommenting line 24 in Startup.cs, then run the app again.
  4. Notice that now, the projection query response is correct.

With NewtonSoft.Json projection query responses are correct, with System.Text.Json they are not.

Can this be fixed any time soon?

gathogojr commented 4 years ago

@josundt I took a look at your repro. The reason your service is not behaving as expected is because you're missing something at the section where you add the endpoint execution to the middleware pipeline. Where you call the UseEndpoints extension method to configure the endpoints, you need to add

endpoints.MapODataRoute(ROUTE_NAME, null/ROUTE_PREFIX, YOUR_EDM_MODEL);

The Edm model MUST be used to describe the data exposed by an OData service and the custom serializer/deserializer depends on it. System.Text.Json is not used.

Kindly take a look at the Startup Setup section on this document for guidance https://devblogs.microsoft.com/odata/enabling-endpoint-routing-in-odata/

gathogojr commented 4 years ago

@josundt Here's what I meant when I said that we don't use System.Text.Json. When AddOData() is invoked, the custom OData input and output formatters are added to the very top of the list of formatters. The System.Text.Json formatter (default for Asp.Net Core I believe) is at the very bottom so it shouldn't take precedence just because you didn't apply the Newtonsoft.Json formatter

image

My assumption is that you want to use EnableQuery without EdmModel, and this should work... and the OData serializers and deserializers should be used However, after more testing I can indeed confirm that when you apply the $select query option specifically you don't get the expected output/response

cc. @xuzhg @mikepizzo

josundt commented 4 years ago

@gathogojr We are using the Non-Edm Approach.

In the article you referred to (https://devblogs.microsoft.com/odata/enabling-endpoint-routing-in-odata/), there's a chapter "Non-Edm Approach".

The chapter starts as follows:

"If you decide to go the non-Edm route, you will need to install an additional Nuget package to resolve a Json formatting issue as follows:

First of all install Microsoft.AspNetCore.Mvc.NewtonsoftJson package [...]"

So this issue is really about: Are you planning to resolve the referred Json formatting issue? When can it be expected?

mikepizzo commented 4 years ago

Hi @josundt -- can you provide a little more info on why you are using the "Non-Edm Approach"? Is there a reason you can't use the "Edm Approach"?

Although it is possible to use some of the OData features individually, they weren't initially designed that way initially, and doing so can lead to unexpected results (like the issue mentioned in the article). While we are working on modularizing the OData features more in the .NET 5 timeframe, you will get a better experience in the current libraries if you opt all-in to the OData "Edm" experience.

For .Net 5 planning, however, we are very interested in understanding scenarios where simply using OData routing and serialization are not desired, and understanding what components are desirable to use independently, and in what scenarios.

m4ss1m0g commented 4 years ago

I have a similar situation: Edm approach not working with private setter (DDD) and I must enumerate ALL properties of my entities. Another issue I found using Edm approach with ODataModelBuilder is that I cannot set some properties name to lowerCase. Example

var entity = builder.EntityType<Seller>();

//entity.HasKey(p => p.Id).Name = "id";
//entity.Property(p => p.EMail).Name = "eMail";
entity.HasKey(p => p.Id);
entity.Property(p => p.EMail);
entity.Property(p => p.FirstName);
entity.Property(p => p.LastName);
entity.Property(p => p.Phone);
entity.Property(p => p.RowVersion);

If I set the name for EMail it works, but if I setup the name of Id (the key) don't work. The name remain the same and if I try to get a single record

http://localhost:5000/api/sellers(1)

Raise 404 Not Found

josundt commented 4 years ago

Hello again and sorry for my late reply.

In my company we are developing different ASP.NET Core Web APIs to serve our own LOB applications, but can also serve any other client since we try to follow best practices and use industry-standards for authorization, versioning, swagger documentation etc.

Most of our APIs are designed following Command/Query Responsibility Segregation principles. OData is by nature very data-centric and cannot cover all the requirements of our APIs, but since OData is an OASIS open standard, it is very suitable when filtering/sorting/pagination/projections is required in the Query part of our APIs. This will allow not only our own LOB apps, but also any other client that supports the OData standard to consume parts of our APIs,

OData is typically especially useful for us (and I bet many others) when is serves as the data source for rich UI Grid layouts with filtering/sorting/pagination support in our clients.

We have one API that uses the full OData stack - in our reporting API. But for most other APIs we really only need the rich query support on certain controllers/action methods, since these APIs are not really data feeds, but rather exposing the full feature set of our LOB apps with commands as well as queries.

If we use [EnableQuery] in our action methods we can't support inline $count, because the response then becomes a simple collection, not the ODataQueryResult object with the value and count in the response.

We therefore found the most useful abstraction for use when using Microsoft.AspNetCore.OData is to use our own action methods with ODataQueryOptions<T> parameters, then apply it to an IQueryable. If $count=true, we need to do some manual work to extract the count query from the queryOptions and apply that to the queryable. We have therefore made an internal NuGet package with some extensions methods to apply the ODataQueryOptions to the IQueryable and return a ODataQueryResult (with the items in the value property and the count in the @odata.count property).

I guess I am not the only one that finds this feature set the most valuable in AspNetCore.OData, and I guess that's why the team continue to support a "Non-EDM approach".

It would be really nice if you could make an improved version of the EnableQuery attribute. I suggest an overload [EnableQuery(UseODataQueryResult = true)] to make the Non-EDM approach even more useful when $count is required.

shanchin2k commented 4 years ago

Is it so that OData yet to support System.Text.Json custom converters as well? I am trying with the sample provided here https://github.com/hassanhabib/ODataWithEndpointRouting3.1, by adding a custom System.Text.Json converter.

  1. Added a simple custom System.Text.Json converter

  2. Added a POST action to the controller

[HttpPost]
public WeatherForecast Post([FromBody] WeatherForecast wf)
{
      return new WeatherForecast();
}
  1. Replaced the following line:

services.AddControllers():

with

services.AddControllers()
                .AddJsonOptions(options =>
                {
                    options.JsonSerializerOptions.Converters.Add(new WeatherForecastRuntimeIgnoreConverter());
                });

The custom serializer's Read() method didn't hit during Post action deserialization. Tested with .Net 5 preview as well, results are same. Is this a known issue?

adrien-constant commented 2 years ago

Is it complicated to get rid of NewtonSoft.Json ?

erionpc commented 2 years ago

I'm using Microsoft.AspNetCore.Mvc.NewtonsoftJson 6.0.2 in a .NET 6.0 web-api project. The project references Microsoft.AspNetCore.OData and enables oData queries on HTTP Get controller actions.

services.AddControllersWithViews(o =>
{
    o.UseGeneralRoutePrefix("api/v{version:apiVersion}");

})
.AddNewtonsoftJson()
.AddOData(options => options
            .AddRouteComponents((new MyDataModelBuilder()).GetEntityDataModel())
            .Select()
            .Filter()
            .OrderBy()
            .SetMaxTop(100)
            .Count());

Everything works fine, until I try using a $select pojection on one of the HTTP GET actions (i.e. https://{host}/{Controller}/{route}$select={property}). I get a deserialization error as follows:

Newtonsoft.Json.JsonSerializationException: Self referencing loop detected for property 'declaringType' with type 'Microsoft.OData.Edm.EdmEntityType'. Path '[0].model.schemaElements[0].declaredKey[0]'.

If I remove .AddNewtonsoftJson() from the services it works, but I need it to enable HTTP Patch actions with JsonPatchDocument<T>. This means I'm stuck with NewtonsoftJson, but I don't know how to make it work with oData. Any ideas would be greatly appreciated.

Issue also raised here: https://github.com/dotnet/AspNetCore.Docs/issues/24952