Add a filter for request validation

[maurobellati]

Summary

This small PR adds :only and :except to the configuration options.
They allow to whitelist and blacklist the validation based on custom criteria.

Whitelist can be useful in case of a big codebase, without API schema, where the schema is written gradually, over time.

Blacklist can be useful in case of specific path that are non standard, or should not included in the current schema (i.e. internal endpoints with their own schema).

[maurobellati]

Hi @ota42y what do you think about this one?
Thank you in advance

[ota42y]

Sorry for my late reply 🙇

If the requested path isn't in the schema file, committee doesn't validate (when strict option is false).
So I think we don't need :only option and :except option because we can remove the definition from schema file.

I understand that even though there is a definition, we don't want to validation temporarily.
But this is the opposite of the basic function because the main function of committee is to validate that the API is as defined in the schema file.
So I recommend that we change the loaded Schema object rather than add this feature in the committee.

For example, committee support register OpenAPI parser object ( when we use OpenAPI 3 ).
So we can modify Schema Object before register committee.

open_api = OpenAPIParser.parse(YAML.load_file('open_api_3/schema.yaml'))

# filter_operation_object filter Operation Object like Array#select
# This method not exist in OpenAPI Parser ( sample code )
only_open_api = open_api.select_operation_object( ->(op) { op.parent.path =~ ONLY_PATH } )
schema = Committee::Drivers::OpenAPI3::Driver.new.parse(only_open_api)
use Committee::Middleware::RequestValidation, schema: schema

This way we can do your objectives without breaking the basic function in the committee.
But I think we need to add more method to make it easier.

Or if there is a use case that is absolutely necessary, I want to know more.

[maurobellati]

Thank you for your reply.
I understand your hesitation and doubts.

It seems a bit odd feature, but I think it's small, retrocompatible and gives users an additional flexibility, without removing anything.
It also not limited to check the path, but also other runtime values (i.e. headers..)
So, in general, I am in favor for such features/options.

Let me try to explain other reasons.

a. Committee (and OpenAPI Parser) are not without bugs, or missing features.
So I would be able to write correct OpenAPI specs (to be used as documentation, or input for other tools) even if Committee does not support such a feature (yet).
For example: at today, there is no support for content encoding in parameters.
And I do want to use strict mode because I don't want to have, by chance, missing endpoints.

So I would like to explicitly exclude just few endpoints, without disable strict mode.

b. In the following scenario, there is a:

• /v1 endpoints: public available. Schema in public-v1.json
• /v1/internal endpoints: for internal usage. Schema in internal-v1.json

/v1/foo
/v1/bar
/v1/internal/aaa
/v1/internal/bbb

I would like to use strict mode, for both validations.

Currently, the validation for /v1 will include /v1/internal, raising an error since they are defined in the other document.

Both these examples can be supported having the proposed feature (that is already used internally as a monkey-patch).

[maurobellati]

Another reason that I forgot.

It seems Committee just supports JSON responses. because it always tries to JSON.parse(full_body).

So I think it would be nice to have an easy way to filter out endpoints with different media type.

[ota42y]

Thank you for more detail information, that's very helpful :)

Your example ( a. and b.) is very reasonable.
That's right, the strict option is sometimes difficult to use for ours.
And it's committee's problem so we should improve it.

It is indeed small and very easy code so I wouldn't mind to merge this.
However, the concept of committee is to check if APIs follow the definitions and I feel like it is different to control validation conditions by many options.
So I think only and except is very easy way for user but it's not simple way for user 🤔

Now, committee provide prefix option.
This option rewrite request path in pre process.
How about providing the same preprocessing functionality.

For example, we create validation_path_hook method.
This method return path which use finding Operation Object (OpenAPI 3)
And if this method nil, we doesn't validate (same to when @router.includes_request?(request) return false )
When we return replaced path, we can use it as prefix option.
So we'll deprecate prefix option

# return path for validation, if return nil we doesn't validation
def validation_path_hook(request)
    # only
    request.path.start_with?('/something') ? request.path : nil
    
    # except
    request.path.start_with?('/something') ? nil : request.path 
    
    # prefix option (/v1/test => /test)
    request.path.gsub(/^\/v1/, "")
end

I think this way can cover multiple use cases in one way.

[maurobellati]

Thank you for your reply.

Well, in general, I think that having a generic method is better than a specific one.
Also, returning a boolean (as an answer to a question) I feel more meaningful than returning nil.
This is the reason why the :only and :except are generic lambda that accepts a request, so many users might use for different reasons.

The discriminator to validate or not a request, might be more complex than just the path.
It might be based on runtime values:

• HTTPVerb: GET /users vs POST /users
• Content: to avoid the issue mentioned above: It seems Committee just supports JSON responses because it always tries to JSON.parse(full_body).

Checking them on a method called validation_path_hook seems not that right, in my opinion.

Anyway, if you don't feel to merge it, it's fine. We are already using this feature via monkey-patch. I though it was nice to share it and make it official.

[ota42y]

Thanks!

Your proposal should be solved, so I want to solve it in some way.
(Because I get same problem sometimes)

I forgot about the prefix option, as I was trying to solve multiple problems at once but it's not good.
In that case, it is better to use a method that returns a bool value.
So we can rewrite like this.
It's almost the same your code, but I think this is more simple because there are fewer options.
(The option name is temporary)

      @accept_request_hook = options[:accept_request_hook] || -> (_) { true }

      def call(env)
        request = Rack::Request.new(env)

        if @router.includes_request?(request) && @accept_request_hook.call(request)
          handle(request)
        else
          @app.call(request.env)
        end
      end

[maurobellati]

Sorry for my late reply.

Your proposal looks ok to me.
Actually it's simply the whitelisting part (:only).
Don't you like that name? It's quite short and readable.

To me, not from a Ruby/Rails background, "hook" it's not clear, in general.
Other options might be something with filter (i.e accept_request_filter or validation_filter).

What do you think?

Anyway, I will take care of removing the :except part for now, and wait for your comment about the name.
Thank you very much.

[ota42y]

Thanks!

I think when we use 'only', the user expect array argument.
(but I'm heavily influenced by the Rails experience.... 🤔)

So I think 'accept_request_filter' is good because it's clear what happens for all user :)

[maurobellati]

Renamed :only to :accept_request_filter deb6df0

[ota42y]

That's great change, I'm going to accept :)
Could you fix failed test?

[maurobellati]

Done ✔️
Thank you

[ota42y]

Thanks! It's a very good change !!!!

[maurobellati]

Thanks! It's a very good change !!!!

You are welcome. And thank you for your support 👍

[ota42y]

I just released 3.3.0 which include this changes 🎉
https://rubygems.org/gems/committee/versions/3.3.0