plone / guillotina

Python AsyncIO data API to manage billions of resources
https://guillotina.readthedocs.io/en/latest/
Other
187 stars 51 forks source link

Path params containing paths #969

Closed dmanchon closed 4 years ago

dmanchon commented 4 years ago

I wonder if accepting actual paths in the path parameter (something like this in fastapi: https://fastapi.tiangolo.com/tutorial/path-params/#path-parameters-containing-paths) could be nice to have.

So defining a service with @api/get_path/{path:path} with the request@api/get_path/folder1/2/3 request will capture path as "/folder/1/2/3"

bloodbare commented 4 years ago

@bloodbare any objections?

I can review today later

codecov-commenter commented 4 years ago

Codecov Report

Merging #969 into master will increase coverage by 0.1%. The diff coverage is 97.6%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #969     +/-   ##
========================================
+ Coverage    94.6%   94.6%   +0.1%     
========================================
  Files         315     315             
  Lines       27907   27939     +32     
========================================
+ Hits        26391   26422     +31     
- Misses       1516    1517      +1     
Impacted Files Coverage Δ
guillotina/routes.py 93.4% <94.2%> (-0.4%) :arrow_down:
guillotina/contrib/swagger/services.py 91.9% <100.0%> (ø)
guillotina/tests/test_configure.py 98.2% <100.0%> (+0.2%) :arrow_up:
guillotina/tests/test_routes.py 100.0% <100.0%> (ø)
guillotina/traversal.py 85.2% <100.0%> (+0.2%) :arrow_up:
dmanchon commented 4 years ago

I may prefer that the path tail has no parameter name, something like a boolean attribute that defines to allow any path tail and store it on a path variable. @dmanchon whats your opinion?

something like this:

@configure.service(context=IContainer, name='@myservice', match_tail=True, method='GET',
                   permission='guillotina.AccessContent')
async def my_service(context, request):
    ...
    filepath = request.tail 

vs

@configure.service(context=IContainer, name='@myservice/{tail:path}', method='GET',
                   permission='guillotina.AccessContent')
async def my_service(context, request):
    filepath = request.matchdict["tail"]

i am ok with either solution, i find the second one slightly more readable, but maybe i am spoiled since i am used to fastapi.

bloodbare commented 4 years ago

i am ok with either solution, i find the second one slightly more readable, but maybe i am spoiled since i am used to fastapi.

I just think that lead to missunderstanding that you can define the name of the variable for the path. I understand that we should be able to extend this behavior with {VAR_NAME:int} and {VAR_NAME:string}

dmanchon commented 4 years ago

i am ok with either solution, i find the second one slightly more readable, but maybe i am spoiled since i am used to fastapi.

I just think that lead to missunderstanding that you can define the name of the variable for the path. I understand that we should be able to extend this behavior with {VAR_NAME:int} and {VAR_NAME:string}

That is a very interesting idea, fastapi allow that but with a different api: https://fastapi.tiangolo.com/tutorial/path-params/ I think there is a misundertanding, you can define the name of the path var: @endpoint/{varname: path} then you will be able to capture varname in the match dictionary of the request.