Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is it possible to use different EDM models for different users and different actions in the same controller? #277

Open
anranruye opened this issue Aug 17, 2021 · 18 comments
Assignees
Labels
investigated question Further information is requested

Comments

@anranruye
Copy link

I have some real-world problems with odata web api.

  1. We want to hide some sensitive fields in the entity class. Normal users or guests should not get these information. However, the resource owner or the system administrators still have the permission to get these fields. So I can not just remove the fields from the edm model.
  2. We want to allow the system administrators to use query options which may affect performance. But normal users should not perfrom these high-risk options.
  3. We want to use different models for CRUD actions in the same controller. For example, the CreateTime property should be returned by Get method, but it should not be included in the input model of Post method(obviously this field should be created by the service, not provided by the user). In traditional web apis, we can use a dto class to avoid over-posting. I don't know how to avoid over-posting in odata world.
  4. For performance, we want to hide some long fields(long text or blob) in the Get method which return entity collection, but still expose these fields in the Get method which return a single entity instance.

I worked more with aspnetcore odata 7.x version rather than 8.x version, so I may miss some key progresses in the new version. If so, I will appreciate if anyone can give suggestions with the new version. Otherwise, I expect my problems can be sovled by 8.x version in the future.

@Sreejithpin Sreejithpin added the question Further information is requested label Aug 17, 2021
@xuzhg
Copy link
Member

xuzhg commented Aug 17, 2021

@anranruye Both 7x and 8x supports multiple routes. In each route, you can set different models. You can try this.

Here's the example: https://github.com/OData/AspNetCoreOData/blob/master/sample/ODataRoutingSample/Startup.cs#L61-L62

@xuzhg xuzhg self-assigned this Aug 17, 2021
@anranruye
Copy link
Author

anranruye commented Aug 18, 2021

@xuzhg Thank you.

In the past two years, I thought about many approaches to solve the problems: using two or more entity classes for one db table(table splitting in EF core side)(thus they can have different edm models), using inheritance hierarchy in one db table(similar to the previous one), using different DbContext classes, etc. Multiple routes is one among the approaches, but I don't like it and never tried this. However, if we want to solve these problems by using different EDM models, you are right, multiple routes is the answer, and I may try it some day.

Multiple routes is not bad, but I don't think it's the best practice for all the four scenarios. For example, for the third one, I prefer to use a dto class directly which doesn't follow the odata way(and this really works in odata web api v7.5). What do you think is the best practice to solve my issues?

Another more issue, for the fourth scenario, the fields which is removed from odata edm model will still be queryed from the database if they are parts of the ef core DbContext model(when there is no $select query option). This happened for odata web api v7 and may still cause performance issues(see dotnet/SqlClient#593). To avoid this, I have to manully apply a projection or use a different DbContext which doesn't include these fields. Is this improved in odata web api v8?

@xuzhg
Copy link
Member

xuzhg commented Aug 18, 2021

@anranruye Let me know any help for multiple routes.

I tried to build the 8 more extensible. I think we can figure out the "best practice". In your third one, "CreateTime " should include in "Get", but not in "Post". Right? I am confused that in the "post" request, the client can decide on which property should be included. OData accepts the scenario which has with "missing" property. I'd love to hear more ideas from you about this.

Most of the query codes between 7 and 8 are the same. But, we do want to improve the query. Would you please share me the related issue in OData web API 7?

@anranruye
Copy link
Author

@xuzhg

Yes, for the "Post" action, it's ok for the client to decide to not include the "CreateTime". And even if the client does provide the "CreateTime" property, the server side will override it with the actual time.

However, things will change for "Put" or "Patch" action. If we do not handle the input paramter properly, there is a chance for the client to wrongly change the "CreateTime" property.

If we can always trust the client, then everything is simple and the discusstion can be over. Unfortunately, that's not true.

Another thing we consider is the api documentation. We want to explicitly romove the "CreateTime" parameter form the documentation of the "Post" and "Put"/"Patch" actions.

As I know, many developers are only intrested in the query part of odata and don't care about the other functions. Some developers only implement "Get" methods in their odata controllers, and write all other methods(Post, Put, Patch, actions and functions) in a traditional way(in another traditional controller which inherits from ControllerBase).


For the fourth scenario, I will open a new issue to describe the details.

@dxrdxr
Copy link
Contributor

dxrdxr commented Sep 12, 2021

@anranruye
For the problem of over-posting, I don't think it is wise to trust the client.

In my scenarios on POST and PUT, I simply do not copy the untrusted properties into the classes of my EFCore model. Functionally the two are effectively the same, but a POST is a new followed by a "safe copy", then an Add and SaveChanges. The Put is a Find followed by the "safe copy", then an Update and SaveChanges.

PATCH is a little bit trickier, to prevent over-posting you have to not apply the parts of the Delta that are not "safe". I recently submitted a PR #290 (in 8.0.2, originally from WebAPI) that allows you to changed the UpdatableProperties in an Entity so that the deltaEntity.Patch(existingEntity) does not change those unsafe properties. For each of my entities I maintain a list of "safe" properties that I set as Updatable before I patch. In your case of "CreateTime" you can just leave it off the list. If you have different roles for different users you could just use a different updatable properties list for each role. I use this per entity updatable properties list for my "safe copy" in POST and PUT I described above.

As a result I have the same EDM model but certain properties are Nullable so the client doesn't have to send them and depending on the role of the caller the controller may ignore them. Depending on the documentation, I either just don't document the property or I call out that the controller may (or will) ignore it depending on the caller.

@anranruye
Copy link
Author

anranruye commented Sep 12, 2021

@dxrdxr Thank you.

I think the skills which you use are reliable but still not the best.

I have two approaches.

One is that I mentioned above, I just use DTO classes instead of entity classes directly, as the traditional web api endpoints. The MVC framework will fall back to use newtonsoft/system.text.json formatters if the odata formatter can not handle the input. And for a patch endpoint, I don't use Delta class, I have special dto classes which will track the changed properties and I map the dto class to entity class with the help of AutoMapper.

Another approach is to use multiple routes(I never used the one). We can use different route templates for different http methods. Then endpoints can have different EDM model. And we can use a middleware to rewrite the request url, the user will feel they are using the same route template when request different endpoints.

@anranruye
Copy link
Author

@xuzhg any new ideas?

mattperdeck added a commit to mattperdeck/AspNetCoreOData that referenced this issue Oct 27, 2022
…ow return null if the field should not be serialized at all (as in, ignored)
mattperdeck added a commit to mattperdeck/AspNetCoreOData that referenced this issue Oct 27, 2022
…Newtonsoft.Json and System.Text), so they return null if the field has the JsonIgnore attribute
mattperdeck added a commit to mattperdeck/AspNetCoreOData that referenced this issue Oct 27, 2022
…ull if the field has the JsonIgnore attribute
mattperdeck added a commit to mattperdeck/AspNetCoreOData that referenced this issue Oct 27, 2022
xuzhg pushed a commit that referenced this issue Dec 9, 2022
* #277 change contract of IPropertyMapper.MapProperty, so it can now return null if the field should not be serialized at all (as in, ignored)

* #277 update implementation of both default MapProperty methods (Newtonsoft.Json and System.Text), so they return null if the field has the JsonIgnore attribute

* #277 update unit tests to reflect that MapProperty now returns null if the field has the JsonIgnore attribute

* #277 add unit tests for Newtonsoft Json version of MapProperty
@habex-ch
Copy link

This is an old issue, but I am also really interested in solving the third one (different entities for CRUD).
What is best practice for different entities (CreateDTO, UpdateDTO, ...)?

@julealgon
Copy link
Contributor

What is best practice for different entities (CreateDTO, UpdateDTO, ...)?

Isn't the best practice not to have different representations for the same entity in the first place? And that's not even for OData necessarily, but REST itself.

Maybe you can elaborate why you want to use different models for different operations.

@habex-ch
Copy link

@julealgon
Well, let's say I have a book shop.
There are different people to modify books. An administrator can modify all properites of a book a normal user can only modify the description and the title of the book but not the ISBN number.

Another example: I am a book publisher.

  1. An author can upload (create) a new book with its title and the description.
  2. A clerk sees the new book and can now change the title and the description AND can add the ISBN number.

In both examples the isbn number should only be allowed to set in certain scenarios.

@julealgon
Copy link
Contributor

@julealgon Well, let's say I have a book shop. There are different people to modify books. An administrator can modify all properites of a book a normal user can only modify the description and the title of the book but not the ISBN number.

Another example: I am a book publisher.

  1. An author can upload (create) a new book with its title and the description.
  2. A clerk sees the new book and can now change the title and the description AND can add the ISBN number.

In both examples the isbn number should only be allowed to set in certain scenarios.

@habex-ch what you are describing to me seems more like "input validation" than needing 2 distinct models. At first glance, if I was in a situation such as the ones you described, I'd probably create different validation profiles, one for each scenario, and apply the validation profile according to the logged in user or something like that.

The underlying model would still be the same object (as would the endpoint itself).

Another alternative to validation (using something like FluentValidation) would be to actually use resource-based authorization with custom AuthorizationRequirements to touch each field etc. I've used it to prevent access to resources, but not for limiting access to certain fields, but I'm sure it could be used for that as well.

@habex-ch
Copy link

habex-ch commented Dec 22, 2022

@julealgon Maybe it is more about input-validation.
I would really like to use the default ASP.NET Core validation with data annotation.

public class BookModel 
{
        [Required()]
        [MinLength(2)]
        [MaxLength(250)]
        public string Title { get; set; }

        [Required()]
        [MaxLength(13)]
        public string ISBN { get; set; }

        ...
}

Is there no way to use them with ODATA?

@julealgon
Copy link
Contributor

Is there no way to use them with ODATA?

Model validation works just fine with OData, especially now in OData v8 with attribute routing, since you can use the [ApiController] behavior and have model validation happen automatically ([ApiController] requires that you use attribute routing). More on ApiController here (strongly recommended for any pure REST API and can also be applied globally to all controllers):

The problem that is specific to the native attribute-based validation, is that it is not well-suited for conditional validation... it would be hard for example to have a field "required for one user, and optional for another" using the standard attributes. You'd either have to create custom attributes for that, or implement IValidatableObject to add the custom logic. But even then, that approach also isn't very DI-friendly.

I mentioned FluentValidation because you can actually create multiple validation "rulesets" with different rules for the same model, and then call each one depending on your scenario. If you wanted to, you could even inject the current request into the validator itself and decide what to do based on that.

@habex-ch
Copy link

Model validation works just fine with OData, especially now in OData v8 with attribute routing, since you can use the [ApiController] behavior and have model validation happen automatically ([ApiController] requires that you use attribute routing). More on ApiController here (strongly recommended for any pure REST API and can also be applied globally to all controllers):

So if I like to use the default DataAnnotation-validation I can't use ODATA and use a normal "Rest API" Controller (including ApiController-Attribute of course) and having different actions in my controller with different models like

...
[ApiController]
public class BookController
{
        ....
        public async Task<IActionResult> Create(CreateBookModel book)
        {
                ....
        }

        ....
        public async Task<IActionResult> Create(ChangeBookModel book)
        {
                ....
        }


}
public class CreateBookModel 
{
        [Required()]
        [MinLength(2)]
        [MaxLength(250)]
        public string Title { get; set; }

        ...
}
public class ChangeBookModel 
{
        [Required()]
        [MinLength(2)]
        [MaxLength(250)]
        public string Title { get; set; }

        [Required()]
        [MaxLength(13)]
        public string ISBN { get; set; }

        ...
}

The problem that is specific to the native attribute-based validation, is that it is not well-suited for conditional validation... it would be hard for example to have a field "required for one user, and optional for another" using the standard attributes. You'd either have to create custom attributes for that, or implement IValidatableObject to add the custom logic. But even then, that approach also isn't very DI-friendly.
I mentioned FluentValidation because you can actually create multiple validation "rulesets" with different rules for the same model, and then call each one depending on your scenario. If you wanted to, you could even inject the current request into the validator itself and decide what to do based on that.

Ok, so ODATA + default DataAnnotation-validation = bad

In sum

  1. When using ODATA, I do only have a single model and all the validation must happen inside my action with things like FluentValidation. And I have to deal/ignore additional properties (as is mentioned in ODATA Standard
  2. When I want to use the default DataAnnotation-validation I must fall back on a normal Rest-API.

Right?

Maybe ODATA for commands (Create/Update/Delete) is not the best approach for me. I will probably using it only for read query (Read).

Thank you very much for your time.

@julealgon
Copy link
Contributor

So if I like to use the default DataAnnotation-validation I can't use ODATA and use a normal "Rest API" Controller (including ApiController-Attribute of course) and having different actions in my controller with different models like

OData represents an entity with a unified model. If you want to use multiple models, you would have to have multiple entities, or consider using custom OData Actions (which as far as I know would accept any arbitrary models you want).

Ok, so ODATA + default DataAnnotation-validation = bad

Not at all. DataAnnotations work just fine with OData.

What is creating a problem for you here is having multiple different representations of the same entity, which is not only non-standard for OData, but also for normal REST APIs. Having different models to represent creations/updates/details is widely considered a bad practice.

  1. When using ODATA, I do only have a single model and all the validation must happen inside my action with things like FluentValidation. And I have to deal/ignore additional properties (as is mentioned in ODATA Standard

For the most part yeah. OData is just enforcing a well-known good practice here that tells API designers to always have consistent representations for the same entity no matter what action is being performed, meaning: use a single model.

  1. When I want to use the default DataAnnotation-validation I must fall back on a normal Rest-API.

As mentioned, DataAnnotations work just fine with OData. The reason I suggested FluentValidation instead in your case is that it would allow you to have different validation profiles for the same model (instead of having different models each with its own validation). DataAnnotations would require heavy customization to support different validation profiles.

Maybe ODATA for commands (Create/Update/Delete) is not the best approach for me.

I'd suggest you reconsider your design actually.

As I said above, having all standard operations use the same payload/model to represent the objects is a good practice for REST APIs in general, and it would do you more good to try to follow that instead of having various separate models.

This is independent of OData.

@habex-ch
Copy link

Thank you very much for your answers/help.

OData represents an entity with a unified model. If you want to use multiple models, you would have to have multiple entities, or consider using custom OData Actions (which as far as I know would accept any arbitrary models you want).

I tried using custom OData actions, but I couldn't get it working so that I could use it like

[ApiController]
public class BookController
{
        ....
        public async Task<IActionResult> Create(MyCustomCreateBookModel book)
        {
                ....
        }
        ....
}

It only works with action like the following. And I don't like that because there is no DataValidation through the normal middleware possible:

[ApiController]
public class BookController
{
        ....
        public async Task<IActionResult> Create(ODataActionParameters parameters)
        {
                MyCustomCreateBookModel book = (MyCustomCreateBookModel ) parameters["book"];
                ....
        }
        ....
}

Sure, I could use TryValidateModel(book); for valiation, but I really don't like that approach.

Ok, so ODATA + default DataAnnotation-validation = bad

Not at all. DataAnnotations work just fine with OData.

Most of the DataAnnotations may work with ODATA but not attributes like RequiredAttribute which may be required only in certain actions but not in all (e.g. ISBN in my example above). So I would still have to validate those things myself (FluentValiation can not work with DataAnnotations).

What is creating a problem for you here is having multiple different representations of the same entity, which is not only non-standard for OData, but also for normal REST APIs. Having different models to represent creations/updates/details is widely considered a bad practice.

It seems to be bad practice in combination with ODATA. Because ODATA only want to use a single model for each entity. But I don't think it is a bad practice in combination with normal REST API or else it wouldn't be like this in one of the most used CleanArchitecture templates existing which implements CQRS where each command is a different Dto-object.

  1. When I want to use the default DataAnnotation-validation I must fall back on a normal Rest-API.

As mentioned, DataAnnotations work just fine with OData. The reason I suggested FluentValidation instead in your case is that it would allow you to have different validation profiles for the same model (instead of having different models each with its own validation). DataAnnotations would require heavy customization to support different validation profiles.

May be different FluentValidation profiles would help wenn I would have a single model.
But then the property mapping would be more work, because I couldn't use AutoMapper for mapping the model to the entity anymore and would have to write this code myself. Which would be a lot of boring work.
Or maybe I could map from the odata-model to a command and then from the command to the entity. Like

  1. Frontent: UserInput => Command => OData-Model
  2. Backend: OData-Model => Command => Entity

I'm not sure if this would be a good approach. But so I could use OData with a single model as my API.
What do you think?

Maybe ODATA for commands (Create/Update/Delete) is not the best approach for me.

I'd suggest you reconsider your design actually.
I have at the moment an single tier application with 3 layers which I have to split it into a multi-tier (frontend/backend) application. I'm now thinking about the best solution for splitting it. If I could use ODATA for this, I would be more than happy.

As I said above, having all standard operations use the same payload/model to represent the objects is a good practice for REST APIs in general, and it would do you more good to try to follow that instead of having various separate models.

This is independent of OData.

My current applications also have inherited entities which make the whole thing even more complicated.

@julealgon
Copy link
Contributor

I tried using custom OData actions, but I couldn't get it working ...
It only works with action like the following. And I don't like that because there is no DataValidation through the normal middleware possible:

[ApiController]
public class BookController
{
        ....
        public async Task<IActionResult> Create(ODataActionParameters parameters)
        {
                MyCustomCreateBookModel book = (MyCustomCreateBookModel ) parameters["book"];
                ....
        }
        ....
}

Indeed, you are correct. The way parameters are passed to custom actions is quite a bit unwieldy, totally agree with you there.

I'll go ahead and open a different issue to discuss the possibility of allowing arbitrary body payloads with custom actions; I feel like that would simplify the framework considerably.

Most of the DataAnnotations may work with ODATA but not attributes like RequiredAttribute which may be required only in certain actions but not in all (e.g. ISBN in my example above).

Yeah I meant in the sense that validating a model using DataAnnotations works fine in conjunction with OData. The need for multiple models though is indeed a limitation, but not necessarily tied to validation per se.

It seems to be bad practice in combination with ODATA. Because ODATA only want to use a single model for each entity. But I don't think it is a bad practice in combination with normal REST API or else it wouldn't be like this in one of the most used CleanArchitecture templates existing which implements CQRS where each command is a different Dto-object.

I guess this enters into a fairly subjective realm, but I still believe the expectation for a "standard" REST API is to represent same entities with same representations across GET/POST/PATCH/PUT, and this doesn't depend on the API being OData or not. If that CleanArchitecture template is not following that pattern, I'd consider it a bad template.

(PS: I'm also not a big fan of the CleanArchitecture project splitting approach but that's a whole other topic)

May be different FluentValidation profiles would help wenn I would have a single model. But then the property mapping would be more work, because I couldn't use AutoMapper for mapping the model to the entity anymore and would have to write this code myself. Which would be a lot of boring work.

This is indeed true. However, I'd like to bring another topic to your attention: using AutoMapper to map from DTO -> Domain (i.e. reverse projection) is also widely considered a bad practice (even though it is admittedly used a lot for that purpose). The author of the library himself recommends using it only for actual projections/flattening from Domain to DTO, but not the other way around.

I'm not saying you should just change everything you are doing. Just figured I'd bring this extra tidbit so you could consider it in your design.

Or maybe I could map from the odata-model to a command and then from the command to the entity. Like

  1. Frontent: UserInput => Command => OData-Model
  2. Backend: OData-Model => Command => Entity

I'm not sure if this would be a good approach. But so I could use OData with a single model as my API. What do you think?

IMHO this would be an ideal design in your case. I understand one can have the commands themselves be directly represented in the API surface, but I've always found that to be a bad idea: commands generally don't quite match what the actual data payload would look like, and this creates massive coupling between the commands and the API contract.

By transforming the payloads into the associated commands I think would give you a lot more freedom to define the API in a clean and more consistent way, without abandoning the idea of having a command pattern take care of actual execution.

I've used this very approach a few times myself to great effect. It adds a bit more boilerplate to map from the API surface to the commands, but in the end you get much better separation.

This is especially true when you want your commands to be part of your domain and be shared across multiple "customer facing systems": say you have both your REST API as well as some WPF app, or an Azure Function, calling the same command infrastructure: the final "layer" becomes just a shell that redirects the calls into your main logic, converting from whatever the input in that shell is, to the one in your domain.

My current applications also have inherited entities which make the whole thing even more complicated.

OData supports inheritance surprisingly well. I wouldn't say the fact that you have inheritance in place would be a detriment to continue to use OData at all actually. You can get some extremely clean results if the modelling is correctly done.

@habex-ch
Copy link

I'll go ahead and open a different issue to discuss the possibility of allowing arbitrary body payloads with custom actions; I feel like that would simplify the framework considerably.

That are good news. I totally agree with you. I would make the framework much more user friendly.

This is indeed true. However, I'd like to bring another topic to your attention: using AutoMapper to map from DTO -> Domain (i.e. reverse projection) is also widely considered a bad practice (even though it is admittedly used a lot for that purpose). The author of the library himself recommends using it only for actual projections/flattening from Domain to DTO, but not the other way around.

I am aware of that problem. I don't validate domain models at the moment because my applications do not have much business logic and in my applications the input validation is sufficient.

IMHO this would be an ideal design in your case. I understand one can have the commands themselves be directly represented in the API surface, but I've always found that to be a bad idea: commands generally don't quite match what the actual data payload would look like, and this creates massive coupling between the commands and the API contract.

Thank you very much for your opinion. It helps me a lot to find a good design for my multi-tier application.

I've used this very approach a few times myself to great effect. It adds a bit more boilerplate to map from the API surface to the commands, but in the end you get much better separation.

But how do I separate multiple update commands from each other.
I tried multiple things but nothings works well. I tried "actions", but with them, I had to encapsulate the entity (e.g. book) in a different json structure like

{
  "book": {
    "id": ...
    "title": ...
  }
}

But the ODataActionParameters do not support inheritance.

Or is there a better way to separate multiple update commands?

OData supports inheritance surprisingly well. I wouldn't say the fact that you have inheritance in place would be a detriment to continue to use OData at all actually. You can get some extremely clean results if the modelling is correctly done.

This seems to be the case for the "default" entitysets (Get/Post/Delete).
But not so much for Patch/Put. I had either implement separate actions or explict add multiple [HttpPatch] attributes to a single action. One for each inherited class with different path-definitions. Like that where Novel derives from Book:

[HttpPatch]
[HttpPatch("odata/Book({key})/Common.Novel")]
public async Task<IActionResult> Patch([FromRoute] int key, [FromBody] Delta<Book> delta)
{
    ...
}

Or is there a simpler way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigated question Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants