zymlabs / nswag-fluentvalidation

Use FluentValidation rules instead of ComponentModel attributes to define swagger schema.
MIT License
59 stars 13 forks source link

Debugging Validation Rules #10

Open andrewwilkin opened 3 years ago

andrewwilkin commented 3 years ago

I've followed the steps as outlined and for some reason I'm not seeing the schema for the object change in the UI. I've confirmed that the validation is indeed being applied.

    public class CreateProfileRequestValidator : AbstractValidator<CreateProfileRequest>
    {
        // ToDo: Case for custom validation here?
        public CreateProfileRequestValidator()
        {
            RuleFor(c => c.FirstName).NotNull().NotEmpty();
            RuleFor(c => c.LastName).NotNull().NotEmpty();

            RuleFor(c => c.IndustryId).NotEmpty();
            RuleFor(c => c.JobTitleId).NotEmpty();

            RuleFor(c => c.CountryId).NotEmpty();
            RuleFor(c => c.City).NotNull().NotEmpty();
            RuleFor(c => c.TimezoneId).NotNull().NotEmpty();
        }
    }

What's the best way of debugging the FluentValidationSchemaProcessor, and seeing that it picked up the validation rule? Have my LogLevel set to debug, and not seeing anything in the output.

Thanks.

andrewwilkin commented 3 years ago

The reason why this is not working is that the CreateProfileRequest inherits from a base class and IsObject and Properties don't work in that case.

I'm not looking to do this as would have lots: https://github.com/RicoSuter/NJsonSchema/wiki/Inheritance

Would say the FluentValidationSchemaProcessor needs to cater for inheritance. Thoughts?

geoffreytran commented 3 years ago

Hi Andrew,

If you set the following setting to settings.FlattenInheritanceHierarchy = true; for the NSwag config, it should include inherited fields as a workaround, but it will not show the base classes in the swagger doc. By default NJson schema includes it into AllOf according to the docs, but I'm not sure I see where it's actually putting it yet.

            serviceCollection.AddOpenApiDocument(
                (settings, serviceProvider) =>
                {
                    var fluentValidationSchemaProcessor = serviceProvider.CreateScope().ServiceProvider.GetService<FluentValidationSchemaProcessor>();
                    settings.SchemaProcessors.Add(fluentValidationSchemaProcessor);

                    settings.SchemaProcessors.Add(new RequiredSchemaProcessor());
                    settings.FlattenInheritanceHierarchy = true; // This setting
               }
geoffreytran commented 3 years ago

It looks like NJsonSchema is correctly generating the AllOf references; however, modifying the FluentValidation schema processor to add the validation rules to the base class in the child class validator would not be a good idea. The reason for this is the validation rule for a property in the base class could conflict with other rules in the base class for other OpenAPI endpoints.

andrewwilkin commented 3 years ago

It's fairly common to use an abstract base class, and the flatten hierarchy would work fine for me in that situation. Though may be able to achieve similar with composition (e.g. having pagination and other properties available on list endpoints).

I think if you had multiple endpoints the validation would not conflict by having it on the base class, the calls are independent of one another and yet some implementation is common 🤔

geoffreytran commented 3 years ago

Yes, I agree, inheritance is a fairly common use case to support without the flattened hierarchy. I'm trying to find a solution that would work with the OpenAPI spec and FluentValidation. Let me know if you have any ideas.

https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/ https://docs.fluentvalidation.net/en/latest/inheritance.html

In the case where you have the following models:

class Pet 
{
    public string Name { get; set; }
    public string Breed { get; set; }
}

class Dog : Pet
{

}

class Cat : Pet
{
}
    public class DogValidator : AbstractValidator<Dog>
    {
        public DogValidator()
        {
            RuleFor(c => c.Breed).MustBeDogBreed();
        }
    }

    public class CatValidator : AbstractValidator<Dog>
    {
        public DogValidator()
        {
            RuleFor(c => c.Breed).MustBeCatBreed();
        }
    }
paths:
  /pets:
    patch:
      requestBody:
        content:
          application/json:
            schema:
              oneOf:
                - $ref: '#/components/schemas/Cat'
                - $ref: '#/components/schemas/Dog'
              discriminator:
                propertyName: pet_type
      responses:
        '200':
          description: Updated
components:
  schemas:
    Pet:
      type: object
      required:
        - pet_type
      properties:
        pet_type:
          type: string
      discriminator:
        propertyName: pet_type
    Dog:     # "Dog" is a value for the pet_type property (the discriminator value)
      allOf: # Combines the main `Pet` schema with `Dog`-specific properties 
        - $ref: '#/components/schemas/Pet'
        - type: object
          # all other properties specific to a `Dog`
          properties:
            bark:
              type: boolean
            breed:
              type: string
              enum: [Dingo, Husky, Retriever, Shepherd]
    Cat:     # "Cat" is a value for the pet_type property (the discriminator value)
      allOf: # Combines the main `Pet` schema with `Cat`-specific properties 
        - $ref: '#/components/schemas/Pet'
        - type: object
          # all other properties specific to a `Cat`
          properties:
            hunts:
              type: boolean
            age:
              type: integer

If you specify a validator on Cat and Dog for the parent Pet, we run into an issue with the OpenAPI spec on how to define the requirement on the Pet property for instance since Pet is shared by both Cat and Dog.

andrewwilkin commented 3 years ago

Will loop back when I get the chance to take a proper look, think I understand the challenge.