DMS-1218: Fixed JSON quoting in localroles configuration#137
Open
chris-adam wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughAdds a 3.1.6 migration that normalizes malformed JSON quoting in Changes3.1.6 Migration: localroles JSON quoting fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@imio/dms/mail/migrations/migrate_to_3_1.py`:
- Around line 488-498: The localroles normalization in migrate_to_3_1 should not
silently continue after a bad rel value; instead, collect the failures in the
normalization loop around the ast.literal_eval/json.dumps handling and stop the
migration if any are found. Update the logic near the logger.error path so the
failure details are retained, then raise after processing (or otherwise prevent
finalization/version bump) so the upgrade cannot be marked successful when some
FTIs still have unnormalized rel data.
In `@imio/dms/mail/migrations/upgrades.zcml`:
- Around line 112-126: Keep the upgrade chain reachable from 3.1.4 in
upgradestep registration. The commented-out genericsetup:upgradeStep for
migrate_to_3_1_5 removes the only path from source="3.1.4", so either restore
that step or make the migrate_to_3_1_6 step start from the last shipped version
while keeping handler imio.dms.mail.migrations.migrate_to_3_1_5.migrate and
imio.dms.mail.migrations.migrate_to_3_1_6.migrate aligned with a valid
source-to-destination progression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c164adcb-b81a-4180-ac34-b345de73e4de
📒 Files selected for processing (5)
CHANGES.rstimio/dms/mail/migrations/migrate_to_3_1.pyimio/dms/mail/migrations/migrate_to_3_1_6.pyimio/dms/mail/migrations/upgrades.zcmlimio/dms/mail/profiles/default/metadata.xml
Comment on lines
+488
to
+498
| try: | ||
| cfg["rel"] = json.dumps(ast.literal_eval(rel)) | ||
| except (ValueError, SyntaxError): | ||
| logger.error("Cannot normalize rel %r on %s/%s/%s/%s", | ||
| rel, fti.getId(), keyname, state, principal) | ||
| continue | ||
| changed = True | ||
| if changed: | ||
| localroles._p_changed = True | ||
| fixed.append(fti.getId()) | ||
| logger.info("Normalized localroles 'rel' on portal types: %s", fixed) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Abort the upgrade when a rel value cannot be normalized.
This path only logs the bad value and keeps going, so the upgrade can still finish and mark the profile as upgraded while some FTIs remain on the broken config that caused the transition failures in the first place. Please collect these failures and raise at the end (or otherwise stop finalization/version bump) so the migration cannot succeed partially.
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 488-488: use jsonify instead of json.dumps for JSON output
Context: json.dumps(ast.literal_eval(rel))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@imio/dms/mail/migrations/migrate_to_3_1.py` around lines 488 - 498, The
localroles normalization in migrate_to_3_1 should not silently continue after a
bad rel value; instead, collect the failures in the normalization loop around
the ast.literal_eval/json.dumps handling and stop the migration if any are
found. Update the logic near the logger.error path so the failure details are
retained, then raise after processing (or otherwise prevent finalization/version
bump) so the upgrade cannot be marked successful when some FTIs still have
unnormalized rel data.
chris-adam
force-pushed
the
DMS-1218/fix-localroles-json-quoting
branch
from
b92d813 to
fc224fd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
imio/dms/mail/migrations/upgrades.zcml (1)
112-125: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore a reachable upgrade chain to
3.1.6a.Line 115 comments out the only
source="3.1.4"step, and Line 123 registers the new migration fromsource="3.1.5"instead of3.1.5a. That leaves existing portals on3.1.4with no forward step, and portals already upgraded to3.1.5awill never match the new handler. Keep the3.1.4 -> 3.1.5astep active and register this step from3.1.5a, which also matches the PR’s stated migration contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@imio/dms/mail/migrations/upgrades.zcml` around lines 112 - 125, Restore the upgrade chain in upgrades.zcml so portals can reach 3.1.6a from both 3.1.4 and 3.1.5a. Re-enable the existing genericsetup:upgradeStep for migrate_to_3_1_5.migrate by removing the accidental comment marker, and change the new genericsetup:upgradeStep for imio.dms.mail.migrations.migrate_to_3_1_6.migrate to use source="3.1.5a" instead of source="3.1.5". Keep the handler names and destination version unchanged so the migration path stays reachable and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@imio/dms/mail/migrations/upgrades.zcml`:
- Around line 112-125: Restore the upgrade chain in upgrades.zcml so portals can
reach 3.1.6a from both 3.1.4 and 3.1.5a. Re-enable the existing
genericsetup:upgradeStep for migrate_to_3_1_5.migrate by removing the accidental
comment marker, and change the new genericsetup:upgradeStep for
imio.dms.mail.migrations.migrate_to_3_1_6.migrate to use source="3.1.5a" instead
of source="3.1.5". Keep the handler names and destination version unchanged so
the migration path stays reachable and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5e67e16-4bf9-4c3f-8f83-f57d10e30eb2
📒 Files selected for processing (5)
CHANGES.rstimio/dms/mail/migrations/migrate_to_3_1.pyimio/dms/mail/migrations/migrate_to_3_1_6.pyimio/dms/mail/migrations/upgrades.zcmlimio/dms/mail/profiles/default/metadata.xml
✅ Files skipped from review due to trivial changes (1)
- CHANGES.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- imio/dms/mail/profiles/default/metadata.xml
- imio/dms/mail/migrations/migrate_to_3_1_6.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes task transition failures reported in SUP-54082:
dexterity.localroles2.0.0b0 switched the local-rolesrelconfig parsing to strictjson.loadswithout migrating legacy single-quoted data. Adds a 3.1.6 migration (Migrate_To_3_1.fix_localroles_json_quoting, called from the newMigrate_To_3_1_6) that normalizes every FTI'srelconfig from Python-repr to JSON (idempotent), registers the 3.1.5a → 3.1.6a upgrade step, and bumps the profile version to 3.1.6.Ticket
DMS-1218 — https://support-imio.atlassian.net/browse/DMS-1218
Pre-PR checks
tests/test_migrations.pyleft untracked at author's request; passes locally 3/3)metadata.xml3.1.4 → 3.1.6)Commits
b92d813 DMS-1218: Fixed JSON quoting in localroles configuration
Summary by CodeRabbit