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.

Sign up for GitHub

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

Jump to bottom

Throw error if multiple matched endpoint formals are found #637

Merged
merged 19 commits into from
Aug 12, 2020
Merged

Throw error if multiple matched endpoint formals are found #637

merged 19 commits into from
Aug 12, 2020

Conversation

schloerke
Copy link
Collaborator

@schloerke schloerke commented Aug 7, 2020 •
edited
Loading

Pivoting on approach of #578 to throw an error if multiple args are found vs trying to merge args.

  • Store the parsed post body into req$postBodyParsed to avoid possible conflicts with req$args
  • Provides a message about how to address the multiple matched formals
  • If only req and res are the available formals, only provide the Plumber req and res. This allow for routes with many arguments matching to req and res not fail and allow users to retrieve the proper information from the Plumber req object

Questions:

  • Should this be enforced for all POST requests? (Only allow named arguments: req, res)
    • No. Allow for req, req to be given only.
  • Should path parameters be put into a special location?
    • Yes. req$argsPath. And query parameters at req$argsQuery

PR task list:

  • Update NEWS
  • Add tests
  • Update documentation with devtools::document()

@schloerke schloerke added this to the v1.0.0 - Next CRAN release milestone Aug 7, 2020
@schloerke schloerke requested a review from cpsievert August 7, 2020 16:05
@schloerke schloerke marked this pull request as ready for review August 7, 2020 16:05
cpsievert
cpsievert reviewed Aug 7, 2020
View reviewed changes
R/plumber.R Outdated Show resolved Hide resolved
cpsievert
cpsievert reviewed Aug 7, 2020
View reviewed changes
R/plumber.R Outdated Show resolved Hide resolved
cpsievert
cpsievert reviewed Aug 7, 2020
View reviewed changes
R/plumber-step.R Outdated Show resolved Hide resolved
cpsievert
cpsievert reviewed Aug 7, 2020
View reviewed changes
R/plumber-step.R Outdated Show resolved Hide resolved
cpsievert
cpsievert requested changes Aug 7, 2020
View reviewed changes
Copy link
Contributor

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

See comments

schloerke and others added 4 commits August 7, 2020 16:03
@schloerke @cpsievert
Display only unique args
0aa63b7 
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
@schloerke
Set req,res in req$args inside pr$call(req)
22de3f2 
For testing purposes, also set req and res if they do not exist.
@schloerke
Merge branch 'multiple_matched_args' of https://github.com/rstudio/pl…
5f52133 
…umber into multiple_matched_args
@schloerke
Merge branch 'master' into multiple_matched_args
1553772 
* master:
  `parser_octet()` returns a named list of filename to content (#636)
  Add `validate_api_spec()` (#633)
  Add another situation for no context JSON post body (#635)
  fix dockerfile extra buildargs
  Misnamed properties private$filename (#638)
@schloerke schloerke requested a review from cpsievert August 7, 2020 20:35
cpsievert
cpsievert reviewed Aug 10, 2020
View reviewed changes
NEWS.md Outdated Show resolved Hide resolved
cpsievert
cpsievert reviewed Aug 10, 2020
View reviewed changes
R/plumber-step.R Outdated Show resolved Hide resolved
@schloerke schloerke requested a review from cpsievert August 12, 2020 14:45
cpsievert
cpsievert reviewed Aug 12, 2020
View reviewed changes
NEWS.md Show resolved Hide resolved
schloerke and others added 2 commits August 12, 2020 11:12
@schloerke
rename routes and output. Remove misleading comments
dab0715
@schloerke @cpsievert
spelling
9b0a1d7 
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
cpsievert
cpsievert approved these changes Aug 12, 2020
View reviewed changes
Copy link
Contributor

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

LGTM pending suggestions for example

@schloerke @cpsievert
Much better phrasing
6d310b3 
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
@schloerke schloerke merged commit 38d5fd7 into master Aug 12, 2020
@schloerke schloerke deleted the multiple_matched_args branch August 12, 2020 16:44
schloerke added a commit that referenced this pull request Aug 12, 2020
@schloerke
Merge branch 'master' into all_params_are_arrays
fbcdb69 
* master:
  Throw error if multiple matched endpoint formals are found (#637)
schloerke added a commit that referenced this pull request Aug 12, 2020
@schloerke
Merge branch 'master' into gut_digital_ocean
cfc94d4 
* master:
  Throw error if multiple matched endpoint formals are found (#637)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpsievert cpsievert cpsievert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@schloerke @cpsievert