Open mhamri opened 3 years ago
xuzhg
commented
3 years ago @mhamri Thanks for your criticisms/questions/concerns/suggestions in your blog. I really appreciate it and am learning it.
By the way, what do you mean "hard to debug"? To be honest, the project supports the source link and the symbols can automatically load.
mhamri
commented
3 years ago thanks for considering it.
When I mentioned hard to debug, I mean the development flow.
Let's say, I added a controller and an action method. Can I be 100% sure that when I run the application it works? It's 50-50. I should go to the ~/odata (assuming I already using that middleware helper) and check if it's discovered by any of the conventions.
And let's assume that my controller and action not discovered by the OData. then what should be my next set of actions and how should I debug it? Could the source link help in this scenario?
Source link only works if you have the object in your code and you run the application, then press f12 on that code. When something is called by the dependency injection, you can't use the source link to debug it.
The only way to debug it is to clone the repo, replace the nugget with a project link, then find the code and put a breakpoint on that. This requires that the user know that how the loading mechanism works, what convention is handling what, ...
if you are expecting a user to debug a library to understand what is wrong with his usage, then it's a good sign of bad architecture.
That's the problem with the convention-based approach, everything becomes magic and a simple usage convert to "debug a library". that's why asp.net core has 1 or two simple conventions, the rest of the things use interface, action, func, attributes, ... to configure stuff.
less magic, more configuration.
RobTF
commented
3 years ago Hi @mhamri,
I had a look through your post and it's really well written and has plenty of detail - it was a good read; interestingly you seem to be coming at this from the exact same angle I did initially, and I found I was fighting the library at every turn (fyi I did find that the source link worked fine for me, however). I think a lot of the points you raised as a general concept (magic vs config etc.) I totally agree with, however I think with this library in particular it's not that straightforward.
I think the crux of a lot of your (and my!) pain this is the clash of two ideologies; I was trying to attack the problem from an ASP.NET Core approach - I wanted to write controllers, put the actions/routes in and have the OData bits Just Work™. Maybe I've drunk the OData koolaid, but I've come to realise why this isn't so straightforward; the primary purpose of this library is to implement the OData standard, and the standard itself is highly opinionated in things like how URIs are structured and it also requires the $metadata endpoint to clients to automatically consume it.
What I also perceived as annoying "magic" (in things like routing) is in fact the library just doing its job - you as the developer aren't the one picking the routes - OData is. If the developer is the one in control of things like routing, that's just non-OData REST, and you could do that already without this library.
As I have now come to understand it, the library works as so;
Originally I was thinking (and I believe you are, at least to some extent) of something like this being better;
However, there are problems with the latter;
In the end, you'd end up having to put things together in such a controlled manner it would end up having little to no benefit. I think there are cases where you do want to arbitrary routes for things like allowing users to "just get a bunch of entities" which is why I'm a big proponent of keeping the non-EDM approach working. That said, myself and my team have moved towards the EDM approach as this enabled use of client side OData clients, which is a big benefit for us (IQueryable on the client is a really nice way of working with OData!!).
Is the library perfect? Certainly not and I think @xuzhg would agree it's a work in progress - especially version 8 which feels like a near-as-damn-it rewrite. I also very much agree with your sentiment about the general development loop needing work, and I think the tooling could be improved greatly (e.g. a code analyser which could perhaps report non-matching routes in the IDE). The fact you have to splodge in demo code that spits out the routes to a blob of HTML to get your bearings whilst performing a bunch of trial and error is testament to this. I'm sure Sam isn't oblivious to this either, and with time I'm sure will come improvements, hopefully with developers such as ourselves helping direct that.
At the same time it's fairly obvious that despite being Microsoft supported, this is a relatively fringe library and I don't think there are many people other than Sam working on it, I hope that changes, and I wish I had time to contribute actual code... maybe someday!
mhamri
commented
3 years ago @RobTF thanks for reading the article. I agree with you at some point, but still, I think the latter was a better option. Let me elaborate:
the primary purpose of this library is to implement the OData standard, and the standard itself is highly opinionated in things like how URIs are structured and it also requires the $metadata endpoint to clients to automatically consume it.
The $metadata could be the product of the available routes and their data. Instead of hard-coding the EDM/name/name-space/container in an old-fashioned way. It could be all discovered (or overridden by some attribute) As I mentioned in the article, the issue is that the specification and implementation are two different things, but ATM is considered as one thing. When that happens the specification always is the stronger element in the equation, it always dictates what and how things need to be implemented. It was just fine if we weren't doing that on top of ASP.net core, but unfortunately, we are. That's why we end up with this heterogeneous solution for OData. to be frank I think the v8 implementation even breaks RESTier
you as the developer aren't the one picking the routes - OData is. If the developer is the one in control of things like routing, that's just non-OData REST, and you could do that already without this library.
As I suggested, this still could be achieved if a middleware translates the request to a standard API (and this won't conflict with $metadata as it's also generated based on the routes). when a route wasn't found, then the middleware (as same as the RazorViewEngine) could show the route it tried and failed to find a controller related to that.
but now it's all guess-work because you don't know even a controller is an OData controller or not, you don't know the action method is discovered or not, and why even not, unless you debug the library (which no one ever should do that).
- Developer writes EDM model.
- Library generates EDM model.
- Library generates routes which match EDM model.
- Library looks for controllers/actions which match those routes at runtime.
- Library wires everything up.
my understanding is a little different:
You can test this by duplicating the official Asp.net sample, remove the controllers, then request for ~odata and see the odata routes. it's empty though the EDMS and everything else is in place.
though what I mentioned with what you have in mind could be combined and still we get pre-generated routes for those EDMs without any need for the developer to create all those controllers.
What if the developer specified routes that are non OData compliant? Are they ignored? If so, will the developer be confused why his routes are ignored?
as mentioned if you have a translator middleware in between you won't end up with this ever.
How does the developer control what is composable etc?
Through a controller. There is nothing in the OData specification that can't be translated/converted to a standard rest API.
How much metadata would need specifying on each route to enable the EDM model to be generated correctly?
You just need a type to generate the EDM. that type can be extracted from the method output type, or specified by attribute. Either way, that's all we need to generate EDM. If more info is needed, still those are the places that should be looked into.
and I think the tooling could be improved greatly (e.g. a code analyser which could perhaps report non-matching routes in the IDE)
To be frank, I think an analyzer for this issue is like using cement to patch a deep wound. those conventions are wrong in the first place. It's PHP like of conventions that shouldn't crossover to the dotnet world. you shouldn't have a convention that dictates how you name this and that. your architecture shouldn't let code without any execution path exist in your application, but now we can end up with the codes that aren't part of OData and not part of asp.net core routing conventions.
the main benefit of typed language is being easy to refactor and build execution error. when a library negates all those benefits in my humble opinion it is doing a very wrong job.
RobTF
commented
3 years ago @mhamri Some good points. I'm still unclear as to the exact workflow you are proposing though; doesn't this risk simply moving the problem around?
In my mind the bullet list you put together seems to mirror what happens today - if the library doesn't find a matching controller/action it ignores it, but isn't that the issue in hand? How does the developer know why their controller/action might have been ignored?
When you say
middleware translates the request to a standard API
Could you elaborate on what translation is taking place? Are you suggesting that the developer writes the routes "their way" and an EDM model should be generated to match those requested routes? I think in principal that sounds good, but arguably it isn't OData anymore, it would become something more akin to a machine readable Swagger wouldn't it? I don't think the EDM stuff is expressive enough to allow any heavy route customization, as it simply defining the names - the OData standard is dictating the routes. The limitations we have might be those imposed by the standard, as opposed to the library.
I totally 100% get your worries and the analogy to PHP, and trust me , I'm as big a hater of "magic conventions" as you are, where development against a library becomes a memory game of simply "knowing" what to name stuff as opposed to working from base principals in an intuitive way. The issue here, however is how you cross the divide of
This is why I suggested a tooling-based solution, as it could analyse the EDM model and say "right, according to the OData spec, based on the EDM you've given me I need /authors, /authors(key), /authors(key)/moreinfo etc." please build me these routes.
There's nothing to stop you deviating and making your own routes via HttpGet and friends, but these will fall (rightly) outside of OData.
mhamri
commented
3 years ago @RobTF I assume we are discussing ideal architectural design and not what's wrong with v8, so all my answers are hypothetical and need to be validated and evaluated, but in rough it's like this
In my mind the bullet list you put together seems to mirror what happens today - if the library doesn't find a matching controller/action it ignores it, but isn't that the issue in hand? How does the developer know why their controller/action might have been ignored?
I refer you to how the razor view engine, when can't find a razor file handles the problem. how razor view engine works, if from a controller you return a view that doesn't exist, it shows you what path it has tried and couldn't find your view.
Samwise, you need to know that a request is targeting an OData controller (more reason to have a translator middleware), but when you don't know that, of course, you can't show a helpful error message.
the ~/metadata should be rendered based on what route is available, then we really won't face this issue ever (but in case a user wanted to override something but failed)
it would become something more akin to a machine readable Swagger wouldn't it
Exactly, that's what I have in mind. If we use the built-in capabilities of asp.net core, then the ~/metadata could be a conversion of what we have in swagger. the only gap in this way becomes container and namespace that could be set by some attributes.
Let's assume we have one controller that returns author. this is equivalent to having an EDM for that type. Without the need of defining it in the startup.cs, it can be generated on the request, it can be cached. (now even the .net core 5 support build-time code generation, just imagine at build time we parse all the controller that are opted-in as OData controller and genrate EDM based on their return types)
and let us say you receive author(key)/moreinfo. you have one controller and that controller is returning the author as the IQueryable, the rest should be a projection and how a normal EDM works.
let us say you add another method that accepts a key and returns an author. then it's more specific (same as how styles apply in html, more specific take higher priority) then that method should be called and projection is applied on top of its result.
Instead of defining one EDM in the startup, you could define a controller and your return type become your EDM. all automatically.
and you as the developer don't need to know anything about the routes unless you want to override one. if you want to override one of the routes. Then you can just define a controller and a route that matches that route, and just because it's more specific, then it should overwrite the default OData routing.
so if you have a request to author(key)/moreinfo in your route, then a controller with the route author/{id}/moreinfo should be able to override it.
your ~metadata generate (akin to swagger) based on any controller opted-in as OData controller.
Developer feeds in EDM configuration it's discoverd based on opt-in and return type of action methods in controller
OData reads EDM and calculates "x routes" we don't need to implement all the routes one by one. the action methods that match the request define the base edm. rest works as how an edm conversion works.
Developer must now make "x routes" to make OData standard happy they don't need to do that if they don't want to override one
How does developer know what "x routes" are?!? they don't need to implement all of the routes. having one base route is enough. (like
~/author)
As I mentioned OData could become a heroine for any API, but with the current implementation, it's something that can't coexist with an API, and needs its own routing.
I have a more specific example at the bottom of the article that handles more stuff, appreciates it if you have a look into that.
xuzhg
commented
3 years ago @mhamri @RobTF Thanks for sharing your thoughts. Besides, these days I am working to use ASP.NET Core attributes to support OData attribute routing, that's to get rid of [ODataRoute] and [ODataRoutePrefix]. The basic scenario works fine. I'd like to invite you to try it and share any thought on it.
The changes can be found https://github.com/OData/AspNetCoreOData/commit/1326b7d77af539a47504754a5a41e401c2097a02.
The changes should be in the latest nightly: https://www.myget.org/feed/webapinetcore/package/nuget/Microsoft.AspNetCore.OData/8.0.0-Nightly202103110707
RobTF
commented
3 years ago @xuzhg this looks interesting, thanks for the update (except it breaks all the existing code :*( ).
Just a couple of questions;
xuzhg
commented
3 years ago Just a couple of questions;
- Are there any specific instructions on porting from the older builds?
- Is this change likely to be permanent, or are you just playing around at the moment?
1) When it's robust enough, we'd like to create a blog about the porting and usage. 2) I think the direction (move to using [HttpGet...] for attribute routing) is permanent.
I have another idea is that: (https://github.com/OData/aspnetcoreodata/tree/NewAttributeRotuing)
a) I don't register any model in startup.cs b) in the action of controller, for example
[ODataModelProvider("odata")]
[HttpGet("Customers/{key}"]
public IHttpAction GetCustomers()
{
}
Hi, I reviewed the v8-preview code and I really didn't like the new changes coming with v8. I wrote a thorough article about it you can find it here
appreciate it if you consider it before releasing v8.
in abstract: