datopian / ckanext-blob-storage

CKAN extension to offload blob storage to cloud storage providers (S3, GCS, Azure etc).
http://tech.datopian.com/blob-storage/
MIT License
14 stars 4 forks source link

Create a new CKAN api action `custom_resoruce_show` to return schema and sample attributes as JSON object instead of string #29

Closed mariorodeghiero closed 3 years ago

mariorodeghiero commented 3 years ago

The resource_show for schema and sample is a string with Unicode like the code below and we need this information as a JSON to be able to render/edit in tableschema component when the user is in the edit page.

"{u'fields': [{u'type': u'string', u'name': u'Series_reference', u'format': u'default'}, {u'type': u'number', u'name': u'Period', u'format': u'default'}, {u'type': u'integer', u'name': u'Data_value', u'format': u'default'}, {u'type': u'string', u'name': u'STATUS', u'format': u'default'}, {u'type': u'string', u'name': u'UNITS', u'format': u'default'}, {u'type': u'integer', u'name': u'MAGNTUDE', u'format': u'default'}, {u'type': u'string', u'name': u'Subject', u'format': u'default'}, {u'type': u'string', u'name': u'Group', u'format': u'default'}, {u'type': u'string', u'name': u'Series_title_1', u'format': u'default'}, {u'type': u'string', u'name': u'Series_title_2', u'format': u'default'}, {u'type': u'string', u'name': u'Series_title_3', u'format': u'default'}, {u'type': u'string', u'name': u'Series_title_4', u'format': u'default'}, {u'type': u'string', u'name': u'Series_title_5', u'format': u'default'}], u'missingValues': [u'']}"

more details about the issue in https://github.com/datopian/datapub/issues/37

Acceptance

Resource edit page works correctly

Tasks

shevron commented 3 years ago

I don't like the idea of creating "special cases" like an API identical to resource_show only named custom_resource_show that adds almost the same content but slightly different. I suggest either:

(1) Preferred - we fix resource_show to include the info you need in the right format (I believe this is not very hard esp with recent CKAN versions that can chain core actions) (2) Create an API endpoint to fetch resource scheming information only, and name it appropriately - something like resource_schema_show or similar.

mariorodeghiero commented 3 years ago

@shevron I think can be the option 1 too. as Anuar said ¨Or to override resource_show method in the extension."

cc: @anuveyatsu

shevron commented 3 years ago

Can you test this with CKAN 2.8.5? I think that based on this: https://github.com/ckan/ckan/pull/5453 this should be a non-issue in CKAN 2.8.5 / 2.9.x.

mariorodeghiero commented 3 years ago

Good news, the return works properly for this version ckan 2.8.5.

Result: Screenshot from 2020-09-29 17-24-52

The steps to use this version in ckan-blob-storage is:

  1. Delete the file ckan-version in .make-status folder
  2. change the ckan version in makeFile. Use CKAN_VERSION := ckan-2.8.5
  3. Run make ckan-install
  4. Run make dev-start

@shevron Thanks for letting us know.

cc: @mbeilin @Daniellappv @lgilies

anuveyatsu commented 3 years ago

Really good to know this! Thanks @shevron

shevron commented 3 years ago

@mariorodeghiero I suggest you update the README to require CKAN 2.8.5 and up and close this. If we ever need to support older versions, let's reopen / open a new ticket.

anuveyatsu commented 3 years ago

@shevron looks like it is critical to support older versions, especially, v2.8.2 (sounds like it isn't straightforward to upgrade right away).

What we need here is to get schema and sample properties of a resource as JSON object so we're planning to add custom_resource_show.

Let me know if you have any comments, please.

cc @mariorodeghiero @mbeilin

shevron commented 3 years ago

Ok, if we need to of course let's do it. But I would much rather name things explicitly ("explicit is better than implicit" - zen of Python). Let's call this API based on what it does. Either resource_show_with_schema if it should be exactly like resource_show with added schema related fields, or resource_schema_show if we want to have an API in addition to resource_show that is called separately and only returns the schema related fields we don't have right now.

I like it when APIs do not overlap eachother so I'd vote for implementing resource_schema_show but I understand if we want to have some overlap and do something more convenient for the client that adds things on top of a single call to resource_show. In this case I would just name it resource_show_with_schema.

anuveyatsu commented 3 years ago

@shevron well, I agree with you. I think we can add 2 actions here:

mariorodeghiero commented 3 years ago

FIXED. I opened another issue to add new tests for these actions. https://github.com/datopian/ckanext-blob-storage/issues/32

rufuspollock commented 3 years ago

@anuveyatsu (or @mariorodeghiero) can you please update the issue description with a clearer statement of the what the problem is and how it relates to specific ckan versions - that would save me trying to work that out from the thread.

In general we want to avoid special case code if we can and i don't understand what the issue was.

shevron commented 3 years ago

There is some data (schema and example) that datapub stores on the resource object in CKAN as "extra fields". CKAN will automatically store any extra fields for resources in DB as "extras", but in CKAN version up to 2.8.4 incl. this could only be a scalar value more or less. A patch merged to CKAN 2.8.5 fixes this to JSON serialize / desrialize lists and dicts as well and this means that if you try to store schema as a dict in an extra field in CKAN 2.8.5 you'll get back what you expect, but in CKAN 2.8.4 or lower you'll get a string dump of a Python dict (which is not what you want in the client side).

Since some users of this cannot upgrade to 2.8.5 yet, it was decided to offer special APIs to access these two extra properties.

rufuspollock commented 3 years ago

@shevron what's your view in just requiring v2.8.5 going forward so we can remove this semi-hack addition?