Compare nullability as well as type when syncing columns - #19
Conversation
needsColumnModification compared only the normalized type, so a column whose nullability changed in the declaration was never modified in any existing database. That let journal_entries.fleetId stay NOT NULL after its declaration went nullable, which broke journal creation in production (siren-team-os 1.11.0). IS_NULLABLE was already fetched; now a mismatch in either direction emits the MODIFY. Declared nullability reads by containment, not equality, because attributes are free strings and NOT NULL often travels with company, as in "NOT NULL DEFAULT 'human'" — equality matching read that as nullable and would have dropped the constraint. Includes the undoing test: nullability-only drift must produce a modification, which fails if the comparison is removed while the fetch remains.
![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: f30d1ed719
ℹ️ 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".
| $currentNullable = strtoupper(trim((string) $currentColumnData['IS_NULLABLE'])) === 'YES'; | ||
| // Attributes are free strings and NOT NULL often travels with company | ||
| // ("NOT NULL DEFAULT 'human'"), so containment, not equality, decides. | ||
| $newNullable = true; |
There was a problem hiding this comment.
When a schema represents non-nullability implicitly, such as new Column(..., 'PRIMARY KEY') without a separate NOT NULL, MySQL reports the primary-key column as IS_NULLABLE=NO, but this initializes the declaration as nullable and never considers PRIMARY KEY. Every synchronization therefore emits a MODIFY; because the existing key continues to force IS_NULLABLE=NO, subsequent runs repeat the table alteration. The replacement of PrimaryKeyFactory with a manually constructed column containing NOT NULL in the no-op test masks this regression.
Useful? React with 👍 / 👎.
| // ("NOT NULL DEFAULT 'human'"), so containment, not equality, decides. | ||
| $newNullable = true; | ||
| foreach ($newColumn->getAttributes() as $attribute) { | ||
| if (str_contains(strtoupper((string) $attribute), 'NOT NULL')) { |
There was a problem hiding this comment.
When a nullable column has an attribute such as DEFAULT 'NOT NULL' or COMMENT 'NOT NULL', raw substring containment mistakes the literal text for a constraint. Against a currently non-nullable column this suppresses the required relaxation, while against a nullable column it triggers the same ineffective MODIFY on every run. The check needs to distinguish the NOT NULL clause from occurrences inside quoted attribute values.
Useful? React with 👍 / 👎.
1 participant
The schema sync compared only column types, so nullability drift never reconciled — the root cause of the siren-team-os 1.11.0 production journal-write outage (Navigator charter 1500, journal 8826). IS_NULLABLE was already fetched; a mismatch now emits MODIFY. Declared nullability reads by containment since attributes are free strings ('NOT NULL DEFAULT ...') — equality matching would have dropped such constraints. Adds four tests including the undoing test and the compound-attribute regression case. Suite: 41 tests, exit code 0.