[Draft] Proof of concept for xloader site url #234

pwalsh · 2024-11-18T08:27:21Z

We want to be able to communicate within a docker network without using the public site_url. This is a minimal proof of concept for achieving this.

ThrawnCA · 2024-11-18T23:07:54Z

CKAN 2.11 build is broken by ckan/ckan-docker-base#86 and needs to run the test container as root to work around it.

wardi · 2024-11-19T14:00:40Z

ckanext/xloader/config_declaration.yaml

+        default:
+        description: |
+            Provide an alternate site URL for the xloader_submit action.
+            This is useful, for example, when the site is running within a docker network.


Suggested change

This is useful, for example, when the site is running within a docker network.

This is useful, for example, when the site is running within a docker network

or where the job runner can't access ckan using its normal public site URL.

wardi · 2024-11-19T14:02:34Z

ckanext/xloader/config_declaration.yaml

@@ -2,6 +2,12 @@ version: 1
 groups:
  - annotation: ckanext-xloader settings
    options:
+      - key: ckanext.xloader.site_url
+        default:


I think we don't need an empty string default, it's normal for optional settings to be not present if not provided

Suggested change

default:

instead of the toolkit.config.get("ckanext.xloader.site_url") or toolkit.config.get("ckan.site_url") logic below we should be able to use just toolkit.config.get("ckanext.xloader.site_url") along with:

validators: configured_default("ckan.site_url",None)

@wardi ok I see. so that default is just set in the config yaml, and then, any code can assume that fallback default for the ckanext.xloader.site_url setting?

yeah, this validator will set ckanext.xloader.site_url to the same as ckan.site_url when it's not given. Another one of @smotornyuk 's clever ideas.

Do we need to bother with that when jobs.py already has the fallback config.get('ckanext.xloader.site_url') or config.get('ckan.site_url')?

IMO we should keep the fallback in jobs.py in case someone manually sets ckanext.xloader.site_url to a blank value (which could happen if eg the config is being automatically generated/populated from somewhere, perhaps using ckanext-ssm-config), so the validator approach is redundant.

wardi · 2024-11-19T14:04:34Z

ckanext/xloader/plugin.py

+        site_url_configs = ("ckan.site_url", "ckanext.xloader.site_url")
+        if not any(site_url_configs):
+            raise Exception(
+                f"One of config options {site_url_configs} must be set to use ckanext-xloader."
+            )


ckan will refuse to start if a site_url isn't provided, so this code would never get executed

@wardi here, I'm just extending the existing check for that. I prefer not to remove existing behavior, but just to provide the minimal new behavior required for the PR

https://github.com/ckan/ckanext-xloader/blob/master/ckanext/xloader/plugin.py#L64

If you did not cleanup the error message, this would not have been commented upon ;)

I'm fine leaving this in as its a belts and braces approach, better to fail early (if it does occur which is very very remote)

duttonw · 2024-11-21T02:22:03Z

ckanext/xloader/tests/test_jobs.py

+        "result_url": callback_url,
+        "metadata": {
+            "ignore_hash": True,
+            "ckan_url": toolkit.config.get("ckanext.xloader.site_url")


going to need more tests created to set these two values for proper validation.
one for default site url, another for the xloader override value. updating the fixture is so/so.

duttonw

would like to see more tests to ensure all paths are covered.

pwalsh · 2024-11-21T10:51:37Z

@duttonw yeah sure, will add tests, this is in draft as it is not ready for review by maintainers, I'm just doing a minimal validation of the approach ( I work with @wardi and @amercader at Link Digital ).

JVickery-TBS · 2024-12-09T21:56:20Z

Does this need to also handle the download of the temp file in _download_resource_data method of ckanext.xloader.jobs?

ckanext/xloader/config_declaration.yaml

ckanext/xloader/jobs.py

missed a callback_xloader_hook call, need to update result_url here too

ThrawnCA · 2025-01-16T11:49:51Z

ckanext/xloader/jobs.py

+            callback_url = config.get('ckanext.xloader.site_url') or config.get('ckan.site_url')
+            callback_url = urljoin(
+                callback_url.rstrip('/'), '/api/3/action/xloader_hook')
+            result_url = callback_url


Why create result_url at all?

You mean assign callback_url back to result_url ? I'm try to minimise changes to the rest of the XLoader code. If you think I should just use callback_url then I can do that

You're already changing from result_url=input['result_url'] to result_url=result_url. Why not just change it to result_url=callback_url? Or just call it result_url instead of callback_url on line 215 in the first place.

duttonw

As it is, it will break external urls for csv files. I also suggest creating a util function that does the url swapping on a per url basis if it is using the 'site_url' that is in our config's. (so it does not break external urls).

duttonw · 2025-01-16T23:03:23Z

ckanext/xloader/jobs.py

@@ -79,9 +81,15 @@ def xloader_data_into_datastore(input):
    # First flag that this task is running, to indicate the job is not
    # stillborn, for when xloader_submit is deciding whether another job would
    # be a duplicate or not
+
+    callback_url = config.get('ckanext.xloader.site_url') or config.get('ckan.site_url')


since this snippet is everywhere, can we make a function and place it in a util/helper function so all places can reference the same code.

i.e. instead of just getting the callback_url, it should take a 'url' and replace the 'site_url' with 'xloader.site_url' when set. or append if domain is not available (which is very rare).

In the jobs table . result_url is always a fqdn i.e. https://www.data.qld.gov.au/api/3/action/xloader_hook

I also see that you dropped input['result_url'] which containers a lookup of the api endpoint. I'm unsure why we should be 'hacking' the callback_url instead of using a 'generic' replacement function to swap the domain like you are doing to the 'resource' url.

The metadata on the 'input' is below

callback_url = p.toolkit.url_for( "api.action", ver=3, logic_function="xloader_hook", qualified=True ) data = { 'api_key': utils.get_xloader_user_apitoken(), 'job_type': 'xloader_to_datastore', 'result_url': callback_url, 'metadata': { 'ignore_hash': data_dict.get('ignore_hash', False), 'ckan_url': config['ckan.site_url'], 'resource_id': res_id, 'set_url_type': data_dict.get('set_url_type', False), 'task_created': task['last_updated'], 'original_url': resource_dict.get('url'), } }

duttonw · 2025-01-17T00:41:22Z

ckanext/xloader/jobs.py

+        callback_url.rstrip('/'), resource_uri)
+
+    url = callback_url
+    url_parts = urlsplit(url) # reparse the url after the callback_url is set


This will break all 'external' url's which are not internally referenced. We should be using a 'generic' function that takes a url and does the swap if the original url.

Details below on my reasoning:

I see in my jobs stats table items is

{"metadata": {"ignore_hash": false, "ckan_url": "https://www.data.qld.gov.au", "resource_id": "5d1e8368-7ec3-435a-92d0-280ad1e3db0d", "set_url_type": false, "task_created": "2025-01-11 00:15:03.218182", "original_url": "https://apps.des.qld.gov.au/data-sets/water/gregory-environmental.csv?timestamp=2025-01-10T10:15:02+10:00", "datastore_contains_all_records_of_source_file": true, "datastore_active": true}, "status": "complete"}

using this as an example. The input of the _download_resource_data function is
res_dict = get_action('resource_show')(context, {'id': resource_id})

looking at : https://www.data.qld.gov.au/api/action/resource_show?id=5d1e8368-7ec3-435a-92d0-280ad1e3db0d

on line 292, it gets the url which would be:

"url": "https://apps.des.qld.gov.au/data-sets/water/gregory-environmental.csv?timestamp=2025-01-16T10:15:01+10:00",

Which since you are 'force' changing the url, instead of checking its pointing to the CKAN site url first. it would then end up with

http://localhost:5000/data-sets/water/gregory-environmental.csv?timestamp=2025-01-16T10:15:01+10:00
if the ckanext.xloader.site_url is http://localhost:5000

which would then be a 404 or hanging situation.

Thanks @duttonw for taking the time to help me with this. It is much appreaciated.

Yes, I agree that a util/helper function is needed to swap out the CKAN URL

With regards to your scenario, why would you set ckanext.xloader.site_url to something if it broke communication? The ckanext.xloader.site_url config option does not exist at the moment in master. If ckanext.xloader.site_url is not set then the callback_url would just fall back to ckan.site_url which would exist as it is mandatory. So in your case it would be set to http://www.data.qld.gov.au/data-sets/water/gregory-environmental.csv?timestamp=2025-01-16T10:15:01+10:00 as I think your ckan.site_url is set to http://www.data.qld.gov.au

Update: actually if ckanext.xloader.site_url is not set, then that config option (ckanext.xloader.site_url) would be set to the value for ckan.site_url through the validator for ckanext.xloader.site_url in the config_declaration.yaml file. Maybe that's not a great idea if we ever want to keep those 2 parameters distinct

Hi @kowh-ai ,

http://www.data.qld.gov.au/data-sets/water/gregory-environmental.csv?timestamp=2025-01-16T10:15:01+10:00 is a 404, since its not on www.data.qld.gov.au, but on apps.des.qld.gov.au (non ckan endpoint)

With the new function def get_ckan_url():
just update it to take a url. And it would alter it if incoming url 'matches' ckan.site_url at the start of the string and replace it with ckanext.xloader.site_url if not empty/blank string. (or if no domain is set, inject).

This will still need at least another 5 unit tests for this new function to 'verify' that it does what we expect it to do. i.e. not break external data assets.

somethings i've been thinking for a while is that if you have the 'archiver' plugin installed, like how there is a use case to 'wait' for the validation plugin to complete successfully. If the archiver has 'saved' the resource locally, then we could/should use that ingest into the datastore. The only negative is like these external csv files which are updated adhoc and the 'manual datastore upload' won't collect the latest like it does now. (harvester normally is set up to run on initial upload and then can be setup to run adhoc { for our instance monthly } , this is too slow and will cause frustration for authors who wish to have the latest from an external csv )

duttonw · 2025-01-17T00:42:26Z

ckanext/xloader/tests/test_jobs.py

-            'task_created': datetime.utcnow().isoformat(),
-            'original_url': resource["url"],
-        }
+        "api_key": apikey,


please also add a test for 'external' url which is not using the site_url for the file.

hopefully getting closer

Proof of concept for xloader site url

43203b8

wardi reviewed Nov 19, 2024

View reviewed changes

duttonw reviewed Nov 21, 2024

View reviewed changes

duttonw requested changes Nov 21, 2024

View reviewed changes

wardi mentioned this pull request Jan 7, 2025

xloader still always pending #240

Open

Updates to config_declaration.yaml, jobs.py

da5c031

ThrawnCA requested changes Jan 15, 2025

View reviewed changes

ckanext/xloader/config_declaration.yaml Outdated Show resolved Hide resolved

ckanext/xloader/jobs.py Outdated Show resolved Hide resolved

kowh-ai added 2 commits January 15, 2025 11:07

Changes for config_declaration.yaml and jobs.py as per PR

93c9e5f

Update jobs.py

deab5cb

missed a callback_xloader_hook call, need to update result_url here too

ThrawnCA reviewed Jan 16, 2025

View reviewed changes

duttonw requested changes Jan 17, 2025

View reviewed changes

Updates to jobs.py and utils.py

729d5b3

hopefully getting closer

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] Proof of concept for xloader site url #234

[Draft] Proof of concept for xloader site url #234

pwalsh commented Nov 18, 2024

ThrawnCA commented Nov 18, 2024

wardi Nov 19, 2024

wardi Nov 19, 2024

wardi Nov 19, 2024

pwalsh Nov 19, 2024

wardi Nov 19, 2024

ThrawnCA Jan 15, 2025

wardi Nov 19, 2024

pwalsh Nov 19, 2024

duttonw Nov 21, 2024

duttonw Nov 21, 2024

duttonw left a comment

pwalsh commented Nov 21, 2024

JVickery-TBS commented Dec 9, 2024

ThrawnCA Jan 16, 2025

kowh-ai Jan 16, 2025

ThrawnCA Jan 16, 2025 •

edited

Loading

duttonw left a comment

duttonw Jan 16, 2025

duttonw Jan 17, 2025

kowh-ai Jan 18, 2025 •

edited

Loading

duttonw Jan 19, 2025

duttonw Jan 17, 2025

	This is useful, for example, when the site is running within a docker network.
	This is useful, for example, when the site is running within a docker network
	or where the job runner can't access ckan using its normal public site URL.

[Draft] Proof of concept for xloader site url #234

Are you sure you want to change the base?

[Draft] Proof of concept for xloader site url #234

Conversation

pwalsh commented Nov 18, 2024

ThrawnCA commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duttonw left a comment

Choose a reason for hiding this comment

pwalsh commented Nov 21, 2024

JVickery-TBS commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThrawnCA Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

duttonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kowh-ai Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThrawnCA Jan 16, 2025 •

edited

Loading

kowh-ai Jan 18, 2025 •

edited

Loading