Skip to content

503 json tagging - #507

Open
kjlippold wants to merge 5 commits into
release/v2.0.0from
503-json-tagging
Open

503 json tagging#507
kjlippold wants to merge 5 commits into
release/v2.0.0from
503-json-tagging

Conversation

@kjlippold

Copy link
Copy Markdown
Contributor

Resolves #503

@kjlippold kjlippold self-assigned this Aug 11, 2026

@daniel-slaugh daniel-slaugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall approach is good. I ran this PR through Claude Code/Codex and they flagged some issues. Listed below are the ones I agree should get patched before merging this PR into release/v2.0.0:

[P1] Python datastream tag deletion is always broken. datastream.py:345 redefines and overrides the new delete_tag method. datastream.delete_tag("season") raises TypeError because the stale implementation still requires value and targets the removed /tags endpoint.

[P1] Empty dictionaries are converted to JSON null. base.py:110 recursively turns {} into None but retains the parent key. A normal ETL task update therefore sends "taskVariables": null from task.py:108, producing a 422 because the API requires an object.

[P2] Prototype-named tag keys break Browse and editing. BrowseFilterTool.vue:594 and tags.ts:16 use unchecked property access or in. Valid keys such as constructor can cause .localeCompare to receive a function, while toString tags cannot be deleted. Use Object.hasOwn()

[P2] Model-level tag shape is unconstrained. The new JSONField accepts arrays or non-string values through Django admin/direct ORM writes, while /tags/keys unconditionally calls .items(). Such records cause public list/tag endpoints to return 500s. Add model/database validation for an object with string values.

[P2] Published SDK examples still use removed APIs. The TypeScript guide still calls createTag/updateTag and passes an object to deleteTag; the Python guide still advertises removed add_tag/update_tag methods.

[P2] Both agents thought the new partial GIN index likely won't ever get used: PostgreSQL must prove that the query implies the partial predicate. It is unlikely to infer that tags @> $1 implies tags <> '{}', particularly with a parameterized right-hand side. PostgreSQL documents that partial-index predicate recognition is deliberately limited.

Empty keys and values are now accepted. I can't think of a case where a user would want to save a key or value of "". Do we want to require non-empty validation here?

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