ckan / ckanapi

A command line interface and Python module for accessing the CKAN Action API
Other
181 stars 74 forks source link

Resource uploads fail on LocalCKAN #211

Closed EricSoroos closed 8 months ago

EricSoroos commented 11 months ago

I spent last night tracking down a resource upload issue in 2.9 / python 3.8-- and it looks like a combination of Ckan and CkanAPI.

Uploads were silently failing to save with this code:

files = {'upload': open(csv_path, 'rb')}                                                                                                                          
localapi.call_action(resource_patch, res_dict, files=files)

LocalCkan creates a cgi.FieldStorage, and passes it into the action chain, which ultimately creates a ResourceUpload here: https://github.com/ckan/ckan/blob/2.9/ckan/lib/uploader.py#L262.

        if bool(upload_field_storage) and \
                isinstance(upload_field_storage, ALLOWED_UPLOAD_TYPES):

This is checking for truthy file uploads, and one of the allowed types.

(Pdb) ALLOWED_UPLOAD_TYPES
(<class 'cgi.FieldStorage'>, <class 'werkzeug.datastructures.FileStorage'>)
(Pdb) type(upload_field_storage)
<class 'cgi.FieldStorage'>
(Pdb) isinstance(upload_field_storage, ALLOWED_UPLOAD_TYPES)
True
(Pdb) upload_field_storage
FieldStorage(None, '/var/tmp/no-of-inspections-all-types-of-early-years-services-2023.csv', b'AreaID,RegionID,Area,Year,Q1,Q2,Q3,Q4\r\n23,23,National,2023,606,628,,\r\n')
(Pdb) bool(upload_field_storage)
False

cgi.FieldStorage is list truthy (if it's list member is true), but setting the file doesn't add any fields into the list, so it's falsy. werkzurg.datastructures.FileStorage has different falsy behavior.

I fixed my issue by

                file_name = csv_path.split('/')[-1]
                resc_dict.update({'upload': werkzurg.datastructures.FileStorage(open(csv_path, 'rb'), file_name, content_type='text/csv')})
                toolkit.get_action('resource_patch')(context,
                                                      resc_dict)

So, there are a couple of things.

1) stdlib cgi is deprecated in 3.13, because there's basically no maintainers and there are weird bugs in it. We are going to eventually need to shift off of it. 2) werkzurg works as a drop in replacement here, and should be supported going forward. It will certainly work for local ckan at any rate, because we're going to have it installed already for ckan. 3) Perhaps the truthy test would be better as a is not None in ResourceUpload

Zharktas commented 8 months ago

Stumbled to this as well, our local file uploads have been failing for awhile and I started looking into it. cgi.FieldStorage should be removed as its there only for backwards compatibility and since ckan 2.8 hasn't been supported for awhile, ckanapi could do without it as well.