OData / AspNetCoreOData

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

SelectSome<TEntity> class serialized to response for select and expand queries #585

Closed kjaworski closed 2 years ago

kjaworski commented 2 years ago

After upgrade to OData v8 I noticed the following error when I used select or expand query in request for my service:

An unhandled exception has occurred while executing the request. Newtonsoft.Json.JsonSerializationException: Self referencing loop detected for property 'declaringType' with type 'Microsoft.OData.Edm.EdmEntityType'. Path 'container.value.model.schemaElements[0].declaredKey[0]'. at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.CheckForCircularReference(Js onWriter writer, Object value, JsonProperty property, JsonContract contract, JsonContainerContract containerContract, JsonProperty containerProperty)

So I used the following code to make it serialize properly:

json.SerializerSettings.ReferenceLoopHandling = ReferenceLoopHandling.Serialize;
json.SerializerSettings.PreserveReferencesHandling = PreserveReferencesHandling.Objects;

And then got tons of data unrelated to my entities in the response. An example:

  {
    "$id": "3538",
    "instance": null,
    "container": {
      "$id": "3539",
      "name": "MyPropertyName",
      "value": "MyPropertyValue",
      "next0": {
        "$id": "3540",
        "name": "MyAnotherPropertyName",
        "value": MyAnotherPropertyValue,
        "autoSelected": true
      },
      "autoSelected": false
    },
    "model": {
      "$ref": "4"
    },
    "untypedInstance": null,
    "instanceType": null,
    "useInstanceForProperties": false
  }

After some investigation I've found out that this is SelectSome<TEntity> class serialized, an internal one in OData. Is it a normal behavior? Can it be configured somehow? Or is it a bug?

xuzhg commented 2 years ago

@kjaworski If you want to serialize SelectSome< T >, you can :

1) use our Microsoft.AspNetCore.OData.NewtonsoftJson package 2) Cast selectSome< T > to ISelectExpandWrapper and call ToDictionary() to get a dictionary, then use it to serialize.

Let me know the result.

kjaworski commented 2 years ago

@xuzhg I'm not sure if this is what I meant. I have no access to SelectSome<TEntity> because it's marked as internal.

Let's assume that my response with no queries applied looks this way:

{
    "Property1": 1,
    "Property2": "2",
    "Property3": true
}

Now let's say I used select query to get only Property1. Before upgrading OData to v8 the response was:

{
    "Property1": 1
}

But after upgrade to v8 it contains serialized SelectSome<TEntity> class (which I don't want to have in my response):

  {
    "$id": "3538",
    "instance": null,
    "container": {
      "$id": "3539",
      "name": "Property1",
      "value": 1,
      "next0": {
        "$id": "3540",
        "name": "MyAnotherPropertyName",
        "value": MyAnotherPropertyValue,
        "autoSelected": true
      },
      "autoSelected": false
    },
    "model": {
      "$ref": "4"
    },
    "untypedInstance": null,
    "instanceType": null,
    "useInstanceForProperties": false
  }

Can I do something about that? To make the response look like before the upgrade?

xuzhg commented 2 years ago

@kjaworski As I mentioned, we have the JSON converter for SelectSome< T >.

1) if you are using System.Text.Json to serialize, the converter is added implicitly when you call "AddOData" 2) If you are using Newtonsoft.JSOn to serialize, you should add https://www.nuget.org/packages/Microsoft.AspNetCore.OData.NewtonsoftJson/ package to call https://github.com/OData/AspNetCoreOData/blob/main/src/Microsoft.AspNetCore.OData.NewtonsoftJson/ODataNewtonsoftJsonMvcBuilderExtensions.cs#L27

3) If you want to serialize the SelectSome< T > by yourself. as I mentioned, when you get an object after calling ApplyTo, you can cast it to ISelectExpandWrapper (This interface is public), Then you can call ToDictionary() on the interface.

If that's not clear to you. Would you please share a simple github repo with your projects , then i can help you to make it pass.

Thanks.

kjaworski commented 2 years ago

@xuzhg Just to let you know - we're still looking at this issue on our side. Will notify when our investigation is done.

kjaworski commented 2 years ago

@xuzhg Sadly I cannot share the code, because it's a commercial project not owned by me, but I'll try to explain as much as I can.

We've finally were able to make it work in some scenarios by inheriting from EnableQueryAttribute and adding something like that to OnActionExecuted in our child class:

            base.OnActionExecuted(context);

            if (context.Result is OkObjectResult tempResult) {
                if (tempResult.Value is IQueryable tempQueryable) {
                    tempResult.Value = tempQueryable.Cast<ISelectExpandWrapper>()
                        .Select(selectExpandQueryResult => selectExpandQueryResult.ToDictionary());
                }
            }

But this requires additional checks and is kind of a sneaky workaround.

We use Newtonsoft to serialize and second option described in your previous post seems to be perfect, but it does not work. In action context I have a collection of SelectSome<MyDto>. In configuration I have:

                services
                    .AddControllers()
                    .AddOData(options => {
                        options.Select().Filter().Expand().Count().OrderBy().SetMaxTop(maxTopValue: null);
                    })
                    .AddODataNewtonsoftJson();

But it doesn't serialize the collection of SelectSome<MyDto> properly, because it doesn't call ToDictionary(). Seems that OData 7.x used to call that method by default, while 8.x doesn't do this.

What we've found out is that SelectExpandWrapper<TElement> class was decorated with [JsonConverter(typeof(SelectExpandWrapperConverter))] in 7.x, while in 8.x there is no attribute on SelectExpandWrapper<TElement> . As a result (at least that's what I believe), serializer contract doesn't have Converter property set (it's just null) in 8.x. In 7.x there was SelectExpandWrapperConverter under Converter property in the contract.

Is there something I can do about it?

kjaworski commented 2 years ago

@xuzhg I cloned OData sources, added the attribute and it seems to work now when I'm testing it locally.

xuzhg commented 2 years ago

@kjaworski As mentioned, Microsoft.AspNetCore.OData.NewtonsoftJson is the package used to introduce the converters into the newtonsoft json serialization setting/config.

julealgon commented 2 years ago

You need some sleep Sam:

image

🤣

kjaworski commented 2 years ago

@xuzhg As mentioned, Microsoft.AspNetCore.OData.NewtonsoftJson is the package used to introduce the converters into the newtonsoft json serialization setting/config.

Yup, and I used it, as described in my previous comment. But it does not work. It works only if I "fix" OData code locally (by adding JsonConverterAttribute), build it and then reference in my project.

Is it a bug? Or am I using it in a wrong way?

xuzhg commented 2 years ago

You need some sleep Sam:

image

🤣

@julealgon Haha, a typo.

@kjaworski SelectSome< T > or any other select and expand related classes are by design not decorated with converter attribute. I can't remember why I didn't decorate the System.Text.Json converter on the classes, but for Newtonsoft.JSON, It's clear that I don't want to have a dependency on the Newtonsoft.JSON package on the Microsoft.AspNetCore.OData project.

kjaworski commented 2 years ago

@xuzhg So is there a way (other than serializing by myself with cast to ISelectExpandWrapper and calling ToDictionary()) to make it work?

xuzhg commented 2 years ago

@kjaworski How do you use the newtonsoft.json ? How do you use newtonsoft.json to serialize? If you are using ASP.NET Core, you can call 'https://github.com/OData/AspNetCoreOData/blob/main/src/Microsoft.AspNetCore.OData.NewtonsoftJson/ODataNewtonsoftJsonMvcBuilderExtensions.cs#L27' to config the converters into the newtonsoft.json serialize settings, then you can use the dependent injection to retrieve the seralize setting to do the serialize.

If you don't use the ASP.NET Core, it could be tricky to build the DI by yourself, for example: https://github.com/OData/AspNetCoreOData/blob/main/test/Microsoft.AspNetCore.OData.NewtonsoftJson.Tests/ODataNewtonsoftJsonMvcBuilderExtensionsTests.cs#L40-L63

Since you can't share the whole project, you can create a simple project (host on github) using parts of your project codes to reproduce the issue, I am very happy to take a look and make it working.

kjaworski commented 2 years ago

Since you can't share the whole project, you can create a simple project (host on github) using parts of your project codes to reproduce the issue, I am very happy to take a look and make it working.

@xuzhg It seemed easier to try to reproduce my problem in ODataNewtonsoftJsonSample project from AspNetCoreOData.NewtonsoftJson solution than to extract parts of our solution (it's quite complex, nasty architecture...). And I did - I went to Startup class, removed AddRouteComponents() from service configuration (because we don't have it). Then I commented out AddODataNewtonsoftJson() and uncommented AddNewtonsoftJson(). That's the final result:

        // This method gets called by the runtime. Use this method to add services to the container.
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddControllers()
                .AddOData(opt => opt.Select().Filter().Count().SetMaxTop(10))
                .AddNewtonsoftJson(
                options =>
                {
                    options.SerializerSettings.DefaultValueHandling = Newtonsoft.Json.DefaultValueHandling.Ignore;
                    options.SerializerSettings.ReferenceLoopHandling = Newtonsoft.Json.ReferenceLoopHandling.Serialize;
                    options.SerializerSettings.PreserveReferencesHandling = Newtonsoft.Json.PreserveReferencesHandling.Objects;
                })
                ;
        }

Here's the original code: https://github.com/OData/AspNetCoreOData/blob/3df3a0b009ed465a9e0a87d0c722d08921d6b4c4/sample/ODataNewtonsoftJsonSample/Startup.cs#L30

With this configuration setup I've got the same issue as in our project. As a consequence, we suspect that settings from AddODataNewtonsoftJson() are somehow overwritten by AddNewtonsoftJson() somewhere in our projects (or ignored because of some precedence rules).

Will let you know after we confirm this.

xuzhg commented 2 years ago

@kjaworski AddODataNewtonsoftJson() is required, as I mentioned, it will add OData converters into the SerializerSettings.

There's no conflict to call AddNewtonsoftJson() after calling AddODataNewtonsoftJson().

I tested at my side using ODataNewtonsoftJsonSample

services.AddControllers()
                .AddOData(opt => opt.Select().Filter().Count().SetMaxTop(10)/*.AddRouteComponents("odata", GetEdmModel())*/)
                .AddODataNewtonsoftJson()
                .AddNewtonsoftJson(
                options =>
                {
                    options.SerializerSettings.DefaultValueHandling = Newtonsoft.Json.DefaultValueHandling.Ignore;
                    options.SerializerSettings.ReferenceLoopHandling = Newtonsoft.Json.ReferenceLoopHandling.Ignore;
                    //options.SerializerSettings.ContractResolver = WebApiJsonResolver.Instance;
                })
                ;

Sending request as http://localhost:5000/WeatherForecast?$select=temperatureC. it should return as:

[{"TemperatureC":51},{"TemperatureC":47},{"TemperatureC":10},{"TemperatureC":43},{"TemperatureC":-19}]

Besides, if you want to serialize the object returned from 'ApplyTo()', you can retrieve the serializer setting through DI. See https://github.com/OData/AspNetCoreOData/blob/3df3a0b009ed465a9e0a87d0c722d08921d6b4c4/sample/ODataNewtonsoftJsonSample/Controllers/WeatherForecastController.cs#L30.

See the debug: image

Any time, when you want to serialize object, you should use the API overload with SerializerSettings.

for example: https://www.newtonsoft.com/json/help/html/M_Newtonsoft_Json_JsonConvert_SerializeObject_5.htm

kjaworski commented 2 years ago

@xuzhg Thanks a lot for help! Seems like we've finally got it working. Your last comment made me think that indeed there is some nasty bug on our side and as an act of desperation - I cleared everything and started from scratch. And... puff, worked like a charm - this time it was enough to add AddODataNewtonsoftJson() call to the configuration.

I've analysed my previous changes and haven't found anything suspicious - that leads me to a conclusion that probably some locally cached nuget packages with my fix attempts messed things for us.

xuzhg commented 2 years ago

@kjaworski FYI. One of our customer met the similar issue. I spent a lot of time to investigate, and Finally, I figured out that one of his attribute decorated on controller reset/config the serializer setting. It could be a direction for you to dig.

Of course, I am glad to see it working. If you have any other questions, feel free file new issues for us. Thanks.