1102371 - client (python): Do not attempt HTTP request without all parameters

Status: RESOLVED FIXED

(Taskcluster :: General, defect)

Description

[Jonas Finnemann Jensen (:jonasfj)]

The python client does not throw nice errors, when/if the user forgets a parameters:
https://pastebin.mozilla.org/7414121

Basically, calling queue like that, the error should tell that you're not providing all the required parameters.

Attempting the HTTP request is a bad idea :)

Comment 1

[John Ford [:jhford] CET/CEST Berlin Time]

There is a check to make sure that you provide the same number of arguments as are specified in the api, the call to the library was done here.  You cannot provide too many, you cannot provide not enough.

You can provide the args as either positional or keyword arguments, as documented in the README.md file[1].  Payload is always a KW argument with the key of 'payload'.  Per this API [2], this method accepts a single argument (taskId) and requires a payload (because it has an input schema).  What's happening here is that you're passing in this dict as the first positional argument.  The result is that you're actually attempting to make the call with the dictionary as the value of taskId.

There are two issues here:

1) Since all args (* or **) are rendered into URL subsections, we should verify that they are in fact strings.  In this case, we should raise an Exception because taskId = {}, not taskId = ""

2) We already check if the API has an 'input' key to handle a possible payload.  It should raise an exception if the API requires a payload.

I've updated the pastebin to work with the library as it exists today here: https://pastebin.mozilla.org/7415034

Two options exist for making this simpler:

a) remove kwarg handling, all api arguments are only positional
b) make it explicit that for all api functions which take an input parameter, assume either the first or last positional argument is the payload.

I'd prefer b), since kwargs are my argument-type-of-choice, but I'm open.


[1]From the README.md:

Options for the topic exchange methods can be in the form of either a single dictionary argument or keyword arguments. Only one form is allowed

  from taskcluster import client
  qEvt = client.QueueEvents()
  # The following calls are equivalent
  qEvt.taskCompleted({'taskId': 'atask'})
  qEvt.taskCompleted(taskId='atask')

Method Payloads are specified through the payload keyword passed to the API method

  from taskcluster import client
  index = client.index()
  index.listNamespaces('mozilla-central', payload={'continuationToken': 'a_token'})

[2] API:
        {
          "type": "function",
          "method": "put",
          "route": "/task/<taskId>",
          "args": [
            "taskId"
          ],
          "name": "createTask",
          "title": "snip",
          "scopes": [
            [
              "queue:create-task:<provisionerId>/<workerType>"
            ]
          ],
          "input": "http://schemas.taskcluster.net/queue/v1/create-task-request.json#",
          "output": "http://schemas.taskcluster.net/queue/v1/task-status-response.json#"
        }

Comment 2

[Jonas Finnemann Jensen (:jonasfj)]

I'm pretty much onboard with (B) too... I think.

IMO, when we support keyword arguments we should interpret arguments as follows:
1) Any parameter given a keyword argument is defined to have the given keyword argument
2) Parameters not given a keyword argument assumes positional arguments in the order they appear in signature.

Example:
Signature: myApi.myMethod(param1, param2, param3, payload)

Example 1:
Valid way to call (with keyword arguments only):
myApi.myMethod(
  param1 = "arg1",
  param2 = "arg2",
  param3 = "arg3",
  payload = {...}
)

Example 2:
Same call without keyword arguments, hence, relying on (2), ie positional arguments:
myApi.myMethod(
  "arg1",
  "arg2",
  "arg3",
  {...}
)

Example 3:
Mixing the two:
myApi.myMethod(
  "arg1",
  "arg2",
  param3 = "arg3",
  payload = {...}
)

Example 4:
Mixing the two (in a bad, but still legal way):
myApi.myMethod(
  param3 = "arg3",
  "arg1",
  "arg2",
  {...}
)

I see no problem supporting all 4 examples. Am I wrong about that?

Note: obviously someone doing example 4 is asking for trouble, but I see no reason not to support it.
I see no reason the poor pattern in example 4 shouldn't work, am I wrong?

Comment 3

[Jonas Finnemann Jensen (:jonasfj)]

Implementation wise, I would do this by starting from the dictionary of keyword arguments...
and then for each parameter that TC reference specifies check if it's in the dictionary, if not then take it's value from the given positional arguments.

If you run out of positional arguments: error
If positional arguments are left over: error
Last you can check if there are more parameters than required in the dictionary, if so: error

Comment 4

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: a couple of fixes

Comment 5

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8526211 [details] [review]
a couple of fixes

I already hit merge :)

This still doesn't facilitate all the example I've I suspect...

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #2)
> I'm pretty much onboard with (B) too... I think.
> 
> IMO, when we support keyword arguments we should interpret arguments as
> follows:
> 1) Any parameter given a keyword argument is defined to have the given
> keyword argument
> 2) Parameters not given a keyword argument assumes positional arguments in
> the order they appear in signature.
> 
> Example:
> Signature: myApi.myMethod(param1, param2, param3, payload)
> 
> Example 1:
> Valid way to call (with keyword arguments only):
> myApi.myMethod(
>   param1 = "arg1",
>   param2 = "arg2",
>   param3 = "arg3",
>   payload = {...}
> )
> 
> Example 2:
> Same call without keyword arguments, hence, relying on (2), ie positional
> arguments:
> myApi.myMethod(
>   "arg1",
>   "arg2",
>   "arg3",
>   {...}
> )
> 
> Example 3:
> Mixing the two:
> myApi.myMethod(
>   "arg1",
>   "arg2",
>   param3 = "arg3",
>   payload = {...}
> )
> 
> Example 4:
> Mixing the two (in a bad, but still legal way):
> myApi.myMethod(
>   param3 = "arg3",
>   "arg1",
>   "arg2",
>   {...}
> )
> 
> I see no problem supporting all 4 examples. Am I wrong about that?

You are.  Keyword arguments cannot appear before positional arguments, either when defining the function or calling one

>>> def hi(*args, **kwargs, payload):
  File "<stdin>", line 1
    def hi(*args, **kwargs, payload):
                          ^
SyntaxError: invalid syntax
>>> hi2(1,a=2,3)
  File "<stdin>", line 1
SyntaxError: non-keyword arg after keyword arg
>>>

And kwargs is not ordered which means there's no mechanism to get the last kwarg passed into the function correctly with **kwargs (https://www.python.org/dev/peps/pep-0468).  That's why we have to make it something like:

def hi(*args, **kwargs):
  if requiresPayload:
    payload = args[0]
    args = args[1:]
  doTheRest()

> Note: obviously someone doing example 4 is asking for trouble, but I see no
> reason not to support it.

Right, I don't think I'd want to do it but I also wouldn't want to stop someone from doing so.  The current implementation is smart enough to understand that someone can use both types.  The checks for the correct number of args collates the kwargs and args before bailing. 

> I see no reason the poor pattern in example 4 shouldn't work, am I wrong?

If we force the payload to be the fist positional argument, we are able to support positional arguments, keyword arguments and have the explicit payload arg, but the only way to be able to have something that looks like:

Api.method('a', 'b', c='c', {})

is to do 

Api.method('a', 'b', c='c', payload={})

Comment 7

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: move-payload.diff

This would make the payload the first argument, instead of being a kwarg.  I'm not entirely sure I like this, but it's the b) option from above.  Changes to the README.md are not included in this patch.

Comment 8

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8526671 [details] [diff] [review]
move-payload.diff

Review of attachment 8526671 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like putting payload as first argument... It'll take a look at this...

And see if I can't figure out how to solve the problem.
(that would make force me to properly read the existing code)

Comment 9

[Jonas Finnemann Jensen (:jonasfj)]

Attachment: Github PR w. proposal for how to handle args

Basically made a few things a bit more robust...
And changed how keyword arguments overwrite positional arguments.

This way, the following examples should work:
A) service.method(arg1, arg2, arg3, payload)
B) service.method(arg1, arg2, arg3, payload = payload)
C) service.method(arg1, arg3, payload, param2 = arg2)
D) service.method(arg1, arg2, param3 = arg3, payload = payload)


I don't recommend (C), it's just bad style.. However, (B) or variations there of like (D) are great... as is (A). Of course I would use indentation like crazy whenever, I give keyword args, why is why I like also have positional ones :)

Note: This was not tested, I couldn't get tests running.
It's just my quick take on how it could be done.

Comment 10

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #9)
> Created attachment 8528119 [details] [review]
> Github PR w. proposal for how to handle args
> 
> Basically made a few things a bit more robust...

And some other things less robust.  I've fixed those.

> And changed how keyword arguments overwrite positional arguments.
> 
> This way, the following examples should work:
> A) service.method(arg1, arg2, arg3, payload)
> B) service.method(arg1, arg2, arg3, payload = payload)
> C) service.method(arg1, arg3, payload, param2 = arg2)
> D) service.method(arg1, arg2, param3 = arg3, payload = payload)
> 
> 
> I don't recommend (C), it's just bad style.. However, (B) or variations
> there of like (D) are great... as is (A). Of course I would use indentation
> like crazy whenever, I give keyword args, why is why I like also have
> positional ones :)

I don't agree with supporting both B/D and A at the same time.  Having two places to put the payload seems like a bad idea all around.

I will fully admit to C looking ugly, but the chances of someone using a *mix* of * and ** is low.

That gives us the supported forms:

i) service.method(arg1, arg2, arg3, payload)
ii)
    apiArgs = { 'arg1': 'a', 'arg2': 'b', 'arg3': 'c' }
    service.method(payload, **apiArgs)

I think both of those forms are completely reasonable and are just fine style-wise.

> Note: This was not tested, I couldn't get tests running.

from the email, you're missing setuptools on your machine

> It's just my quick take on how it could be done.

Yep, I pushed a patch to the branch that fixes all the broken stuff :)

Comment 11

[Jonas Finnemann Jensen (:jonasfj)]

Hmm,
For the record I believe we agreed that (ii) could be done as:
  apiArgs = { 'arg1': 'a', 'arg2': 'b', 'arg3': 'c' , 'payload': payload}
  service.method(**apiArgs)

By, using the "payload" keyword argument if provided, and otherwise pop a positional argument of the end of the list of positional arguments.

Anyways, if not... Let's fix it next week :)

Comment 12

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #11)
> Hmm,
> For the record I believe we agreed that (ii) could be done as:
>   apiArgs = { 'arg1': 'a', 'arg2': 'b', 'arg3': 'c' , 'payload': payload}
>   service.method(**apiArgs)
> 
> By, using the "payload" keyword argument if provided, and otherwise pop a
> positional argument of the end of the list of positional arguments.
> 
> Anyways, if not... Let's fix it next week :)

I have pushed to the PR code that looks good to me.  I *really* don't like being able to provide the payload to the function in two different ways.

I'd be OK with merging the PR as is at this point, all the tests are working

Comment 13

[Jonas Finnemann Jensen (:jonasfj)]

As I wrote on PR#5 I think arg parsing could be more elegant.. maybe split in a case for keyword and a case for positional...

But if you're happy feel free to merge it, we can always fix it later..

IMO, I don't worry so much about breaking compatibility in the client library. People can always keep using the old client library, if they don't bother upgrading.

Comment 14

[Jonas Finnemann Jensen (:jonasfj)]

I don't remember but didn't we resolve this at PDX? And make it all work in a sane way?
(if we should mark this bug as resolved to it's easier to track what we have around).

Comment 15

[John Ford [:jhford] CET/CEST Berlin Time]

Yes.  We decided that there would be two calling conventions:

Component.method(arg1, arg2, payload)
Component.method(payload, arg1=1, arg2=2)

That was implemented.  I just verified that the library will throw if you do not specify a payload where required and when you are missing an argument

taskcluster.exceptions.TaskclusterFailure: Payload is required as last positional arg
taskcluster.exceptions.TaskclusterFailure: listNamespaces takes 1 args, only 0 were given

Comment 16

[Pete Moore [:pmoore][:pete]]

Resetting Version and Target Milestone that accidentally got changed...