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

Filtering Input Parameters to Childchain/Watcher API depending on HTTP Method #1424

Merged
merged 41 commits into from
Apr 10, 2020

Conversation

kalouo
Copy link

@kalouo kalouo commented Mar 25, 2020

Addresses #1411

Overview

This PR implements filtering for input parameters depending on the HTTP method used.

  • For a POST: query_params will be ignored and body_params will be used.
  • For a GET: body_params will be ignored and query_params will be used.

Changes

Creates plug in method_param_filter.ex files which are added to endpoint.ex in in both omg_childchain_rpc and omg_watcher_rpc to implement above logic.

Testing

  • Tests written for method_param_filter.ex (two files: one for childchain and one for watcher)
  • Tests added to selected endpoints on Watcher Info (/account.get_balance and /transaction.get) and Child Chain (/block.get) to test that passing passing query parameters will return a bad request error.

_For the second set of tests: while these do the job, iI could not think of how to target tests to endpoint.ex. Any insights welcome. Note that we have no GET methods that take input parameters, so no tests written as above.

mix test test/omg_child_chain_rpc/web/controllers/block_test.exs
mix test test/omg_watcher_rpc/web/controllers/block_test.exs
mix test test/omg_watcher_rpc/web/controllers/transaction_test.exs
mix test test/omg_child_chain_rpc/web/plugs/method_param_filter_test.exs
mix test test/omg_watcher_rpc/web/plugs/method_param_filter_test.exs

Thanks to @mederic-p who implemented the logic right before his baby was born. I just added a few tests.

@kalouo
Copy link
Author

kalouo commented Mar 25, 2020

Hi all, I just had a chat with @thec00n and there are a few things where we could use more experienced eyes:

In endpoint.ex:

    parsers: [:urlencoded, :multipart, :json],
    pass: ["*/*"],

@mederic-p changed to (child chain):

    parsers: [:urlencoded, :json],
    pass: [],

@mederic-p changed to (watcher):

   parsers: [:json],
   pass: [],

It looks fine in the sense that we want the endpoints to only process valid JSON, but I'm not sure if there's something else to consider here, and why :urlencoded stays in the child chain endpoint.ex file.

Would appreciate any insights you have there!

@coveralls
Copy link

coveralls commented Mar 25, 2020

Coverage Status

Coverage decreased (-0.03%) to 77.341% when pulling ce14a71 on 1411-filter-params-http-method into 91cb67f on master.

@InoMurko InoMurko force-pushed the 1411-filter-params-http-method branch from 5846ad9 to 8c05f0e Compare April 10, 2020 10:20
@InoMurko
Copy link
Contributor

@unnawut pls review!

@kalouo kalouo requested a review from achiurizo as a code owner April 10, 2020 11:48
Copy link
Contributor

@InoMurko InoMurko left a comment

Choose a reason for hiding this comment

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

pls remove .tool_versions, distillery, and try if parsers: [:urlencoded, :json], works only with parsers: [:json],

@kalouo
Copy link
Author

kalouo commented Apr 10, 2020

yep sorry mistake, intended to push only changes to endpoint.

@kalouo kalouo removed the request for review from achiurizo April 10, 2020 12:07
Copy link
Contributor

@thec00n thec00n left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@unnawut unnawut left a comment

Choose a reason for hiding this comment

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

:mindblown: I wouldn't have thought of checking Connection modules. Thanks you two!

@kalouo
Copy link
Author

kalouo commented Apr 10, 2020

Thanks to @InoMurko for that one!

@kalouo kalouo merged commit 1a36b27 into master Apr 10, 2020
@kalouo kalouo deleted the 1411-filter-params-http-method branch April 10, 2020 13:42
@unnawut unnawut added api API-level issues breaking Contains breaking changes enhancement New feature or request and removed enhancement New feature or request labels Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API-level issues breaking Contains breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants