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

update to generate swagger docs for mounted routers #274

Closed
wants to merge 3 commits into from

Conversation

bradleyhd
Copy link

@bradleyhd bradleyhd commented Jun 19, 2018

Hi,

I'm not sure if plumber is currently taking unsolicited PRs, but I'm new to both R and plumber and really wanted the automatic swagger doc generation to work with mounted routers. These changes seem to work for me (at least for walking this list of endpoints for the router children of the root router). Open to any/all feedback/criticism and happy to revise.

Thanks!

(I think this addresses #219)

@CLAassistant
Copy link

CLAassistant commented Jun 19, 2018

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Jun 19, 2018

Codecov Report

Merging #274 into master will increase coverage by 1.97%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
+ Coverage   87.91%   89.89%   +1.97%     
==========================================
  Files          25       25              
  Lines        1109     1128      +19     
==========================================
+ Hits          975     1014      +39     
+ Misses        134      114      -20
Impacted Files Coverage Δ
R/plumber.R 84.09% <100%> (+1.8%) ⬆️
R/plumber-step.R 84.61% <0%> (+1.53%) ⬆️
R/swagger.R 100% <0%> (+28.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 095b537...44cadd7. Read the comment docs.

@schloerke
Copy link
Collaborator

schloerke commented Jun 19, 2018

Hi @bradleyhd !

Thanks for the PR! Good catch! (Unsolicited PRs are welcome anytime!)

Couple things to address.

  1. Could the for loop be changed to a
mountedEndpoints <- unlist(lapply(names(self$mounts), function(path) {
...
  if (length(mountEndpoints) == 0) return(NULL)
...
}))
endpoints <- c(endpoints, mountedEndpoints)

NULL values with unlist() are removed. When possible, avoid answer <- c(answer, newItem) as it does not scale well.

  1. Formatting
  • The inner logic seems good for calculating mount_endpoints. Could you upgrade the variable names (i,j) to something more descriptive? (mountEndpoint, mountEndpointEntry or something similar)
  • Could you change the code to use camelCase (vs snake_case)?
  1. We should make the swaggerFile mount calculations recursive. In the current state, it is missing swaggerFile information for p3 (/sub2/sub3)
pr <- plumber$new()
pr$handle("GET", "/nested/path/here", function(){})
pr$handle("POST", "/nested/path/here", function(){})

stat <- PlumberStatic$new(".")

pr2 <- plumber$new()
pr2$handle("POST", "/something", function(){})
pr2$handle("GET", "/", function(){})

pr3 <- plumber$new()
pr3$handle("POST", "/else", function(){})
pr3$handle("GET", "/", function(){})

pr$mount("/static", stat)
pr2$mount("/sub3", pr3)
pr$mount("/sub2", pr2)

pr$swaggerFile()

Thank you in advance,
Barret

@schloerke schloerke self-assigned this Jun 19, 2018
@schloerke
Copy link
Collaborator

schloerke commented Jun 19, 2018

Yes, it solves #219

@bradleyhd
Copy link
Author

bradleyhd commented Jun 19, 2018

Thanks for the pointers/feedback! I've updated to walk routers/mounts recursively and it seems to work on your example and a few others I tried out. However, if you repeatedly call pr$swaggerFile(), the paths are repeatedly prepended with their parent paths. I'm not sure what's idiomatic here for preventing this from happening but something should probably be done 😄

@schloerke
Copy link
Collaborator

schloerke commented Jun 20, 2018

Correct. This is caused by endpointEntry being a R6 object which is a "pass by reference" , rather than the typical R "pass by value" object.

I've added some small GitHub review changes. By cloning endpointEntry, it won't be effected by the multiple calls of swaggerFile().

Last thing before merging... Add a test in tests/testthat/test-swagger.R that uses the recursive example above that checks for the 5 path names expected from pr$swaggerFile()

> names(pr$swaggerFile()$paths)
[1] "/nested/path/here" "/sub2/something"   "/sub2/"           
[4] "/sub2/sub3/else"   "/sub2/sub3/" 

Thank you!

@bradleyhd
Copy link
Author

bradleyhd commented Jun 20, 2018

@schloerke maybe I'm not looking in the right place, but I don't see any review changes. Mind re-adding them?

R/plumber.R Outdated
endpointEntry
})

endpoint
Copy link
Collaborator

@schloerke schloerke Jun 20, 2018

Choose a reason for hiding this comment

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

change to endpointEntries as the value of endpointEntries will be different than endpoint.

parentPath <- sub("[/]$", "", parentPath)
endpoints <- lapply(router$endpoints, function(endpoint){

endpointEntries <- lapply(endpoint, function(endpointEntry){
Copy link
Collaborator

@schloerke schloerke Jun 20, 2018

Choose a reason for hiding this comment

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

add a line just inside function(endpointEntry) to be

function(enpointEntry) {
  enpointEntry <- enpointEntry$clone()
  ...
}

It avoids altering the global endpointEntry$path value

@schloerke
Copy link
Collaborator

schloerke commented Jun 20, 2018

My fault.. never hit "submit review". (GitHub review is new to me)

@bradleyhd
Copy link
Author

bradleyhd commented Jun 20, 2018

No worries at all. I've pushed the test and clone changes, let me know if there's anything else!

@schloerke schloerke requested review from trestletech and removed request for trestletech Jun 29, 2018
mountedEndpoints <- private$swaggerFileWalkMountsInternal(mountedSubrouter, paste(parentPath, mountPath, sep="/"))
}))

c(endpoints, list(mountedEndpoints))

Choose a reason for hiding this comment

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

Is there a reason you call list here? My understanding is that at this point endpoints is a list and mountedEndpoints is a character vector, so the result will definitely be a list.

Copy link
Author

@bradleyhd bradleyhd Jul 2, 2018

Choose a reason for hiding this comment

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

@alandipert I'm a bit fuzzy on the types but I think endpoints is a list of lists and mountedEndpoints without the extra list call is just a list. If you remove that call and try to run swagger documentation, you'll get Error in for (e in fil) { : invalid for() loop sequence. When I print out the result of calling c above without modification, I get entries like

[[1]]`$__no-preempt__14`
<PlumberEndpoint>
...

Choose a reason for hiding this comment

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

Ah, that makes sense, thanks for clarifying.

schloerke added a commit to schloerke/plumber that referenced this pull request Jul 16, 2018
schloerke added a commit that referenced this pull request Aug 7, 2018
* update to generate swagger docs for mounted routers

* update swagger doc generation to recursively walk mounted routers

* add test for swaggerFile paths and clone endpointEntry

* prepareSwaggerEndpoints should use entries and not nested entries within endpoints

* Collect endpoint entries, not nested entries within endpoints that contain filter structure

use small helper methods to deal with slashes

For the endgame of swagger docs, filter information is not needed and should be removed to not misrepresent that it does exist.

A true "flatten router" method should be implemented.  Then, swaggerFileWalkMountsInternal will become `flatten(pr)$endpoints`

* add swaggerFile recursive mounts tests

multiple entries
static file
parameter in path
filter
nested mounts
trailing slash in route

* add news item for #274

* tag bradleyhd in news

* document

* use regular roxygen version
@schloerke
Copy link
Collaborator

schloerke commented Aug 7, 2018

Merged in #280

Thank you @bradleyhd!!

@schloerke schloerke closed this Aug 7, 2018
@turiaso
Copy link

turiaso commented Aug 8, 2019

Hi, a quick question. Do you upload the version with this feature 0.4.6.9000 (based on versión downloaded by install_github) to CRAN repositories?. Same case on 0.5.0 version, based on changelogs. I understand that both versions are more similars, one is a snapshot another is a release. Am I mistaked?
Regards!

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.

None yet

6 participants