Fix Upload Field Condition

[JVickery-TBS]

fix(dev): FieldStorage support;

• Added condition to check for file attribute from FieldStorage object for ckanapi support.

CKANAPI uses cgi.FieldStorage (https://github.com/ckan/ckanapi/blob/master/ckanapi/localckan.py#L67) as to not require a lot of dependencies. Related issue on cgi.FieldStorage: https://bugs.python.org/issue19097

To support all py versions and cgi dependency versions, just checking for the file attribute might be good enough?

[ThrawnCA]

We've seen situations before where the FileStorage object exists but is blank, and it's necessary to further check that it has a truthy filename.

[JVickery-TBS]

@ThrawnCA reckon I should add the filename attribute condition after the new file one? Or just replace the new file condition with filename?

[ThrawnCA]

Offhand I'm not sure whether testing file and testing filename are interchangeable.

[JVickery-TBS]

@ThrawnCA good enough for me! haha, I have added the filename condition in the case that the file attribute of a File like object is empty, but there is a filename. Should still get queued for validation and then the validation will fail and tell the user that their file is empty. This make sense to me

[ThrawnCA]

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

[JVickery-TBS]

@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.

[duttonw]

@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

[JVickery-TBS]

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.

[ThrawnCA]

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.

[JVickery-TBS]

@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?

[JVickery-TBS]

@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.

[ThrawnCA]

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)"?

[ThrawnCA]

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?