Convert likely string course ID to an integer (#284) #285
Conversation
…on, throw errors appropriately
| if course_id is not None and isinstance(course_id, int): | ||
| request.session['course_id'] = course_id | ||
| course_id_error = Exception('The canvas_course_id custom LTI variable must be configured.') | ||
| if course_id is not None: |
There was a problem hiding this comment.
Not exactly sure this would ever come up in practice, it'd be more likely than we'd get KeyError with course_id being undefined. However, it is technically possible, since it is None if you set the value of the course_id key to null in the LTI config.
| @@ -158,10 +158,17 @@ def create_user_in_django(request: HttpRequest, message_launch: ExtendedDjangoMe | |||
| user_obj.backend = 'django.contrib.auth.backends.ModelBackend' | |||
| django_login(request, user_obj) | |||
|
|
|||
| if course_id is not None and isinstance(course_id, int): | |||
| request.session['course_id'] = course_id | |||
| course_id_error = Exception('The canvas_course_id custom LTI variable must be configured.') | |||
There was a problem hiding this comment.
Maybe a nitpick, but feels like we should define a custom exception class or maybe use an LtiException here. https://github.com/dmitry-viskov/pylti1.3/blob/master/pylti1p3/exception.py
And these aren't exactly the same error.
In the first raise it's a value error that course_id is not defined as an integer
The second is that it's not configured at all.
So seems like 2 separate exceptions.
There was a problem hiding this comment.
I felt like this was a little of the problem with the existing code, one message was catching for two different errors, and since they were different errors and it was a little hard to tell which was failing.
There was a problem hiding this comment.
Okay, this partially addresses this: da76ae3
I'm somewhat opposed to using pylti1p3.LtiException since the exception is not originating from that library. I could create a custom exception class if you feel strongly about this. That seems like it would be more valuable if we planned to catch and do something about these errors.
Just a point of clarification: the is not None is not actually catching if the variable is not set up at all. That would just be a KeyError, I believe. We aren't doing this for any of the other custom variables, but I could also add that if we want to strengthen this a bit now.
There was a problem hiding this comment.
I'm fine leaving it. I know we created CanvasHTTPError and maybe the errors in here should have been something.
They don't originate from the library but they are related to errors with the library.
The Django-LTI is using LtiException.
And I feel like we've raised exceptions from other packages like GraphQL, Django and maybe Pandas. Not sure if there's a correct answer here. I guess it just makes it easier to catch, if necessary, if we have specific exceptions. But maybe these are just uncaught.
There was a problem hiding this comment.
Some of the verification changes could probably be changed to be more programatic but up to you. I'm fine with with this and the exception looks good.
But a small nitpick, I was reading that some books like Python in a Nutshell and Stack Overflow recommend using a docstring rather than a pass in custom exceptions that do nothing. They call it "maybe perfect". :)
…bles are not present
|
|
||
| custom_param_keys = custom_params.keys() | ||
| if not ( | ||
| USERNAME_KEY in custom_param_keys and |
There was a problem hiding this comment.
I believe this could be written as
if not (set([USERNAME_KEY, COURSE_ID_KEY, COURSE_ROLES_KEY]).issubset(custom_params.keys())):
You could probably also just try/except a KeyError later on 145. I prefer to write EAFP in Python rather than verifying everything. But that's an individual preference. Just trying to make have this a little less verbose.
There was a problem hiding this comment.
Yeah, I like your version better. I actually looked at set before cause it felt like I could do something there, but I missed this method.
I need to do some more research and soul-searching on EAFP. I think my instinct is to look before you leap (LBYL) because I find it more readable and have been burned by imprecise try/excepts before, but there's probably a time and a place for both.
There was a problem hiding this comment.
I read/skimmed this article just now: https://realpython.com/python-lbyl-vs-eafp/. It has some good points about using EAFP in Python, especially to minimize extra checks and prevent race condition issues.
There was a problem hiding this comment.
In Java, you basically have to NPE check almost everything but otherwise it's usually just exception handling everywhere.
Interesting article and the point about the zero-cost exceptions is something I didn't know Python didn't have. So basically Java and Python have very inexpensive exception handling.
I just see patterns like the one here and they seem repetitive and redundant, where they could just be solved with a try/except. This is the case for readability and repetition in the article. And there are cases to use both and not be all-in.
I think this case would fall into the uncommon and unlikely to fail side of the column.
| Use LBYL for | Use EAFP for |
|---|---|
| Operations that are likely to fail | Operations that are unlikely to fail |
| Irrevocable operations, and operations that may have a side effect | Input and output (IO) operations, mainly hard drive and network operations |
| Common exceptional conditions that can be quickly prevented beforehand | Database operations that can be rolled back quickly |
There was a problem hiding this comment.
Definitely room for me to grow here. I'll have to be more mindful of my options for exceptions in the future.
…l-its-umich-edu#285) * Convert likely string course ID to an integer before storing in session, throw errors appropriately * Use different messages with exception * Define custom exception LTILaunchError; throw error if required variables are not present * Switch to docstring; use one_set.issubset(other_set)
This PR aims to resolve #284.