Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ufedaseyeuconsultant! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
tbain
left a comment
tbain
left a comment
There was a problem hiding this comment.
Possible Bug: actions.py:75; The AI review of this bit of code is concerned that the logic is not properly handling the case of a pre-existing tag with an external ID colliding with an old tag that might have the same ID during import. Do we need to do some de-conflicting logic here, like we do for the backfill() function in the migration, or do you feel like the use case for this logic properly handles that issue?
Possible Bug: 0022_tag_external_id_not_null.py:42; The AI review points out that this logic works for null, but does not take into account empty strings or other 'nullish' values. I think this is an interesting thing to point out; the AC states "tags without an external identifier receive one" but it doesn't specify null only. We might need to ask if we need to handle 'nullish' values instead of just null values for this backfill logic?
Bug: 0022_tag_external_id_not_null.py:50-60; The AI review points out that while the batching logic handles concerns with SQL size concerns, it does not handle in-memory batching so the python memory usage can become bloated. I think this is kind of a minimal concern considering the data we're dealing with and realistic sizes of a given real-world taxonomy, but I think it's valid. A simple way to (perhaps) handle it according to the AI is to clear the batch after it's processed a batch, e.g. something like:
batch.append(tag)
if len(batch) >= 500:
Tag.objects.bulk_update(batch, ["external_id"], batch_size=500)
batch.clear()
if batch:
Tag.objects.bulk_update(batch, ["external_id"], batch_size=500)
Bonus points if you use a new BATCH_SIZE constant :D
Suggestion: test_models.py:1271-1287; Can we make this test more consistent with similar tests, and also clearer to future developers by changing the numbers to be based on the TAG_EXTERNAL_ID_MAX_LENGTH constant? e.g.:
value_one = "X" * (TAG_EXTERNAL_ID_MAX_LENGTH + 45)
value_two = ("X" * TAG_EXTERNAL_ID_MAX_LENGTH) + ("Y" * 50)
first = Tag.objects.create(taxonomy=self.taxonomy, value=value_one)
second = Tag.objects.create(taxonomy=self.taxonomy, value=value_two)
assert first.external_id == "X" * TAG_EXTERNAL_ID_MAX_LENGTH
AI (Codex) Raw output:
Details
Verdict: I found two correctness issues and one scalability issue; I would not merge this PR as-is. - Bug. [actions.py:75](https://github.com/openedx/openedx-core/blob/d4064f02ac4bf9b9bfb912e2f225b65e96e1811e/src/openedx_tagging/import_export/actions.py#L75) treats a pre-upgrade numeric database ID as a valid external ID. If it matches another tag’s external ID, importing an old export can rename or re-parent the wrong tag, violating the issue’s “fails safely with no side effects” criterion. The tests only cover the non-collision case. - Bug. [0022_tag_external_id_not_null.py:42](https://github.com/openedx/openedx-core/blob/d4064f02ac4bf9b9bfb912e2f225b65e96e1811e/src/openedx_tagging/migrations/0022_tag_external_id_not_null.py#L42) backfills only NULL values. The field remains blank=True, and empty-string identifiers are therefore not repaired; direct model writes can also generate an empty identifier from an empty tag value. - Bug. [0022_tag_external_id_not_null.py:50-60](https://github.com/openedx/openedx-core/blob/d4064f02ac4bf9b9bfb912e2f225b65e96e1811e/src/openedx_tagging/migrations/0022_tag_external_id_not_null.py#L50) accumulates every missing tag in one taxonomy before calling bulk_update. batch_size=500 limits SQL statement size, not Python memory usage, so a large taxonomy can consume excessive memory during the upgrade despite the stated scale requirement. Suggestion: the PR’s “Manual testing via UI for all of ticket AC” entry is not reproducible manual-testing documentation; it should list concrete steps and expected results.
@tbain According to AC and code this one is already covered as needed Additional Claude reviewInvestigated this instead of adding a de-conflicting check in actions.py. Short version: the concrete failure mode the review describes (a wrong tag getting renamed or re-parented) doesn't reproduce for the scenario the ticket's AC actually requires, because _get_tag() doesn't decide anything in isolation — every row still runs through the full action-generation pipeline, and an existing check in that pipeline already closes the gap.The scenario. A pre-#758 export references a tag by its old database id, because that tag had no external_id at export time. If that old id happens to equal a different tag's real, current external_id, _get_tag() resolves the row to that other tag instead of failing to find one. Why it still fails safely. generate_actions() checks every action class against a row, not just the first one that matches. For a row like this, RenameTag.applies_for() also evaluates, and it's true here: the wrongly-matched tag's value can't equal the row's value, because Tag.value is unique per taxonomy and the tag the row is actually about still holds that value (the re-import is unchanged, so nothing renamed it away). That mismatch means a RenameTag action gets built for the wrongly-matched tag too, and its own validation (_validate_value) finds the value already taken and raises an error. TagImportPlan.execute() is all-or-nothing (if self.errors: return), so that one error blocks the entire plan, including whatever UpdateParentTag action might otherwise have re-parented the wrong tag. Verification. Confirmed this empirically before trusting the trace: wrote a probe reproducing the exact collision (a tag whose real external_id equals another tag's old PK, re-imported unchanged) in both a flat and a parent/child taxonomy shape. In both cases the plan reported "Duplicated tag value" and neither tag's value nor parent changed. Kept the flat-taxonomy case as a permanent regression test: test_reimport_pre_upgrade_export_file_fails_safely_on_id_collision in tests/openedx_tagging/import_export/test_api.py. What's genuinely still open. The protection depends on the row's value still matching an existing tag. If a re-imported file also carries a value nothing else in the taxonomy currently has — meaning it's not just "re-import unchanged" but a deliberate rename layered on top of a stale, colliding id — that combination isn't guarded against. That's a narrower scenario than "re-import an old export unchanged," which is what the ticket's acceptance criteria describes, so I've documented it as a known limitation in ADR 0012 rather than adding matching logic for it. Closing it fully would mean weighing it against rejecting legitimate imports that use a plain numeric external_id (a common style for CBE competency identifiers) that happens to equal an unrelated tag's database id, since the import format has no way to tell those two cases apart. Conclusion: no change to actions.py. Added the regression test above and the "Known limitations" section in docs/openedx_tagging/decisions/0012-non-nullable-tag-external-id.rst to make this reasoning and its boundary explicit for future readers. |
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
Minor nit. Please squash, adjust the commit message as needed, and I'll merge. Thank you.
| if attempt == 1: | ||
| return value.strip()[:TAG_EXTERNAL_ID_MAX_LENGTH].strip() | ||
| suffix = f"-{attempt}" | ||
| return value.strip()[: TAG_EXTERNAL_ID_MAX_LENGTH - len(suffix)].strip() + suffix |
There was a problem hiding this comment.
Please explain in plain English or by example what the sequence of candidate external IDs looks like.
There was a problem hiding this comment.
Done. Squashed commits and added comment
Every tag now always carries an external identifier (the Competency ID shown on the Competency Management page), whether an institution assigns one or the system generates it from the tag's name. Previously a tag only got an external_id via file import; one created through the taxonomy editor or the tagging API had none at all. Tag.save() generates a candidate from the tag's value when external_id is empty, resolving collisions with a casefold-based per-taxonomy uniqueness check and a numeric suffix. Taxonomy.add_tag() rejects a caller-supplied duplicate (exact or case-different) with ValueError instead of letting it reach the database as an IntegrityError. A new migration backfills every existing null or empty-string external_id, in capped in-memory and SQL batches, without overwriting institution-assigned ones, then makes the column NOT NULL; rollback only loosens that constraint and leaves generated data in place. A stale pre-upgrade export whose id collides with another tag's real external_id still fails safely on re-import, tripping a co-triggered rename action's existing duplicate-value check; this behavior and its narrower residual limitation are documented in ADR 0012. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
c3cd75c to
e95a9bf
Compare
5 participants
Summary
Implements ADR docs/openedx_tagging/decisions/0012-non-nullable-tag-external-id.rst (left in Proposed status; accepting it is a separate step this PR doesn't take).
Closes #758.
Test plan
🤖 Generated with Claude Code