json-api-dotnet / JsonApiDotNetCore

A framework for building JSON:API compliant REST APIs using ASP.NET and Entity Framework Core.
https://www.jsonapi.net
MIT License
679 stars 159 forks source link

Overriding the controller methods #483

Closed sadedil closed 5 years ago

sadedil commented 5 years ago

Description

Hello, i've a question (and i think there is a problem in here) I've a controller named GamesController, and i want to override the patch method to change some properties on the fly. So i've change the controllers content similar to this.

Controller

    [Authorize]
    [Route("jsonapi/[controller]")]
    public class GamesController : JsonApiController<Game>
    {
        public GamesController(
            IJsonApiContext jsonApiContext,
            IResourceService<Game> resourceService,
            ILoggerFactory loggerFactory)
            : base(jsonApiContext, resourceService, loggerFactory)
        {
        }

        [HttpPatch("{id}")]
        public override Task<IActionResult> PatchAsync(int id, [FromBody] Game entity)
        {
            //Force to change some property of entity <------ question is here
            entity.Title = $"{entity.Title} {DateTime.Today.ToShortDateString()}";
            return base.PatchAsync(id, entity);
        }
    }

Sample request body

{
 "data": {
  "type": "games",
  "attributes": {
       "title": "My game"
     }
   }
}

Actually it's working without giving an error, but not accepting the value change.

Summary

Environment

wisepotato commented 5 years ago

This might be a quirk from the [FromBody] function. These kinds of additions are also not within the scope of the controller. Try and do this in the service, it is more suited for that.

sadedil commented 5 years ago

Hi @wisepotato, without [FromBody], content of the entity wasn't filled. It had default values.

By the way the example is simplified because of easily reproduce the scenario. Not about the writing clean or elegant solution. Also I think decoupling business from the controller is good thing too.

wisepotato commented 5 years ago

I was just thinking out loud, because i couldnt find anything wrong with your scenario, have you tried debugging to see if your code is actually being hit/ your controller is registered correctly? Can you write a test to ensure something is wrong? If so, it might be JADNC, but I doubt it..

Writing out a test probably helps.

milosloub commented 5 years ago

Sorry for previous post, there was mistake. For updating values there is JsonApiContext (IIJsonApiContext param of your Controller), which is holder of changed attributes from PATCH request.

You should store jsonApiContext to private variable and do this

        [HttpPatch("{id}")]
        public override Task<IActionResult> PatchAsync(int id, [FromBody] Game entity)
        {
            //Force to change some property of entity <------ question is here
            var titleAttr = _jsonApiContext.AttributesToUpdate.FirstOrDefault(i => i.Key.InternalAttributeName == nameof(Game.Title));
            if (titleAttr.Key != null)
                _jsonApiContext.AttributesToUpdate[titleAttr.Key] = $"{entity.Title} {DateTime.Today.ToShortDateString()}";

            return base.PatchAsync(id, entity);
        }
wisepotato commented 5 years ago

Why not do it in the repository layer?

milosloub commented 5 years ago

It can be everywhere - Controller, Service, Repository, but problem is the same - during PATCH you have to work with IJsonApiContext, not with input model/resource param. It's about some convention, where to put changes like that. Personally I would place it into Service - it's kind of business logic I would say

sadedil commented 5 years ago

Hi @milosloub, your suggestion worked perfectly. Thanks for information 👏

For updating values there is JsonApiContext (IIJsonApiContext param of your Controller), which is holder of changed attributes from PATCH request.

After all, working with the object directly seems a little strange to me. I didn't analyze all the source code, but I'm curious, is there any limitations to work with the entities instead of objects? Maybe there is a future milestone about the change this approach?

Do you have extra information about this subject?

milosloub commented 5 years ago

There is the limitation comming from HttpPatch itself. HttpPatch reflects only truly changed attributes. For example, you have Game entity, you send PATCH on your title attribute, then you face several problems: 1) input Game model holds only Title property filled and all the others are null (or default values). If you send this to DB via context.SaveChanges(), you will reset all the other properties. 2) you can send NULL to your title property and then you are not able to distinguish if it's comming from PATCH or if it's default value when Game instance is created. 3) you will face the same problem in case of navigation properties (relationships) -> you are not able to distinguish wheter you want to unset relationship or if it is just default value comming from new instance.

That's the reasons why AttributesToUpdate and RelationshipsToUpdate exists. .NET Core itself uses JsonPatchDocument https://dotnetcoretutorials.com/2017/11/29/json-patch-asp-net-core/ but it's coupled with application/json-patch+json standard and it's quite overkill when api needs only application/vnd.api+json

sadedil commented 5 years ago

Hmm... I've missed that point. Thanks for the explanation @milosloub.