Support long reference URLs - #203
Conversation
Reference extraction runs after an edit commits, so a URL longer than the MySQL varchar limit could turn a successful edit into a 500 response. Store full URLs as text and retain database uniqueness through a SHA-256 digest. Amp-Thread-ID: https://ampcode.com/threads/T-01a0436f-c1c9-7775-a5ed-72abb9b7b20a Co-authored-by: Amp <amp@ampcode.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce624e10be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| add_index :coplan_references, [ :plan_id, :url_digest ], unique: true, | ||
| name: "index_coplan_references_on_plan_id_and_url_digest" |
There was a problem hiding this comment.
During a rolling deploy, an old application process can continue inserting references without url_digest after this migration completes. Because SQL unique indexes permit multiple NULL values, two concurrent old-process requests for the same plan and URL can both pass the model's race-prone uniqueness check and insert duplicates; the previous database index prevented this. The new callback only repairs such rows if they are later validated by new code, so the digest needs to be populated at the database level or enforced in a later migration after old processes have been retired.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖
|
|
||
| def down | ||
| remove_index :coplan_references, name: "index_coplan_references_on_plan_id_and_url_digest" | ||
| change_column :coplan_references, :url, :string, null: false |
There was a problem hiding this comment.
Once the feature stores any URL longer than 255 characters, this rollback narrows the column back to MySQL's default VARCHAR(255). In strict mode the rollback fails with a data-too-long error, while permissive configurations may truncate URLs before recreating the unique index, so the migration is not reversible after its intended use. The down path should explicitly handle the oversized rows or declare the migration irreversible instead of attempting a lossy narrowing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖
Generated SHA-256 digests preserve the uniqueness constraint for old and new application processes throughout a rolling deploy. Treat rollback as irreversible rather than risking truncation of stored long URLs. Amp-Thread-ID: https://ampcode.com/threads/T-01a0436f-c1c9-7775-a5ed-72abb9b7b20a Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d375fc071
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| end | ||
|
|
||
| def remove_url_index_and_expand_column | ||
| remove_index :coplan_references, column: [ :plan_id, :url ] |
There was a problem hiding this comment.
On MySQL, each of these DDL statements commits independently, so dropping the original unique index here leaves writes unprotected throughout the subsequent column alteration and digest-index build. If concurrent writers insert the same plan/URL during that window, both model validations can pass and the final add_index ... unique: true can fail, leaving this nontransactional migration partially applied. The generated column addresses the previously reported NULL-digest problem, but the updated ordering provides fresh evidence of this separate gap; create the digest unique index immediately after adding the generated column, before removing the old index.
AGENTS.md reference: AGENTS.md:L30-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖
The generated-digest unique index is now created before the old URL index is removed on MySQL, so every step of its nontransactional DDL sequence retains database-level uniqueness.
Build the generated-digest unique index before removing the old URL index on MySQL so concurrent writes remain protected throughout its nontransactional DDL sequence. Amp-Thread-ID: https://ampcode.com/threads/T-01a0436f-c1c9-7775-a5ed-72abb9b7b20a Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 117e854f93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| when "Mysql2" | ||
| "SHA2(url, 256)" |
There was a problem hiding this comment.
On MySQL, url and its former unique index use the table's case-insensitive utf8mb4_0900_ai_ci collation, while SHA2 hashes the original bytes. Consequently, two concurrent creates for the same plan using URLs that differ only by host or path casing can both pass the model's collation-aware uniqueness query and then receive different digests, allowing rows that both the validator and previous database index considered duplicates. Normalize the digest consistently with the intended URL collation, or make URL comparisons explicitly binary so validation and database enforcement agree.
AGENTS.md reference: AGENTS.md:L30-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖
MySQL now stores reference URLs with a binary collation, matching PostgreSQL’s case-sensitive equality and the byte-sensitive SHA-256 digest while preserving valid case-sensitive URL paths. Fresh migration and schema-load tests cover the contract on both adapters.
Use a binary collation for MySQL reference URLs so application equality, database uniqueness, and byte-based SHA-256 digests agree while retaining case-sensitive URL paths. Amp-Thread-ID: https://ampcode.com/threads/T-01a0436f-c1c9-7775-a5ed-72abb9b7b20a Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Why
A Markdown URL longer than 255 characters can overflow
coplan_references.urlduring post-commit reference extraction, returning a 500 after an otherwise successful plan edit.What
Risk Assessment
Medium — this changes the live references table and uniqueness index. The database-generated digest preserves uniqueness throughout rollout; the migration is explicitly irreversible because narrowing back to 255 characters could truncate newly stored URLs.
Testing
Generated with Amp