503 json tagging - #507
Conversation
daniel-slaugh
left a comment
There was a problem hiding this comment.
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?
Resolves #503