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.

Sign up for GitHub

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

Fix Upload Field Condition #96

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix Upload Field Condition #96

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef173d9
fix(dev): FieldStorage support;
JVickery-TBS Feb 14, 2024
6210210
fix(dev): FieldStorage support;
JVickery-TBS Feb 16, 2024
dfd3685
fix(syntax): condition;
JVickery-TBS Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ckanext/validation/plugin/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,9 @@ def before_update(self, context, current_resource, updated_resource):
needs_validation = False
if ((
# New file uploaded
updated_resource.get(u'upload') or
(updated_resource.get(u'upload') or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be an and. We want to check that the object exists and contains a non-blank file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThrawnCA the new getattr default to False, so do not need to check for upload object, if it is not an object or does not have those attributes, it will be False.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JVickery-TBS which versions of ckan are we testing the validation plugin against?
i see https://github.com/ckan/ckanext-validation/blob/master/.github/workflows/test.yml 2.9 only here

we have bumped ours to do 2.9 and 2.10: https://github.com/qld-gov-au/ckanext-validation/blob/master/.github/workflows/test.yml

and we have started introducing 'master/head' testing (allowing failures) so we know what's coming + allowing 'forks' to be tested again (i.e. qld-gov-au ckan delta that has the altered upload interface to allow 'download' in once nice place): https://github.com/ckan/ckanext-xloader/blob/master/.github/workflows/test.yml#L26

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the ckan/ckanext-validation is only 2.9 as it does not have 2.10+ support in this repo yet? That I know of at least. Unless if you added it.

Copy link
Contributor

@ThrawnCA ThrawnCA Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is not an object or does not have those attributes, it will be False.

But that's why we need and, to make that False take effect and override the True resulting from the object's presence.

If the object is present, but lacks both file and filename, then this will proceed when it shouldn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThrawnCA right okay. Sorry I kind of forgot what this fix was for. I cannot test for the updated_resource.get(u'upload') AND because updated_resource.get(u'upload') is going to be false if it is a cgi.FieldStorage object. Here is the issue in cgi: https://bugs.python.org/issue19097

So that is why we have to just check for the filename or file.

I guess we could check the class type of updated_resource.get(u'upload')? and if it is an instance of FieldStorage then check for the filename or file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThrawnCA okay, I modified the condition now, so we check for cgi.FieldStorage and then check if the filename or file attribute is there. Put an inline comment regarding the bug we are working around here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...that is an inconvenient bug indeed, but the current logic still isn't right. We still proceed any time upload is truthy, regardless of the filename and file fields, which is a problem when dealing with Werkzeug storage containing no data.

What about something like "(upload is either truthy or an instance of cgi.FieldStorage) and (either file or filename is present)"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that bug says it's fixed on Python 3, which is now the only supported version for this plugin. Do we still need to worry about it?

getattr(updated_resource.get(u'upload'), 'file', False) or
getattr(updated_resource.get(u'upload'), 'filename', False)) or
# External URL changed
updated_resource.get(u'url') != current_resource.get(u'url') or
# Schema changed
Expand Down