Throw away invalid JSON in data migration #330
Conversation
WalkthroughAdds VSCode search exclusions to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Script as Migration Script
participant DB as Database
Script->>DB: Execute 0006_data_migration.sql
loop For each affected user row
DB->>DB: Trim metadata -> NULLIF(TRIM(metadata),'')
alt metadata is NULL or empty
DB-->>DB: Set metadata = NULL
else metadata is JSON-valid AND json_type = 'object'
DB-->>DB: Preserve trimmed metadata string
else invalid JSON or non-object
DB-->>DB: Set metadata = NULL
end
end
DB-->>Script: Migration complete
note over DB: Behavior changed from applying json(...) to preserving raw trimmed JSON text when valid.
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
db_migrations/0006_data_migration.sql (1)
9-12: Optional: trim and restrict to JSON objects/arrays to avoid scalar “valid JSON” slipping in.If downstream expects
metadatato be an object (or array), consider guarding type and trimming whitespace to tolerate leading/trailing spaces.Apply this diff to the CASE:
- CASE - WHEN metadata IS NOT NULL AND metadata != '' AND json_valid(metadata) THEN metadata - ELSE NULL - END as metadata + CASE + WHEN json_valid(NULLIF(TRIM(metadata), '')) + AND json_type(NULLIF(TRIM(metadata), '')) IN ('object','array') + THEN NULLIF(TRIM(metadata), '') + ELSE NULL + END AS metadataTo gauge impact before changing behavior, run this diagnostic query in your migration test DB:
SELECT COUNT(*) AS total_rows, SUM(metadata IS NOT NULL AND TRIM(metadata)!='') AS non_empty_rows, SUM(json_valid(metadata)) AS valid_json_rows, SUM(json_valid(metadata) AND json_type(metadata) IN ('object','array')) AS obj_or_arr_rows FROM contacts;.vscode/settings.json (1)
5-10: Nit: also leverage .gitignore in Search to reduce maintenance.Enabling
"search.useIgnoreFiles": truelets VS Code honor.gitignore/.ignore, so you don’t need to mirror patterns in settings.Apply this diff:
"rust-analyzer.cargo.features": "all", + "search.useIgnoreFiles": true, "search.exclude": { "**/Cargo.lock": true, "**/target": true, "**/.git": true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.vscode/settings.json(1 hunks)db_migrations/0006_data_migration.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-23T16:01:59.664Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#324
File: .vscode/settings.json:4-5
Timestamp: 2025-08-23T16:01:59.664Z
Learning: The rust-analyzer VS Code setting `"rust-analyzer.cargo.features"` accepts multiple valid values including "all" (enables all features), "none" (disables all features), "default" (uses default features), or an explicit array of feature names like ["feat1", "feat2"].
Applied to files:
.vscode/settings.json
📚 Learning: 2025-08-23T16:01:59.664Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#324
File: .vscode/settings.json:4-5
Timestamp: 2025-08-23T16:01:59.664Z
Learning: The rust-analyzer VS Code setting `"rust-analyzer.cargo.features": "all"` is valid and enables all available Cargo features for IDE analysis, not just an array of specific feature names.
Applied to files:
.vscode/settings.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (2)
db_migrations/0006_data_migration.sql (1)
9-12: Good hardening: only import valid JSON and preserve original text.Adding
json_valid(metadata)and inserting the original string avoids accidental canonicalization and prevents bad JSON from enteringusers.metadata. Looks correct..vscode/settings.json (1)
5-10: VS Code search exclusions look good; rust-analyzer features setting is valid.Change is syntactically correct JSON, and
"rust-analyzer.cargo.features": "all"is a valid value per our prior learning. Including**/targetand**/Cargo.lockinsearch.excludereduces noise.Note: I used the previously retrieved learning about rust-analyzer features from PR #324.
Summary by CodeRabbit
Bug Fixes
Chores