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

Required Guid from Header is not shown as required #683

Closed
michael-freidgeim-webjet opened this issue Apr 12, 2018 · 5 comments
Closed

Required Guid from Header is not shown as required #683

michael-freidgeim-webjet opened this issue Apr 12, 2018 · 5 comments

Comments

@michael-freidgeim-webjet

I am using 2.4 version and for property:

    [FromHeader(Name = "x-guid-id")]
        [Required]
        public Guid GuidId { get; set; }

generated documentation doesn't show Required.
It works correctly for

[FromHeader(Name = "x-string-id")]   
        [Required]
        public string StringId { get; set; }

You can loaded an example from https://github.com/MNF/Samples/tree/master/SwashbuckleExample

@domaindrivendev
Copy link
Owner

Swashbuckle sets the required flag according to ASP.NET Core's built-in request validation behavior (i.e. ModelState.IsValid). In your example, that validation will succeed even if the request DID NOT contain the GuidId parameter. This is because the RequiredAttribute has no effect on properties that have a non-null default value. Therefore, and by design, SB does not mark it required.

Swashbuckle aside, you need to make the Guid nullable to get the validation behavior you want. And, if you do that, SB will follow suit by describing it as required

@michael-freidgeim-webjet
Copy link
Author

Thank you for explaining the implementation details. I’ve also found #599, that gives me more info about the approach.

However I think that you should honour RequiredAttribute.
As an API author, I am marking the property as Required and wish to see it in the generated documentation to highlight it for clients.

Developers, including myself, can fall into the trap of using RequiredAttribute on a value type without realizing that a subsequent validation can't possibly fail. 
From #599

Yes, it’s technically redundant, but I don’t think, that it should be Swashbuckle concern. Neither Visual Studio nor Resharper warn that it’s redundant.
It would be good if Swashbuckle will generate a warning, but follow author’s instructions and the promise on the readme:

this metadata will be inferred automatically so long as you're using ASP.NET Core Model Binding or Data Validation attributes to implement request validation.

BTW see related requirement Swashbuckle: Make non-nullable properties required

@domaindrivendev
Copy link
Owner

A few things ...

Firstly, it is Swashbuckle’s concern because it’s underlying principle is to treat the “implementation as truth”. There might be other libraries that take the opposite approach but not Swashbuckle. This has always been the goal.

Secondly, this is described in the readme so not sure what promise you’re referring to.

Thirdly, Swashbuckle provides many extension points so you could override this with the behavior you want quite trivially.

Finally, experience with this project has shown that it’s truly impossible to keep everyone happy. So, I stick to the underlying goal - implementation is truth.

@domaindrivendev
Copy link
Owner

“this metadata will be inferred automatically so long as you’re using ASP.NET Core or Data validation attributes to implement request validation”.

The last few words are the key here. If you’re using the RequireAttribute redundantly on a value type then you’re not using it to implement request validation :)

I thought this, combined with the additional note just after, makes it pretty clear. But, I could always take a second pass to clarify further.

@MNF
Copy link

MNF commented Apr 14, 2018

As an API author, I don’t need to understand exact meaning of each word in readme. I had 2 properties (string and guid) that are mandatory for my request. I’ve marked both as “Required” and wanted to see the same in swagger.json document.

Is it a reasonable expectation?

The actual behaviour is confusing and required research to understand the difference.

Would it be better if the default behaviour will be consistent, intuitive and driven by explicit properties in the code?

Yes, there are different possible workarounds ( I just made both properties strings). My feedback is that your library converts c# APIs to swagger very intuitively, but in this case it is not intuitive at all.

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

No branches or pull requests

3 participants