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

Some RPCs are GET-only #966

Merged
merged 52 commits into from
Mar 23, 2020
Merged

Conversation

eddyashton
Copy link
Member

Resolves #633.

This PR:

  • Adds support for restricting which verbs are supported for a specific RPC (install(...).set_http_get_only(), or more generally .restrict_allowed_verbs({HTTP_DELETE, HTTP_GET,...})
  • Sets some built-in RPCs to be GET-only (getCommit, listMethods, etc). Mostly things that are Read and have simple params types
  • Modifies the Python infra to take a specific verb, in particular using c.get rather than c.rpc
  • Adds a url-decoder/unescaper. For GETs we need to put params in the query, not the body. Some of these queries need escaping (whenever we have a json-string value, its surrounding quotes need to be escaped). I spent a long time trying to convince Python-requests to not escape the query, but all the workarounds are just arounds (they don't work), so I bit the bullet and wrote a quick escaper on the C++ side
  • Related tidying (removing the unneeded client.do in Python, removing empty params so we have actually empty bodies rather than JSON nulls)

NB: I'm using "verb" to describe GET, POST etc. These are generally referred to as methods, but we already have a lot of code using method to describe the RPC-identifier. This is a bit clumsy - maybe we should do a renaming pass to make the old method a resource, and then we can use method instead of verb? Opinions welcome.

@eddyashton eddyashton requested a review from a team as a code owner March 19, 2020 18:12
samples/apps/txregulator/clients/loader.py Show resolved Hide resolved
src/node/rpc/handlerregistry.h Outdated Show resolved Hide resolved
@jumaffre
Copy link
Contributor

Looks like there are still some issues with the CI but the changes look good!

@ghost
Copy link

ghost commented Mar 20, 2020

verb_selection@6187 aka 20200323.6 vs master ewma over 30 builds from 5897 to 6181
images

@codecov-io
Copy link

codecov-io commented Mar 23, 2020

Codecov Report

Merging #966 into master will increase coverage by 0.22%.
The diff coverage is 95.4%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #966      +/-   ##
==========================================
+ Coverage   68.06%   68.28%   +0.22%     
==========================================
  Files         103      103              
  Lines        8177     8234      +57     
==========================================
+ Hits         5565     5622      +57     
  Misses       2612     2612
Flag Coverage Δ
#unit_BFT 68.28% <95.4%> (+0.22%) ⬆️
#unit_CFT 68.28% <95.4%> (+0.22%) ⬆️
Impacted Files Coverage Δ
src/http/http_rpc_context.h 39.39% <0%> (+0.46%) ⬆️
src/node/rpc/memberfrontend.h 67.4% <100%> (-0.18%) ⬇️
src/node/rpc/frontend.h 66.09% <100%> (+1.86%) ⬆️
src/node/rpc/handlerregistry.h 81.82% <100%> (+2.87%) ⬆️
src/node/rpc/commonhandlerregistry.h 28.57% <100%> (+2.76%) ⬆️
src/node/rpc/nodefrontend.h 79.01% <100%> (+0.35%) ⬆️
src/node/rpc/jsonhandler.h 83.67% <100%> (+0.52%) ⬆️
src/http/http_parser.h 88.57% <95.83%> (+1.39%) ⬆️
src/tls/keypair.h 74.69% <0%> (+0.31%) ⬆️

@achamayou achamayou merged commit 4d480f4 into microsoft:master Mar 23, 2020
eddyashton added a commit to eddyashton/CCF that referenced this pull request Mar 24, 2020
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.

Modify read-only built-in endpoints to GET
5 participants