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

RFC: support passing the whole parsed arguments as a keyword argument #830

Closed
greyli opened this issue Apr 25, 2023 · 10 comments
Closed

RFC: support passing the whole parsed arguments as a keyword argument #830

greyli opened this issue Apr 25, 2023 · 10 comments

Comments

@greyli
Copy link
Contributor

greyli commented Apr 25, 2023

Hi team,

Currently, we provide two options to pass the parsed arguments to the view function:

  • use_args
  • use_kwargs

It seems there is a mismatch here. use_args passes the parsed arguments as a single positional argument, while the use_kwargs passes all the arguments as separate keyword arguments.

I'm thinking of adding the missing feature between this mismatch, which is to support passing the whole parsed arguments as a keyword argument.

What are the benefits? It can help to ensure the natural order for Flask views:

@app.post('/pets/<pet_id>')
@use_args(PetQueryIn, location='json')
@use_args(PetBodyIn, location='query')
def create_pet(json_data, query_data, pet_id):
    pass

Since the URL variables (i.e. pet_id) will be passed as keyword arguments, we have to declare the arguments from use_args first.

Although we can use use_kwargs to pass all arguments as keyword arguments, but consider we will use three stacked use_kwargs and each schema contains five fields...

I made some experiments, there are two ways to achieve this:

  • Create a new function to pass the whole parsed arguments as a keyword argument (maybe called use_named_args):
@app.post('/pets/<pet_id>')
@use_named_args(PetQueryIn, location='json', arg_name='data')
@use_named_args(PetBodyIn, location='query', arg_name='query_data')
def create_pet(pet_id, data, query_data):
    pass
  1. Add an arg_name argument to use_args and use_kwargs.
@app.post('/pets/<pet_id>')
@use_kwargs(PetQueryIn, location='json', arg_name='json_data')
@use_kwargs(PetBodyIn, location='query', arg_name='query_data')
def create_pet(json_data, query_data, pet_id):
    pass

Do you think it's a useful feature? Which solution is preferred?

I'm happy to submit a PR. Thanks!

Related issue: apiflask/apiflask#427

@lafrech
Copy link
Member

lafrech commented Apr 25, 2023

Yeah, current naming is not that self-explanatory.

We could change it and release a major. I'm short of better ideas, but open to suggestions.

If we keep current names, I like the second option better, keep the use_args name and pass an optional name.

However, unless I didn't understand the proposal, I don't think it makes sense for use_kwargs.

@sirosen
Copy link
Collaborator

sirosen commented Apr 25, 2023

I agree with the premise of this issue and would like to do something about it, but I'm not sure we have my ideal solution in hand yet.

use_kwargs "explodes" the loaded dict items into individual arguments to pass as kwargs. So I'm trying to think if there's some way to draw the line around the fact that this is "non-exploding use_kwargs".
And there's the fact that use_kwargs is just use_args(..., as_kwargs=True) to consider.

So I'm looking at names and thinking what we can do... I don't like proposal (2) above, given the two choices. arg_name being valid for use_kwargs but not use_args feels confusing.
use_args(..., arg_name="foo") makes sense to me as an implementer, but the naming and combination with use_kwargs would be confusing for users.

Proposal (1) above, a new method, might be the easiest and most feasible path, but I'd rather we give that capability to use_args in some way, so that we have fewer very similar methods to explain.


Here's what I'd like to have in the future, just to spitball a bit:

  • use_args only passes keyword arguments (never positional args)
  • use_args derives the argument names from location, always passing a consistently named value (e.g. json => parsed_json, query => parsed_query, etc)
  • In support of exotic usages, use_args allows for arg_name=... as an override
  • as_kwargs is deprecated and removed, to be replaced with expand_kwargs or another clearer name
  • use_kwargs is renamed to match the above, prioritizing clarity over brevity e.g. use_expanded_kwargs

I think we could introduce most of this in backwards compatible ways by adding arg_name with a default of None, meaning "pass as positional arg", and making it mutually exclusive with as_kwargs=True.

@greyli
Copy link
Contributor Author

greyli commented Apr 26, 2023

Looks good! Your proposal is more complete and reasonable. I had something similar when I think about this at downstream (apiflask/apiflask#427). But I plan to set the location value as the argument name. Do you think it's a good idea? It's more intuitive and easy to declare:

@app.post('/pets/<pet_id>')
@use_args(PetBodyIn, location='json')  # -> json
@use_args(PetQueryIn, location='query')  # -> query
def create_pet(pet_id, json, query):
    pass

The only problem is that the json argument will overwrite the json module.

@sirosen
Copy link
Collaborator

sirosen commented Apr 26, 2023

If we want to introduce this in v8.x of webargs, we don't need to solve the naming problem right away because the default will remain unchanged (pass as a positional arg).
That said, if you're going to implement something in apiflask, it would be cool if we could agree now so that our future default is something you can match. But we might still choose something different knowing that it can be tweaked by setting arg_name, if we go that route.

But I plan to set the location value as the argument name. Do you think it's a good idea? It's more intuitive and easy to declare:

...

The only problem is that the json argument will overwrite the json module.

This is probably only a problem for json but it makes me a little wary of doing it, which is why I was thinking parsed_json or json_data, etc.

I don't have super-strong feelings about the naming, so long as it's clear that it's the parsed data coming from webargs (or a wrapping framework like apiflask! 😁 ). So let's just look at some names:

  • json_data
  • parsed_json
  • json_payload
  • json_body
  • webargs_json (sort of verbose? 🤷 )
  • json_input
  • input_json
  • request_json
  • req_json

I'm going to cool it for a bit so that @lafrech has a chance to weigh in -- I'd like to know if he likes where this is going.

I think this is a good path if we're comfortable with the idea of deprecating passing data as positional arguments, and removing that in v9. It may force a lot of users to change their usage, but it makes things way clearer when decorators are stacked.

@lafrech
Copy link
Member

lafrech commented Apr 26, 2023

Thanks for your thoughts on this.

No strong feeling about a breaking change. A clear interface and less API surface is what matters. As long as no use case is left behind, a find'n replace breaking change doesn't bother me.

I wouldn't put webargs in the name. Verbose indeed, plus webargs may be hidden in apiflask/smorest so the user doesn't have to know.

Could be json_args, that's in line with webargs.

@greyli
Copy link
Contributor Author

greyli commented Apr 27, 2023

Haha, we could make a vote for these names. There are eight emoji actions for an issue comment. We could use them to vote for eight options :P.

BTW, I like json_data and json_args.

@sirosen
Copy link
Collaborator

sirosen commented Jul 10, 2023

8.3.0 has been released with Parser.USE_ARGS_POSITIONAL as a class-level control.
There's also support for passing arg_name=... to set the argument name automatically.

I really like the result and I hope everyone else does as well!


I have considered trying out, in my own usage, using a wrapper which looks for my schema objects to have a webargs_arg_name attribute. So we could do something like

class FrobulatorSchema(Schema):
    foo = fields.Str()
    webargs_arg_name = "frobulator"

@my_webargs_wrapper(FrobulatorSchema, location="json")
def myview(*, frobulator: dict): ...

I haven't tried this yet and there's no feature in webargs for it, but if it seems interesting, I'd be happy to pursue it in future issues.

For now, I'm marking this as resolved by the 8.3.0 release. Thanks for filing this issue!

@sirosen sirosen closed this as completed Jul 10, 2023
@ThiefMaster
Copy link
Contributor

ThiefMaster commented Jul 28, 2023

I just saw this now, I think when you use the json_or_form location the arg name will be very convoluted (json_or_form_args). And having to specify arg_name='args' all the time isn't great either.

I understand that it'd be tricky to special-case the whole thing when there's only one use_args decorator on a function, but just a plain args would be so much nicer in this - probably most common - case...

@sirosen
Copy link
Collaborator

sirosen commented Jul 28, 2023

I'm not sure I'd call json_or_form_args a negative or positive effect of the change.
On the one hand, it's verbose, but on the other hand, it's unambiguous.
I wish it weren't so long, but I think it's nice that you were able to deduce exactly what it will be correctly without any trouble.

We still have a bit of leeway on this for 9.0 since the only behavior we've "locked in" is the 8.x USE_ARGS_POSITIONAL=False default. I'm willing to entertain ideas about how to improve it.

I don't really like the idea of special-casing the single-decorator usage. Not from the perspective of messy implementation -- IMO it's messy but doable -- but because I think it's likely to confuse people.

What if we made the default behavior, when arg_name is not passed, customizable?
So update this something like so:

         if arg_name is None and not self.USE_ARGS_POSITIONAL:
-            arg_name = f"{location}_args"
+            arg_name = self.get_default_arg_name(location, schema)

where of course we just make a method

    def get_default_arg_name(self, location: str, schema: Schema) -> str:
        return f"{location}_args"

Then we just document + test it and it's easy to subclass and override the method.
(There is a minor tweak needed to put it after the schema gets built, but that's just ordering which doesn't matter today and will start to matter.)

It's not the most elegant interface in the world, but you could do something like

    def get_default_arg_name(self, location: str, schema: Schema) -> str:
        if location in {"json", "form", "json_or_form"}:
            return "payload"
        return f"{location}_args"

or even

    def get_default_arg_name(self, location: str, schema: Schema) -> str:
        if isinstance(schema, MySpecialSchemaDeclaringArgName):
            return schema.arg_name
        return f"{location}_args"

I think I would use that last example myself, since it lets me have

class FrobulatorInput(Schema):
    arg_name = "frobulator_input"

@ThiefMaster, if this solution appeals to you, to keep the current default but open up some avenues to tune it, let me know and I'll see if I can work up a PR to discuss it further.

@ThiefMaster
Copy link
Contributor

What if we made the default behavior, when arg_name is not passed, customizable?

sounds like a perfect solution for me :)

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

No branches or pull requests

4 participants