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

Poor error response when using ModelState Validation with [ApiController] #1224

Closed
Alex-Doms opened this issue Dec 8, 2022 · 9 comments · Fixed by #1246
Closed

Poor error response when using ModelState Validation with [ApiController] #1224

Alex-Doms opened this issue Dec 8, 2022 · 9 comments · Fixed by #1246

Comments

@Alex-Doms
Copy link

SUMMARY

Hello, i try to add some data annotations on my resources. On my first tests, the ValidateModelState seems fired well when an attribute is not correct, but in the response, i don't have any details.

STEPS TO REPRODUCE

Resource :

 public class PostIt : Identifiable<int>
    {
        
        [Attr]
        [Required]
        public bool Actif { get; set; }

        [HasOne]
        public DPIDentaire DPIDentaire { get; set; }
        [Attr,Required]
        public DateTime Date { get; set; }

        [Attr]
        [MaxLength(2, ErrorMessage ="Error")] 
        public string Message { get; set; }

        [Attr]
        [Required]
        public string RefOperateur { get; set; }


    }

Request :

curl --location --request PATCH 'http://localhost/v1/post-its/835405' \
--header 'Content-Type: application/vnd.api+json' \
--header 'Authorization: Bearer token' \
--data-raw '{
    "data": {
        "type": "postIts",
        "id": "835405",
        "attributes": {
            "actif": false,
            "message": "HELLO POST IT",
            "refOperateur": "1"
        }
}
}'

Response :

{
    "links": {
        "self": "http://localhost/WS_ApiGalaxie/v1/post-its/835405"
    },
    "errors": [
        {
            "id": "1af709c0-ccfa-45ec-8f75-7fc7470e7fc4",
            "links": {
                "about": "https://tools.ietf.org/html/rfc7231#section-6.5.1"
            },
            "status": "400",
            "title": "One or more validation errors occurred.",
            "meta": {
                "stackTrace": [
                    "JsonApiDotNetCore.Errors.UnsuccessfulActionResultException: Exception of type 'JsonApiDotNetCore.Errors.UnsuccessfulActionResultException' was thrown.",
                    "   at string JsonApiDotNetCore.Serialization.Response.JsonApiWriter.GetResponseBody(object model, HttpContext httpContext) in C:/projects/jsonapidotnetcore/src/JsonApiDotNetCore/Serialization/Response/JsonApiWriter.cs:line 84"
                ]
            }
        }
    ]
}

I should have the detail where my "message" attribute is listed with my custom error.
Where did I miss something?

VERSIONS USED

  • JsonApiDotNetCore version: 5.1.0
  • ASP.NET Core version: 6
  • Entity Framework Core version: no (custom)
  • Database provider: oracle
@bkoelman
Copy link
Member

bkoelman commented Dec 8, 2022

Hi @Alex-Doms, thanks for asking.

I've created #1225 in an attempt to reproduce the problem, unfortunately without success.

Can you share your controller code? Even better would be if you could extend the code from that PR to reproduce the response you're getting.

@Alex-Doms
Copy link
Author

Hi bart, thanks for your help again ,

I found this in debug :
image

Detail is null, but i have an error list with my message, does it normal?

Found at GetResponseBody call in JsonAPiWritter
image

Mycontroller is a simple class inherited from BaseJsonApiController

    [ApiController]
    [DisableRoutingConvention]
    [ApiVersion(versionAPI.Version)]
    [ApiExplorerSettings(GroupName = "dentaire-v1")]
    public class BaseControllerDentaire<TResource, TId> : BaseJsonApiController<TResource, TId>, IDBContext
        where TResource : class, IIdentifiable<TId>
    {

        protected IServiceDBContext _ServiceDBContext;

        protected readonly ILogger<BaseControllerDentaire<TResource, TId>> _logger;
        protected readonly IDiagnosticContext _diagnosticContext;

        public BaseControllerDentaire(IServiceDBContext serviceDBContext, IJsonApiOptions options, IResourceGraph resourceGraph,
        ILoggerFactory loggerFactory, IResourceService<TResource, TId> resourceService, IDiagnosticContext diagnosticContext)
            : base(options, resourceGraph, loggerFactory, resourceService)

        {
            _ServiceDBContext = serviceDBContext ?? throw new ArgumentNullException(nameof(serviceDBContext));
            _logger = loggerFactory.CreateLogger<BaseControllerDentaire<TResource, TId>>() ?? throw new ArgumentNullException(nameof(loggerFactory));
            _diagnosticContext = diagnosticContext ?? throw new ArgumentNullException(nameof(diagnosticContext));

        }
    

        //action de modification
        [ApiExplorerSettings(IgnoreApi = false)]
        [HttpPatch("{id}")]
        public override async Task<IActionResult> PatchAsync(TId id, [FromBody] TResource resource, CancellationToken cancellationToken)
        {
            return await base.PatchAsync(id, resource, cancellationToken);
        }
}

Where the model state validation is called?

@bkoelman
Copy link
Member

bkoelman commented Dec 8, 2022

Thanks, that's helpful.

The reason you're hitting this is because your controller contains [ApiController]. I've pushed a commit to the PR that reproduces the issue and corrects the error response you're getting.

I'm not opposed to releasing that fix (assuming tests are added first). However, the big downside of using [ApiController] is you'll lose the information that's normally in source/pointer, which reliably indicates where in the request body the issue occurred. For a simple request with a few fields, that's probably less of a concern. But in an atomic:operations request that usually spans many changes, it becomes essential to know in which operation the problem is, and when updating multiple relationships, which relationship is invalid.

By using [ApiController], ASP.NET is basically saying: "don't handle validation errors, we'll produce a response for that". As a general approach, that's better than nothing. But JADNC needs to adhere to the JSON:API spec, so with ProblemDetails it has to kinda infer what happened, based on insufficient information. So the code that we have in place for that is just a fallback, yet doesn't provide the quality level users get when leaving it up to JADNC to produce the error response.

Do you have a specific reason to use [ApiController]? If not, I'd recommend removing it.

@bkoelman bkoelman changed the title Validate Poor error response when using ModelState Validation with [ApiController] Dec 8, 2022
@bkoelman bkoelman added the bug label Dec 8, 2022
@Alex-Doms
Copy link
Author

Hum, well i'm using it because i'm migrating old API REST to JSONAPI, and we used SwaggerUI.
If i remove [ApiController] my routes will are not visible, but right now my attributes works.

@Alex-Doms
Copy link
Author

I missed something else : the attribute [required ]does not seem to be checked as well.
In my example, the Date field is missing, but the ModelState validation ignored it.

@bkoelman
Copy link
Member

bkoelman commented Dec 8, 2022

Hum, well i'm using it because i'm migrating old API REST to JSONAPI, and we used SwaggerUI.
If i remove [ApiController] my routes will are not visible, but right now my attributes works.

I see. Unfortunately, Swagger doesn't work very well with JADNC, regardless of [ApiController] usage. The effort to enable that is tracked at #1046.

I missed something else : the attribute [required ]does not seem to be checked as well.
In my example, the Date field is missing, but the ModelState validation ignored it.

That's intentional. PATCH partially updates a resource, so any fields omitted in the request body are left unchanged. You should get an error in POST for that one, though.

@Alex-Doms
Copy link
Author

Ok, well noted! Thanks again!

@Alex-Doms
Copy link
Author

Hi, i have a last question, i tried a post request, with an Identifiable<string> class.
I have this error on my call :

  "errors": [
        {
            "id": "f4696090-385b-493c-ab70-ed771f99a44b",
            "status": "422",
            "title": "Input validation failed.",
            "detail": "The Id field is required.",
            "source": {
                "pointer": "/data/id"
            }
        }

If i overrive the ID with this in my class :
public override string? Id { get => base.Id; set => base.Id = value; }
The modelstate will be valid now. Do i have to do this on all my Identifiable entities?

@bkoelman
Copy link
Member

bkoelman commented Dec 9, 2022

Why would you specify string for TId, only to undo its effects by overriding with string? instead? Seems like you should just use Identifiable<string?>, assuming you want Id to be optional.

Of course, then you'll need to assign a value server-side in case null is sent. What people usually do with string-based Ids, is require the client to send the value by using Identifiable<string> and setting options.AllowClientGeneratedIds to true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants