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

Expose conn validation for reuse in custom plugs #129

Merged

Conversation

soundmonster
Copy link
Contributor

The validation plug is quite handy, but only works best at the top-level
router. This commit allows piping the whole API scope (e.g. /api)
throught the validator plug, but allowing Swagger UI to be hosted
somewhere under the scope's path (e.g. /api/v1/docs) without validation
errors.

"""
def init(opts), do: opts

def call(conn, opts) do
validation_failed_status = Keyword.get(opts, :validation_failed_status, 400)
regex_result = opts
|> Keyword.get(:exclude_paths, [~r/$^/]) # the default regex matches nothing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List.wrap() will convert a nil into an empty list, so no need for the default regex.

@doc """
Performs unconditional validation of a request. Will halt and send error on failed validation.
"""
def handle_validation(conn, opts) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a private function

* `{:error, message, path}` if the request was mapped but failed validation
"""
def validate(conn) do
result =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this result variable is unused

@0xAX
Copy link
Member

0xAX commented Sep 12, 2017

Otherwise looks good to me

@mbuhot
Copy link
Contributor

mbuhot commented Sep 12, 2017

but allowing Swagger UI to be hosted
somewhere under the scope's path (e.g. /api/v1/docs) without validation
errors.

I believe this already works, eg see the example project:

  pipeline :api do
    plug :accepts, ["json"]
    plug Validate, validation_failed_status: 422
  end

  scope "/api", SimpleWeb do
    pipe_through :api
    resources "/users", UserController, except: [:new, :edit]
  end

  scope "/api/swagger" do
    forward "/", PhoenixSwagger.Plug.SwaggerUI, otp_app: :simple, swagger_file: "swagger.json"
  end

@mbuhot
Copy link
Contributor

mbuhot commented Sep 12, 2017

I'm wondering if there is another way to opt-out of validation that doesn't involve matching multiple regexes against each request.

Eg, if we set a flag like conn.private.phoenix_swagger.valid in an earlier plug, then the validate plug can just pattern match on that flag and skip the validation.

@soundmonster
Copy link
Contributor Author

It seems to me that this plug has two interesting parts: validation of the request parameters against a Swagger Spec from ETS, and acting upon the results of the validation. Maybe it is a better idea to just extract validate/1 and its dependencies from the plug into a standalone module, and provide a simple plug with its functionality as is in master right now. Then everyone can use the plug for simple scenarios, or build their own solution using validate/1 in cases where custom error formatting is needed, etc.

@soundmonster soundmonster changed the title Allow excluding paths from validation plug. Expose conn validation for reuse in custom plugs Sep 14, 2017
@soundmonster
Copy link
Contributor Author

@0xAX @mbuhot thank you for your feedback! I have extracted request validation from the plug, so now it can be reused in custom plugs or called directly from controllers. Also, there is now a flag conn.private.phoenix_swagger.valid that, if true, would bypass the validator plug. I have added some info on how to use Plug.Validate and ConnValidator to the readme.

Leonid Batyuk added 7 commits February 14, 2018 10:21
The validation plug is quite handy, but only works best at the top-level
router. This commit allows piping the whole API scope (e.g. /api)
throught the validator plug, but allowing Swagger UI to be hosted
somewhere under the scope's path (e.g. /api/v1/docs) without validation
errors.
@soundmonster soundmonster force-pushed the path_filtering_in_validation_plug branch from 43b054f to 84dd479 Compare February 14, 2018 09:45
@soundmonster
Copy link
Contributor Author

@0xAX @mbuhot hi guys, I've rebased the PR on the latest master and made sure the tests pass. Any chance of getting this merged? The other open PR (#160) modifies the code that I have moved to ConnValidator - I'd be glad to help moving @pablobcb's changes to the new module.

@mbuhot
Copy link
Contributor

mbuhot commented Feb 14, 2018

@soundmonster sorry this has been unmerged for so long. I'll take a look and try to merge this week. 👍

@soundmonster
Copy link
Contributor Author

Great! BTW, we're running off of my fork in production since late Oct 2017 and no problems so far 😉

@mbuhot
Copy link
Contributor

mbuhot commented Feb 17, 2018

👍 Moving the query/path/header param validation out of the Plug and into a regular module looks good.

It might be a bit confusing for new users now that there are 3 ways to perform validation with phoenix_swagger. I've created a separate isse #167 to refactor the README into guides, where we can make it clear when to use the Plug.Validate vs Validator vs ConnValidate

@mbuhot mbuhot merged commit 6223736 into xerions:master Feb 17, 2018
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

Successfully merging this pull request may close these issues.

3 participants