Dataset author normalization#317
Conversation
The gwy-build-and-push job's 'if' condition omitted the branch guard for workflow_dispatch events. Due to &&-over-|| precedence, any workflow_dispatch from any branch could push dev-<sha> images to ghcr.io. Wrap both push and workflow_dispatch clauses with the master/main branch check.
|
Review changes with Changed Files
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| REGISTRY="ghcr.io/spectrumx/sds-gateway" | ||
| docker buildx imagetools create \ | ||
| --tag "${REGISTRY}:stable" \ | ||
| "${REGISTRY}:dev-${SHA}" |
There was a problem hiding this comment.
Promote tag SHA mismatch
Medium Severity
The promotion workflow uses the raw commit_sha input to find the dev- image tag. CI builds, however, tag images with a 7-character short SHA. This mismatch means if the input isn't the exact 7-character SHA, the image won't be found, and promotion will fail, even if the SHA is valid in Git.
Reviewed by Cursor Bugbot for commit 5dae463. Configure here.
Iterate queryset.values() instead of model instances so Dataset.from_db() does not json.loads(authors) before per-row error handling. Parse authors manually and use queryset.update() for writes, matching migration 0017. Co-authored-by: Lucas Parzianello <lucaspar@users.noreply.github.com>
Dataset.save() only normalized authors when the field was already a Python list. Legacy values passed as valid JSON strings (e.g. from versioning or direct ORM writes) were validated but left unchanged, allowing the old list-of-strings format to persist and trigger production warnings. Extract _normalize_authors_list() and apply it in both the list and JSON string branches. Add regression tests for JSON-string write paths. Co-authored-by: Lucas Parzianello <lucaspar@users.noreply.github.com>
When authors contained both legacy strings and new dict entries, save() only normalized when the first entry was a string and rebuilt the list from string entries only. Dict-shaped co-authors were silently dropped. Normalize any string entries while preserving existing dict entries, matching AuthorsManager.normalizeAuthorEntries behavior. Co-authored-by: Lucas Parzianello <lucaspar@users.noreply.github.com>
| {"name": author, "orcid_id": ""} | ||
| for author in authors | ||
| if isinstance(author, str) | ||
| ] |
There was a problem hiding this comment.
Migration drops dict co-authors
High Severity
The migrate_dataset_authors command can cause data loss. If a dataset's authors list contains a mix of string and dict entries, and the first entry is a string, the script's conversion logic only processes string elements. This drops any existing dict-formatted co-authors from the updated data, permanently removing their metadata, unlike Dataset.save() normalization.
Reviewed by Cursor Bugbot for commit 5abf551. Configure here.
- Refactor migrate_dataset_authors command to satisfy ruff complexity rules - Move Dataset.save before _normalize_authors_list per Django style guide - Parse JSON-string authors in get_authors_display for in-memory instances - Update migrate command tests to use raw DB authors and avoid factory bug - Shorten test docstrings to satisfy line-length checks
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2981058. Configure here.
| dataset_uuid: str, | ||
| ) -> list | None: | ||
| if isinstance(authors[0], dict): | ||
| return None |
There was a problem hiding this comment.
Migration skips dict-first mixed
Medium Severity
_convert_legacy_authors returns early when authors[0] is a dict, so rows that still contain legacy string authors after the first entry are never updated. Dataset.save() would normalize those strings via _normalize_authors_list, but the bulk migration leaves them in the database.
Reviewed by Cursor Bugbot for commit 2981058. Configure here.
3 participants
Note
Merge #315 first
I was still getting warnings in prod because of datasets in the old authors format (str); this PR changes the
Dataset.save()method to set the authors field value as a list of dicts, even if the dataset authors is in the old strings form.The migration command converts existing data into the new format.
Note
Medium Risk
Dataset author writes touch all save paths (behavior change for legacy strings); production deploys now depend on published images and the promote workflow instead of compose builds.
Overview
Dataset authors are normalized on every
Dataset.save()so legacy["Name", …]values become[{"name": "…", "orcid_id": ""}, …], including JSON-string inputs and paths that bypass forms (e.g. versioning). A newmigrate_dataset_authorsmanagement command backfills existing rows (with--dry-run/--uuid), andget_authors_display()is hardened for string JSON. Tests cover save normalization and the command.Gateway delivery adds a post-check job that builds and pushes
ghcr.io/spectrumx/sds-gateway(dev,dev-<short-sha>) on default-branch pushes, plus a manual workflow to retagdev-<sha>asstable. Production compose now pulls that image (SDS_GATEWAY_TAG, defaultstable) for app and Celery services instead of local builds.Workflows also bump common GitHub Actions (checkout, cache, uv pin, artifacts) and minor
package.jsonformatting.Reviewed by Cursor Bugbot for commit 2981058. Bugbot is set up for automated code reviews on this repo. Configure here.