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

Bug caused by updated to how Flask handles the file upload field #5020

Merged
merged 4 commits into from
Nov 4, 2019

Conversation

shasan-marsdd
Copy link
Contributor

… filename for the file_url field when I'm not actually uploading a new file, just changing other metadata (such as description). This effectively kills kills the uploaded image link for the organization/groups.

Fixes #

Proposed fixes:

Issue occurs only when I have an image for the organization/groups. I need to be able to update other metadata but not uploading a new image. It breaks due to the fact how old Pylons requests object not returning anything in the "image_upload" field when the user doesn't upload a new image. Flask's FileStorage class has a boolean check instead where we can just do if "data_dict['image_upload']" or similar instead.
I have kept the original if statement in place to be backwards compatible, additional check added in order to account for the Flask method.

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

Please [X] all the boxes above that apply

… filename for the file_url field when I'm not actually uploading a new file, just changing other metadata (such as description). This effectively kills kills the uploaded image link for the organization/groups.
@smotornyuk
Copy link
Member

Given the fact that we are migrating away from Pylons, my comment may be ignored, but there is one issue with Pylons' controllers with this fix. You are checking file instance using implicit boolean casting, which has quite unexpected implementation for cgi.FieldStorage (https://bugs.python.org/issue19097) and you can get unexpected results for Pylons uploads unless you check with explicit is not None

@shasan-marsdd
Copy link
Contributor Author

Since I need to care about actual file upload, and in both cases I have checked the "filename" field instead, which is where the bug is actually occurring. This ultimately should continue to work whether we use cgi.FieldStorage or FlaskFileStorage class. It also reduces need to individually check based on each instance, keeping things cleaner (hopefully).

@shasan-marsdd
Copy link
Contributor Author

Given the fact that we are migrating away from Pylons, my comment may be ignored, but there is one issue with Pylons' controllers with this fix. You are checking file instance using implicit boolean casting, which has quite unexpected implementation for cgi.FieldStorage (https://bugs.python.org/issue19097) and you can get unexpected results for Pylons uploads unless you check with explicit is not None

I did some changes, Not sure if that's ok with you?

@smotornyuk
Copy link
Member

It works pretty well for me, so i'm merging this PR

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

Successfully merging this pull request may close these issues.

None yet

2 participants