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

Add field usage as post endpoint #14

Merged
merged 6 commits into from
Dec 21, 2020
Merged

Conversation

TPRobots
Copy link

Pull request opened by github-pullrequestcreator.

CHANGELOG.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
sanicargs/__init__.py Show resolved Hide resolved
sanicargs/__init__.py Show resolved Hide resolved
sanicargs/__init__.py Outdated Show resolved Hide resolved
@@ -62,9 +93,17 @@ async def generate_csv(request, query: str, businessunitid: str):
async def inner(request, *old_args, **route_parameters):
kwargs = {}
name = None
if legacy or request.method == "GET":
parameters = request.args
Copy link
Contributor

Choose a reason for hiding this comment

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

what about both query args and json_body args ?
in this way it seems you cant have both?
i suggest:

parameters = request.args
 try:
    body_parameters = json.loads(request.body)
    body_parameters.update(parameters) # query.args are higher priority than body params
    parameters = body_parameters
except json.decoder.JSONDecodeError:
    pass

else you loose the ability to:
@route("/age//lol")
@parse_params(request, age:int, country: str, name: str)
with request being:
url= /age/21/lol?country=dk
body={"name": "lolcat"}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I'll post an issue related to this saying it isn't currently implemented and if someone wants to do it. they can have a go at it.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 just refer here

Copy link
Contributor

Choose a reason for hiding this comment

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

@sallas remember that not supprting path params together with json parameters atleast breaks your api's cus then you cant get businessunitid no more

Choose a reason for hiding this comment

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

Good point. Path parameters must be supported in both cases. The intention was to not implement support for query params in a POST request

Copy link
Contributor

Choose a reason for hiding this comment

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

i think the easiest is support path+query+json and maybe take inspiration in my comments.
and then maybe focus on priority:

  1. path
  2. query
  3. json
    so path is most important, json least impotant
    so big warning: dont use same names in path+query+json cuz then trouble.

but supporting all should make less complicated code i think
@jorgenpersson @sallas

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

kewl!

for name, arg_type, default in parameters:
raw_value = request.args.get(name, None)
for name, arg_type, default in func_parameters:
raw_value = parameters.get(name, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe do something like:

query_parameters = request.args
json_parameters = json.loads(request.body)
...

NOTHING = object()
raw_value_origin = None:
raw_value = NOTHING
query_arg_value = query_parameters.get(name, None)
json_param_value = json_parameters.get(name, None)
if query_arg_value is not None:
    raw_value_origin = "query"
    raw_value = query_arg_value
elif json_param_value is not None:
    raw_value_origin = "json"
    raw_value = json_arg_value

cuz then you know where you got it from and the
elif name not in parameters:
can now be:
elif raw_value is NOTHING:

and the:
raise KeyError(
f"Missing required {'argument' if legacy else 'parameter'} {name}"
)
can now be:
raise KeyError(
f"Missing required {raw_value_origin} parameter {name}"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll link to both of these comments in the issue. This could be enough for someone to figure out how to proceed. For now I don't want to spend the time taking a decision in something that seems like a half way there solution.

However, if we could somehow specify which arg is in the query vs in the body, that would be my preferred solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍
i could imagine something like:
def my_handler(request, varname: List[int])
would implicitly use the priority hierachy (so first path, then query, then json, (if name clashing)

and then give the following option to specify from where:

from sanicargs import types
def my_handler(request, varname: types.path.List[int]) 
or
def my_handler(request, varname: types.query.List[int]) 
or
def my_handler(request, varname: types.json.List[int]) 

then we can differentiate.

i will make a pr at some point on that issue

pyproject.toml Outdated Show resolved Hide resolved
sanicargs/__init__.py Outdated Show resolved Hide resolved
sanicargs/__init__.py Show resolved Hide resolved
sanicargs/__init__.py Outdated Show resolved Hide resolved
@sallas sallas merged commit 78f2cdc into master Dec 21, 2020
@sallas sallas deleted the add-field-usage-as-post-endpoint branch December 21, 2020 08:22
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.

4 participants