Fixed file objects with hyphen issue (ucfopen#647 ) and added test cases

[nayanpaa]

Proposed Changes

• Based off the description of the issue in File objects get attribute content-type which must be accessed through getattr #647, the issue of attributes with hyphens can be narrowed down to set_attributes in canvas_object.py. Any attribute in the JSOn object with a hyphen("-") is translated to an underscore("_").
• In test_canvas_object.py, I have added a function to include test cases that may target this issue.

Effects

• With these changes, all tests are passing, and the coverage report remains at 100%

[dbosk]

One thought, maybe we should keep the old behavior too? So that both works. Because, while being a bug, this would still be a breaking change. (I don't like the redundancy that is the result of that though. So I prefer it the way it is now.)

[bennettscience]

I think I agree with @dbosk about keeping both in there for now. One solution would be to throw a warning into the function that will display when files are downloaded. DeprecationWarning is suppressed by default, so UserWarning might be the best course. Here's how warnings are used in the Canvas module:

if "http://" in base_url:
    warnings.warn(
        "Canvas may respond unexpectedly when making requests to HTTP "
        "URLs. If possible, please use HTTPS.",
        UserWarning,
    )

Don't forget to import warnings at the top of the module.

[nayanpaa]

I see, I think when I approached this issue I assumed that hypens were common in many attributes. So now I will only translate hyphens to underscores in the case that I get the attribute "content-type"?

[bennettscience]

As far as I know, that's the only attribute returned specifically from Canvas with a hyphen in the name. I don't have any guess as to how many people are operating on files, but this would be a tricky bug to sort out without any sort of lead time. Keeping the old content-type attribute with a warning pointing people to update to content_type will prevent major headaches. We can also mark the hyphenated attribute to be removed in a future version.

[bennettscience]

These look good to me 👍

[nayanpaa]

Build failed, I think my code didn't follow the contribution guidelines. Just committed the reformatted code.

[Thetwam]

Hey @nayanpaa, thanks for taking the time to work on this!

Unfortunately, there's a little more nuance to this issue than meets the eye. I'm going to create a few (failing) test cases that demonstrate the issues.

Also the test suite is currently failing due to some weirdness from Codecov, but I don't think that's related to this PR. Don't worry about that bit.

I invite any feedback from you, @dbosk, @bennettscience, and anyone else about potential solutions to these concerns.

[Thetwam]

I appreciate what @bennettscience was suggesting here, but unfortunately the user doesn't typically have control over what gets passed into this function. Canvas is the one who sends the content-type attributes back, and we have to accept them as-is. Adding this will result in a bunch of warnings that the user will have no recourse to fix. We also wouldn't be able to remove the attribute in a future version until Canvas does.

[Thetwam]

Upon re-reading the discussion, was the intention to warn users who were trying to access content-type via getattr? In that case, a warning could make sense but it would have to be on the getter function, not the setter function as it is here.

I think modifying __getattribute__ could do the trick?

[jonespm]

Thanks @Thetwam I just looekd at this too and was thinking the same, that it's probably better to have this in a __getattribute__ or possibly even better in __getattr__ rather than in the set_attributes. Then you don't lose a similarly named attribute with.

__getattr__ would only catch the ones that don't exist so wouldn't need to run through for everything.

I think it's unlikely they'd have a content_type and content-type but it's possible for other headers.

[Thetwam]

TIL there's difference between __getattribute__ and __getattr__.

However, if I'm understanding it correctly, we'd still want to use __getattribute__ for the time being, since the current behavior is to keep content-type while adding content_type.

I moved the warning to __getattribute__, which passes the test__getattribute__content_type_warns test, whereas __getattr__ fails (doesn't throw the expected warning)

[jonespm]

Nevermind, my idea wouldn't work as it is setting the values right on the class rather than as an dictionary in the class. This seems good to me.

[Thetwam]

While the problem here is with content-type, it technically happens with any value that contains a -. I'm not aware of any other similar values in Canvas' responses, but in theory it's possible. Whatever solution we decide on for content-type could be generalized by replacing all - with _.

If there are major concerns with a broader approach, I'm happy to adopt a content-only fix for now and spin it off into its own issue for later.

[bennettscience]

On a cursory glance, I couldn't see any other response attributes that had hyphens. There are some other endpoints where camelCase is used (like the result and score endpoints), but those are only to report to IMS and LTI tools.

I think the question for me is which is more Pythonic - changing all the returned properties into class attributes or keeping the object as close to the returned structure (ie, keeping a hyphenated name and requiring the getattr method) from Canvas so the library matches the docs?

[jonespm]

I was also thinking of making it more generic, like if the value contained a hyphen to set it as a value with an underscore. I think I'm fine for making this into a future issue.

It really might be easiest to change this class to store all of the variables in a dictionary. Then the __getattribute__ and __getattr__ can be used to return from this dictionary by default. Then we can store both forms of the value with and without underscores. I'm fine with that as a separate issue but seems like it might be harder to get that fixed.

[dbosk]

The original PR proposed the general replace-all-hyphens-with-underscores. I also favour that solution. Canvas might add new attributes that have hyphens instead of underscores.

I also think it makes the most sense to keep the hyphenated version and provide the underscored version for convenience, to match the documentation as mentioned above. But it's important to get it right so that they sync, in case they're changed (although it doesn't make sense to change this particular attribute).

Edit: Looked at @jonespm's solution below, I also think that's the way to do it.

[Thetwam]

This test brings up some bigger questions about how we want to handle the case where both the - and _ versions of what is otherwise the same variable are set. In this case, the order of the items in the dictionary matters. I'll create and push new a test case the same as this one but reversing the order to demonstrate.

It's pretty unlikely that Canvas would send back two attributes with only the -/_ differentiating them, let alone with different values, but it's something we'll need to consider how we want to handle.

[jonespm]

Yeah, I thought about this too, I think we can solve both of those problems but maybe in a separate issue. I don't think it's super likely either.

[dbosk]

Would it be worth adding tests that detect if Canvas would ever do this?

I do quite some API "reverse engineering" of APIs I want to use but don't have any official documentation (they're for "internal" use of the web UI). Then I add tests that will fail whenever some return value from the server changes.

Such tests must be added as a separate test suite run for this particular purpose. It would require a Canvas instance to run them against and, consequently, a login and token to the API. So not the ordinary test setup.

I'm leaning towards not worth it. Canvas is pretty stable and documents everything. So the likelihood of being useful is close to zero.

[jonespm]

I modified this to be more generic by internally storing these as dict so anything that has a dash or not a dash in i works. Both values dashed and not dashed are accessible. This passes all of the existing tests but I removed the test for the DeprecationWarning since I'd removed it. Can have this as a followup or just addon to this issue. There was a special case in this code where validation_token still needed to actually be an actual attribute so I just left that as-is.

78620a0

[bennettscience]

That change makes sense to me...I had to do a bunch of reading to understand the difference between __getattr__ and __getattribute__, so I appreciate that this handles either case. It's also more robust if Instructure decides to do weird things with models down the road like they've done with quiz and new_quiz objects.