canvasapi icon indicating copy to clipboard operation
canvasapi copied to clipboard

combine_kwargs erroneously parses `file` argument

Open jessemcbride opened this issue 4 years ago • 0 comments

Describe the bug

In https://github.com/ucfopen/canvasapi/pull/222, we added support for passing the files dict sent by requests.post directly:

# Grab file from data.
files = None
for field, value in data:
    if field == "file":
        if isinstance(value, dict):
            files = value
        else:
            files = {"file": value}
        break

# Remove file entry from data.
data[:] = [tup for tup in data if tup[0] != "file"]
return self._session.post(url, headers=headers, data=data, files=files)

Unfortunately, this has a strange interaction with combine_kwargs, where combine_kwargs will iterate over the value and spit out something like this:

('author', 3889181),
 ('message', 'this is a test'),
 ('user_can_see_posts', 'true'),
 ('published', 'true'),
 ('file[attachment][]', b'#!/usr/bin/env python\n'),
 ('file[attachment][]', b'import os\n'),
 ('file[attachment][]', b'import sys\n'),
 ('file[attachment][]', b'\n'),
 ('file[attachment][]', b'if __name__ == "__main__":\n'),
 ('file[attachment][]',
  b'    os.environ.setdefault("DJANGO_SETTINGS_MODULE", "config.settings.loc'
  b'al")\n'),
 ('file[attachment][]', b'\n'),
 ('file[attachment][]', b'    try:\n'),
 ('file[attachment][]',
  b'        from django.core.management import execute_from_command_line\n'),
 ('file[attachment][]', b'    except ImportError:\n'),
 ('file[attachment][]',
  b'        # The above import may fail for some other reason. Ensure that t'
  b'he\n'),
 ('file[attachment][]',
  b'        # issue is really that Django is missing to avoid masking other\n'),
 ('file[attachment][]', b'        # exceptions on Python 2.\n'),
 ('file[attachment][]', b'        try:\n'),
 ('file[attachment][]', b'            import django  # noqa\n'),
 ('file[attachment][]', b'        except ImportError:\n'),
 ('file[attachment][]', b'            raise ImportError(\n'),
 ('file[attachment][]',
  b'                "Couldn\'t import Django. Are you sure it\'s installed and'
  b' "\n'),
 ('file[attachment][]',
  b'                "available on your PYTHONPATH environment variable? Did '
  b'you "\n'),
 ('file[attachment][]',
  b'                "forget to activate a virtual environment?"\n'),
 ('file[attachment][]', b'            )\n'),
 ('file[attachment][]', b'\n'),
 ('file[attachment][]', b'        raise\n'),
 ('file[attachment][]', b'\n'),
 ('file[attachment][]',
  b'    # This allows easy placement of apps within the interior\n'),
 ('file[attachment][]', b'    # soulpatch directory.\n'),
 ('file[attachment][]',
  b'    current_path = os.path.dirname(os.path.abspath(__file__))\n'),
 ('file[attachment][]',
  b'    sys.path.append(os.path.join(current_path, "soulpatch"))\n'),
 ('file[attachment][]', b'\n'),
 ('file[attachment][]', b'    execute_from_command_line(sys.argv)\n')]

To Reproduce

Steps to reproduce the behavior:

  1. On any endpoint that accepts a file upload, pass the file handler as part of a dictionary with the keyword argument file:

    create_discussion_topic(file={"attachment": open("some-attachment.txt")})
    
  2. Watch as it doesn't work.

  3. Bonus: Enable logging to see the insane output generated by combine_kwargs.

Expected behavior

Passing a dictionary named files should allow me to send the parameter names expected by Canvas with the files to upload. In the case of course.create_discussion_topic(), the parameter name is attachment.

Environment information (please complete the following information)

  • Python version (python --version): 3.8.0a2+ (though it is present in all versions)
  • CanvasAPI version (pip show canvasapi): 0.14 (introduced in 0.12)

Additional context

We may want to consider exposing a keyword argument like _files or something that will always take the files dictionary, rather than using a name like file which could potentially conflict with required arguments in various endpoints.

jessemcbride avatar Oct 11 '19 20:10 jessemcbride