OData / AspNetCoreOData

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

It is very hard or impossible to migrate application using 7.5.8 to 8.x.x.(it as if they are operating two codebase) #237

Open chrisgate opened 3 years ago

chrisgate commented 3 years ago

With these three lines of code : endpoints.EnableDependencyInjection(); endpoints.Select().Expand().Filter().OrderBy().MaxTop(100).Count(); endpoints.MapODataRoute("odata", "odata", GetEdmModel());

We are in and odata is there waiting silently but 8.x.x seem to be noisy and forceful in my opinion.

The above made it too hard or impossible to migrate application using 7.5.8 to 8.x.x.

I just wish it can be easy!

robertmclaws commented 3 years ago

Hey Chris! Sorry you're having issues with the upgrade. According to the SemVer spec, incrementing the Major version number (8.0.0 vs 7.5.8) means that you can expect breaking changes in the APIs. We did a lot of work to optimize those APIs to be a modern platform for moving OData into the future on .NET 5.0 and beyond. That means we had to drop a few things, and move some things around. That's the reason it feels like it's two different codebases, because it is.

I'm just an outside contributor that worked on some of those API changes for the 8.0 release, but I think it would make sense for the team to put together a migration guide to help. In the meantime, you can check out this sample, which shows how to turn everything on, and also demonstrates multiple OData services per app.

Hope that helps!

HuntJason commented 3 years ago

I really wish the OData and Swagger teams were on the same page. Seems OData releases breaking changes and the Swagger team is always playing catch-up.

dcadlereii commented 3 years ago

@xuzhg What would really help at this point is to have your sample projects in the repo include a routing sample with the Microsoft.AspNetCore.OData v8.0.1 and the example show the correct configuration for the v8.0.1. Or do an updated blog post like you did with https://devblogs.microsoft.com/odata/attribute-routing-in-asp-net-core-odata-8-0-rc/ only for v8.0.1

I am just trying to go from 8.0.0_preview3 to 8.0.1 and I am getting errors like The type or Namespace ODataModelAttribute could not be found or IServiceCollection' does not contain a definition for 'AddOData' and the best extension method overload 'ODataMvcBuilderExtensions.AddOData(IMvcBuilder)' requires a receiver of type 'IMvcBuilder'. Looking on Google for working examples has a staggering number of examples that do not actually work with v8.0.1.

xuzhg commented 3 years ago

@dcadlereii

Do you mean 'https://github.com/OData/AspNetCoreOData/tree/master/sample/ODataRoutingSample'? All the samples use the latest codes.

For the post, i'd like to draft one in the future. For the renaming, sometime you can find a brief summary about the changes from https://docs.microsoft.com/en-us/odata/changelog/aspnetcoreodata-8x#800

dcadlereii commented 3 years ago

@xuzhg I am using Microsoft.AspNetCore.Odata v8.0.1 and if I just do something simple in Startup.cs like just "services.AddOData();" it says 'IServiceCollection' does not contain a definition for 'AddOData' and the best extension method overload 'ODataMvcBuilderExtensions.AddOData(IMvcBuilder)' requires a receiver of type 'IMvcBuilder'

I get the same issue if I just copy and paste the following code from your ODataRoutingSample;

        services.AddOData()
            .AddODataRouting(options => options
                .AddModel(EdmModelBuilder.GetEdmModel())
                .AddModel("v1", EdmModelBuilder.GetEdmModelV1())
                .AddModel("v2{data}", EdmModelBuilder.GetEdmModelV2()));

Looking through the using statements in your sample Sartup.cs, I do not see namespaces related to OData that I do not already have. But obviously, I am missing something. Just not seeing what it is.

HuntJason commented 3 years ago

@dcadlereii Double check your packages folder to ensure it hasn't rolled back your Microsoft.AspNetCore.Odata package

xuzhg commented 3 years ago

@dcadlereii did you update your local codes or fork repo?

Here's the snapshot of startup.cs in the master branch image

your codes are old version.

HuntJason commented 3 years ago

@xuzhg, I think you meant @dcadlereii

dcadlereii commented 3 years ago

@xuzhg the ../blob/master/sample... was the problem. Following the version listed resolved the registration of OData issue and the samples showed examples of the change in the attribute from [ODataModel("v1")] to [ODataRouteComponent("v1")] fixed the issue with the controllers. Also the change for OData Function routes from [ODataRoute("GetPrice(organizationId=...")] to [HttpGet("GetPrice(organizationId=...")]. However, I am still trying to resolve the multi-part route. In v8.0.0=preview3, I was able to use [ODataRoute("SalesOrders({key})/SalesOrderLines")] as an attribute for my action but changing this to [HttpGet("SalesOrders({key})/SalesOrderLines")] does not work and gets a 404. Do you know why that is happening?

xuzhg commented 3 years ago

@dcadlereii

Since rc version, i removed [ODataRouteAttribute] and [ODataRoutePrefixAttribute] and replace to use [HttpGet] etc. When we use [HttpGet] etc, please put the "prefix" in the route template.

For your information, here's a post related to the attribute routing for your information: https://devblogs.microsoft.com/odata/attribute-routing-in-asp-net-core-odata-8-0-rc/

dcadlereii commented 3 years ago

@xuzhg OK, so adding the v1/ to [HttpGet("v1/SalesOrders({key})/SalesOrderLines")] allowed the routing to hit the action in the controller.

Odd that I didn't have to add the prefix to the HttpGet calls for my OData functions. For example, [HttpGet("GetAvailableProducts(organizationId={organizationId})")] doesn't have the route prefix appended but hits the function properly.

Now that I am hitting the action, I am getting the following error in Postman;

"Message": "The query specified in the URI is not valid. Could not find a property named 'salesOrderId' on type 'EII.CD.Models.ERP.M1.SalesOrderLine'.",
"ExceptionMessage": "Could not find a property named 'salesOrderId' on type 'EII.CD.Models.ERP.M1.SalesOrderLine'."

But I have SalesOrderId in my SalesOrderLine model of the correct data type. The only thing that seems odd to me here is that in the error message, it is saying it can't find a property named "salesOrderId" starting with a lowercase "s" rather than the uppercase "S" that the property actually has.

dcadlereii commented 3 years ago

@xuzhg ok, I changed my Controller classes so that they derive from ODataController instead of ControllerBase (using ControllerBase was done in your Routing sample controller code.) and now, my method works without the error.

Is it recommended to always use ODataController as the base class for OData based controllers?

xuzhg commented 3 years ago

@dcadlereii No, ODataController is not necessary in my original design. Both in conventional routing and attribute routing.

However, in order to avoiding polluting the existing ASP.NET Core attribute routing, I introduced the ODataAttributeRoutingAttribute. Only Action or controller has this attribute decorated, we consider it's an OData attribute routing. ODataController is decorated with this attribute, so if you derive your controller from ODataController, you don't need to decorate your controller using ODataAttributeRoutingAttribute.

dcadlereii commented 3 years ago

@xuzhg OK, that makes sense about the ODataCOntroller derived class and the ODataAttributeRoutingAttribute. The only question now is why did I need to change to the ODataController derived class from ControllerBase in order to not get the following error, as described in my earlier post.;

"Message": "The query specified in the URI is not valid. Could not find a property named 'salesOrderId' on type 'EII.CD.Models.ERP.M1.SalesOrderLine'.", "ExceptionMessage": "Could not find a property named 'salesOrderId' on type 'EII.CD.Models.ERP.M1.SalesOrderLine'."

stack111 commented 3 years ago

@dcadlereii is the exception related to #251

dcadlereii commented 3 years ago

@stack111 possibly. It seems like a casing issue. I will try your work around this week and let you know if it fixed the issue.

TehWardy commented 3 years ago

I have great respect for the people that work at Microsoft (and in particular @xuzhg and his team looking after odata) but there seems to have been a shift since .Net Core came around and this whole Github ecosystem and its frustrating to say the least.

My experience was quite confusing but ultimately no worse than I expected due to there being no clear cut guide on this (unless you want an out of date preview guide). there are however the sample projects which helped to no end! When I got it going the changes were actually fairly simple on review and it took me about a day ...

My frustration was less about getting it up and running and more one of "why is this standard stuff broken now despite my cleaner codebase". The less code I write the more stuff appears to be broken. If It's my code I can fix it, if it's framework it's a matter of waiting (in my experience) anything up to about 6 years for Microsoft to fix it or having to maintain the framework too through PR's (hard to find the time when I have my own codebase to maintain). In waiting I have to upgrade which starts the loop again, so for me its often about picking the version with the bugs I can bear rather than striving to stay up to date with an expectation that things have improved.

I'm still (it's only been a week) dealing with a lot of fallout from our .Net 5 upgrade and we even had to rebuild pieces of our API in order to avoid functionality that no longer works but something as fundamental as expands (which seem to only go 1 level deep now for some reason) should not be broken.

I get the need for breaking changes, but if I apply the new API calls correctly after making those documented breaking changes I should not have less functionality than the previous now effectively unsupported version that had most of this worked out but now has a host of new problems due to the architecture shift as well.

Potential suggestion here (if Microsoft puts this up I could see myself adding to this) ...

For something like OData surely the answer here is to have a test suite that goes with the OData version rather than the framework / implementation version as a separate solution that does not depend on any implementation code. When a new version of the implementation is rolled out, point that test suite at it and run them all. Each time a bug is identified write a test that causes it and then make that test pass in the current version. Over time as each new major version comes out the suite will be inherited and more tests added in theory making the solution more and more stable.

For each version released mention that it's been tested with the separate test suite and report on the status when the code is published to its own github.

For me I don't care so much what the framework requires me to write, if it's messy I'll put my own architecture in place to make it clean, I care mostly about functionality and I should not see a regression between versions.