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

Awkward to set context_codes[] parameter in Calendar.list_calendar_events call #55

Closed
liblit opened this issue Aug 25, 2017 · 11 comments
Closed
Assignees
Labels

Comments

@liblit
Copy link
Contributor

liblit commented Aug 25, 2017

The Canvas.list_calendar_events method feeds its parameters down to a GET /api/v1/calendar_events request. That request accepts a context_codes[] parameter, but setting that parameter correctly is surprisingly awkward in version 0.6.0 of canvasapi. The problem is that the parameter name really must be context_codes[], not context_codes, but creating a Python keyword argument with brackets in its name is subtle:

context = 'course_123456'

canvas.list_calendar_events(context_codes=context) 
# canvasapi.exceptions.CanvasException: API encountered an error processing your request

canvas.list_calendar_events(context_codes=[context])
# canvasapi.exceptions.CanvasException: API encountered an error processing your request

canvas.list_calendar_events(context_codes[]=context)
# SyntaxError: invalid syntax

canvas.list_calendar_events('context_codes[]'=context)
# SyntaxError: invalid syntax

canvas.list_calendar_events(**{'context_codes[]': context})
# success!

The syntactically invalid ones I knew were wrong before I even tried them. But for the first two attempts shown above, I guess I expected canvasapi to add the brackets for me either because it knew that context_codes[] was special, or perhaps it would always append brackets when I passed a value as a list ([context]) rather than a single item.

Arguably this is my fault: give the wrong parameter name, and of course things will fail. But I would have hoped that canvasapi would make this easier. The use of **{...} is obscure enough that many canvasapi users may not know how to do it at all.

@Thetwam Thetwam added the bug label Aug 25, 2017
@Thetwam
Copy link
Member

Thetwam commented Aug 25, 2017

Hey @liblit, thanks for reporting this issue!

Yeah, this is definitely not intentional behavior. Good idea to use **{...} syntax, that's a nice workaround!

Despite the documentation listing the type as string, it looks like it's supposed to accept a list. Because of that, in my mind, context_codes=[context] makes the most sense.

Do you think implementing it such that passing in a list as an argument auto-adds the brackets would be sufficient?

@liblit
Copy link
Contributor Author

liblit commented Aug 25, 2017

You are most welcome, @Thetwam. Thank you for providing canvasapi in the first place! It has been a big help to me.

Despite the documentation listing the type as string, it looks like it's supposed to accept a list

Ah, good catch about Canvas's own API documentation giving the wrong type (or at least a confusing type) for this parameter. If I look for other examples of attributes ending in [], I see quite a mix in Canvas's API documentation: some are described as Array while others are described as string.

Do you think implementing it such that passing in a list as an argument auto-adds the brackets would be sufficient?

Yes, that seems quite natural. Perhaps you could add brackets for tuples as well? I generally use tuples rather than lists when the number of items is fixed and known to me in advance.

@liblit
Copy link
Contributor Author

liblit commented Aug 27, 2017

There is something strange about that context_codes[] parameter. I was recently tracing through the low-level details of a completely different call: Canvas.list_multiple_submissions, which turns into a GET /api/v1/courses/:course_id/students/submissions request. As documented here, this request describes an assignment_ids[] parameter of type string. I am able to set that parameter in the natural, obvious way using either a single value, a list of values, or a tuple of values:

submissions = connect.list_multiple_submissions(assignment_ids=123,        ...)  # OK
submissions = connect.list_multiple_submissions(assignment_ids=[123, 456], ...)  # OK
submissions = connect.list_multiple_submissions(assignment_ids=(123, 456), ...)  # OK

All of the above work. For a single value, this turns into a request URL that includes ...&assignment_ids=123&.... For multiple values, this turns into a request URL that repeats the parameter name for each value: ...&assignment_ids=123&assignment_ids=456&.... The logic for turning lists and tuples into repeated parameter settings comes from down deep in the requests module, and can be seen in an example in the requests documentation. Notice that I do not need to include [] in the parameter name, even though the Canvas documentation identifies the parameter as assignment_ids[] rather than simply assignment_ids.

Compare this with the context_codes[] parameter. The Canvas documentation includes [] as part of the names of both the assignment_ids[] and context_codes[] parameters, and claims that both parameters have type string. Yet while assignment_ids[] works properly in a request URL with one or more bracket-free assignment_ids parameters, context_codes[] apparently must keep its brackets in the request URL.

Weird. Is this a Canvas bug?

@liblit
Copy link
Contributor Author

liblit commented Aug 27, 2017

Yet further experimentation shows that ...&assignment_ids=123&... works properly only for a single value. Repeating it for multiple values (...&assignment_ids=123&assignment_ids=456&...) actually ignores all but the last value. To really provide multiple values, you must repeat the setting with brackets added to the attribute name: ...&assignment_ids[]=123&assignment_ids[]=456&....

So I think this brings us back to where we were earlier, with @Thetwam’s suggested fix:

Do you think implementing it such that passing in a list as an argument auto-adds the brackets would be sufficient?

Yes, I still think this is the right approach. Furthermore, it seems this should be done for all parameters, not merely context_codes[]. Across all of the various methods, instance.method(param=values) when values is a list or tuple should add [] to the end of the param name before passing it down further into the requests layer. We can then rely on the requests logic itself to handle repeating the bracket-added param[] name as many times as needed.

@Thetwam Thetwam added this to the CanvasAPI v0.7.0 milestone Aug 28, 2017
@Thetwam
Copy link
Member

Thetwam commented Aug 28, 2017

What you've stumbled upon is a feature of the Requests library. Given a list, it automatically splits the parameters up. Some webservers accept ...&assignment_ids=123&assignment_ids=456&... as a list, but Canvas does not. For it to be treated as a list, it must have [] appended to the end of the param.

So, my earlier suggestion of the library auto-appending [] as-needed should work...

...until we run into stuff like creating grading standards with params like grading_scheme_entry[][name] and grading_scheme_entry[][value]. It's also worth noting these are order-sensitive.

The way we currently handle kwargs is by flattening the dictionaries into entries that look like param[key1][key2][etc], then sending a single params or data dictionary with those kwargs to the appropriate Requests get/post/etc method. This breaks down with the grading_scheme_entry examples I give above because the keys don't follow 2 important principles of dictionaries:

  1. Dictionaries have no order, but the order of these params matters
  2. Dictionary keys must be unique, but duplicate keys are allowed for this endpoint. For example, the following set of arguments is valid: (I split them up for readability)
grading_scheme_entry[][name]=pass
grading_scheme_entry[][value]=50
grading_scheme_entry[][name]=fail
grading_scheme_entry[][value]=0

We can probably get around number 1 by using OrderedDict from collections, but number 2 has thrown me for a loop.

@liblit
Copy link
Contributor Author

liblit commented Aug 28, 2017

@Thetwam, thank you for your patience as I haphazardly rediscover things you already know about Requests, REST, etc.

If I understand the problem correctly, it arises in the requests.requests API. This method is documented as taking optional params as “Dictionary or bytes”. No dict you could possibly provide will ever cause requests to produce the sequence of parameters you need, because those parameters might need to have repeated names and a dict cannot do that. So that makes it seem as though the only alternative is to provide the parameters as “bytes”, presumably after doing all of your own encoding, escaping, ampersand-delimiting, etc. That seems tedious but doable.

However, tracing down into the requests parameter-processing code more deeply, eventually I reach RequestEncodingMixin._encode_params, documented as follows:

Encode parameters in a piece of data.

Will successfully encode parameters when passed as a dict or a list of 2-tuples. Order is retained if data is a list of 2-tuples but arbitrary if parameters are supplied as a dict.

So this makes me think that the “Dictionary or bytes” mentioned in the requests.requests documentation may have been incomplete. I think that it may also work to pass params as a list.of 2-tuples. It is certainly worth trying! If that does indeed work, then you can get the right parameters for grading scheme creation by using something like:

[('grading_scheme_entry[][name]', 'pass'),
 ('grading_scheme_entry[][value]', 50),
 ('grading_scheme_entry[][name]', 'fail'),
 ('grading_scheme_entry[][value]', 0)]

Regarding the higher-level API for creating grading scheme entries, perhaps we can leverage my relative ignorance. Let me ponder how I would expect to provide data for such a call. Reading just the Create a new grading standard Canvas REST API documentation, and knowing very little about how such things work, I would assume that one of the following styles would work:

  1. Provide a list of GradingSchemeEntry instances, where GradingSchemeEntry is a canvasapi-provided Python class that encapsulates a single name and a single corresponding value

  2. Provide a list of dict. Each such dict must contain a name key mapping to a string, a value key mapping to an int, and nothing else.

  3. Provide a dict containing a name key and a value key. The name key maps to a list of string, and the value key maps to a list of int. Both lists must be the same length. That same-length requirement is why I find this the least desirable alternative.

  4. Provide a list of tuple. Each such tuple must consist of a (string, int) pair. Note that this can be combined with my first idea if GradingSchemeEntry is a namedtuple.

In each of these cases, I am expecting to pass in a single value for the grading_scheme_entry parameter, as in course.create_grading_scheme(title=..., grading_scheme_entry=something) where something is a piece of data styled according to one of the suggestions in my list. I would have a hard time guessing which of the above is the right thing to use, unless canvasapi generously provided documentation to tell me. Lacking that, I would probably search around for some canvasapi call that would retrieve an existing grading scheme, and I would assume that the format of data returned by this retrieval call would resemble the format of data I should provide when creating a new scheme.

@Thetwam
Copy link
Member

Thetwam commented Aug 28, 2017

That list of tuples thing looks really interesting, gonna give that a shot right now.

As far as what the developer can expect to pass, I'd imagine it looking something like:

g_scheme = [
    {
        'name': 'pass',
        'value': 50
    },
    {
        'name': 'fail',
        'value': 0
    }
]
course.create_grading_scheme(title='Title', grading_scheme_entry=g_scheme)

I would have a hard time guessing which of the above is the right thing to use, unless canvasapi generously provided documentation to tell me.

Yeah, we've had trouble expressing exactly how we handle nested kwargs in the documentation (Hence this issue: #9). Figuring out how to handle these weird args will go a long way in making that documentation easier to write/generate!

@liblit
Copy link
Contributor Author

liblit commented Aug 28, 2017

As far as what the developer can expect to pass, I'd imagine it looking something like:

That matches option 2 from my list. I would find this perfectly usable and unsurprising: two qualities that a good API should exhibit.

@Thetwam
Copy link
Member

Thetwam commented Aug 28, 2017

Good news! I did a quick test using the list of 2-tuples format and it worked!

I'm still working on the best way to translate the format that I mentioned before into the list of 2-tuples format, but I can see the light at the end of the tunnel!

jessemcbride pushed a commit that referenced this issue Aug 30, 2017
@jessemcbride
Copy link
Member

@Thetwam Can we revisit this following PR #63's acceptance?

@Thetwam
Copy link
Member

Thetwam commented Sep 5, 2017

@jessemcbride Issue is essentially complete, but not merged into master yet. Once we do a release, I'll close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants