Skip to content

Update command.py fix#267

Open
piersoft wants to merge 1 commit intockan:masterfrom
piersoft:patch-1
Open

Update command.py fix#267
piersoft wants to merge 1 commit intockan:masterfrom
piersoft:patch-1

Conversation

@piersoft
Copy link
Copy Markdown

fix for eval too long with api (insecure) when ckan xloader status

fix for eval too long with api (insecure) when ckan xloader status
job_params = eval(job.description.replace(
'ckanext.xloader.jobs.xloader_data_into_datastore', ''))
job_metadata = job_params['metadata']
# FIX DEFINITIVO:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please use English.

'ckanext.xloader.jobs.xloader_data_into_datastore', ''))
job_metadata = job_params['metadata']
# FIX DEFINITIVO:
# Non usare job.description (non è un canale dati stabile e può essere troncato).
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How does this affect backward compatibility?

metadata = {}

try:
if getattr(job, 'args', None) and len(job.args) >= 1:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Doesn't the truthiness check already ensure it's not empty?

if isinstance(payload, dict):
metadata = payload.get('metadata') or {}
elif getattr(job, 'kwargs', None) and isinstance(job.kwargs, dict):
payload = job.kwargs.get('data') or job.kwargs
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why would it be "either the 'data' field or the whole thing"?

payload = job.kwargs.get('data') or job.kwargs
if isinstance(payload, dict):
metadata = payload.get('metadata') or {}
except Exception:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we use a narrower exception type? Surely retrieving keys has a limited range of types it can raise.

except Exception:
metadata = {}

res_id = metadata.get('resource_id', 'N/A')
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm dubious about using "N/A" as the default value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants