Skip to content

Fixed #36947 -- Avoided altering column type when only db_comment changes. - #20817

Merged
sarahboyce merged 2 commits into
django:mainfrom
cliffordgama:ticket-36947-db_comment-migration
Sep 16, 2026
Merged

Fixed #36947 -- Avoided altering column type when only db_comment changes.#20817
sarahboyce merged 2 commits into
django:mainfrom
cliffordgama:ticket-36947-db_comment-migration

Conversation

@cliffordgama

@cliffordgama cliffordgama commented Mar 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Trac ticket number

ticket-36947

Backends that allow altering column comments without altering the column type should set supports_independent_comment_alteration=True.

Thanks @a-p-f for the report.

AI Assistance Disclosure (REQUIRED)

  • No AI tools were used in preparing this PR.
  • If AI tools were used, I have disclosed which ones, and fully reviewed and verified their output.

Checklist

  • This PR follows the contribution guidelines.
  • This PR does not disclose a security vulnerability (see vulnerability reporting).
  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

Credok12
Credok12 reviewed Mar 3, 2026
View reviewed changes

@Credok12 Credok12 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@tim-mccurrach tim-mccurrach mentioned this pull request May 20, 2026
Closed
11 tasks
@cliffordgama
cliffordgama force-pushed the ticket-36947-db_comment-migration branch from c5e6fab to 3ff3091 Compare July 18, 2026 22:09
blighj
blighj reviewed Jul 24, 2026
View reviewed changes

@blighj blighj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played around with this PR, code looks good, I confirmed it worked on postgres locally. I haven't tested other dbs, one feels enough to give me confidence it works.

Should there be a regression test with a GeneratedField per the ticket bug?

@cliffordgama
cliffordgama force-pushed the ticket-36947-db_comment-migration branch from 3ff3091 to dcf464b Compare July 31, 2026 17:41
@cliffordgama

cliffordgama commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

Should there be a regression test with a GeneratedField per the ticket bug?

Hi @blighj! Thanks for the review. I've added a test

@cliffordgama
cliffordgama force-pushed the ticket-36947-db_comment-migration branch 2 times, most recently from f7cd4fc to c079371 Compare July 31, 2026 17:52
blighj
blighj approved these changes Aug 1, 2026
View reviewed changes

@blighj blighj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now.

adamchainz
adamchainz reviewed Aug 19, 2026
View reviewed changes

@adamchainz adamchainz left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @cliffordgama !

I just hit the same bug myself, on PostgreSQL. I added comments to lots of fields, and where tables had related views, the ALTER COLUMN ... TYPE clauses caused errors that needed the views to be rebuilt. I am working around it with custom migrations, but this fix would be much better.

The patch looks generally good to me. I had tested it myself too.

Comment thread tests/schema/tests.py
if connection.features.supports_independent_comment_alteration:
self.assertIn("COMMENT ON COLUMN", ctx.captured_queries[0]["sql"])
else:
self.assertIn("ALTER TABLE", ctx.captured_queries[0]["sql"])

@adamchainz adamchainz Aug 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assert on the second query too?

Suggested change
self.assertIn("ALTER TABLE", ctx.captured_queries[0]["sql"])
self.assertIn("ALTER TABLE", ctx.captured_queries[0]["sql"])
self.assertIn("COMMENT ON COLUMN", ctx.captured_queries[1]["sql"])

@cliffordgama cliffordgama Aug 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MySQL backend shows a failure for this one because it doesn't provide the same SQL for comments:

django/django/db/backends/mysql/schema.py

Lines 228 to 230 in cccc004

def _alter_column_comment_sql(self, model, new_field, new_type, new_db_comment):
# Comment is alter when altering the column type.
return "", []

Instead it produces a single ALTER FIELD. So we got an IndexError after applying the suggestion.

Comment thread tests/schema/tests.py
):
editor.alter_field(GenFieldModelComment, old_field, new_field, strict=True)

self.assertEqual(len(ctx), 1)

@adamchainz adamchainz Aug 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not assert on SQL here too?

@cliffordgama
cliffordgama force-pushed the ticket-36947-db_comment-migration branch from c079371 to 010c41c Compare August 19, 2026 22:47
@cliffordgama

cliffordgama commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

Hey @adamchainz! I'm happy you ran into this bug so I could get an extra set of eyes on this patch 😅 Thanks for the review!

@jacobtylerwalls

jacobtylerwalls commented Aug 22, 2026

Copy link
Copy Markdown
Member

buildbot, test on oracle.

cliffordgama and others added 2 commits September 16, 2026 10:48
@cliffordgama @sarahboyce
Fixed typo in django/db/backends/mysql/schema.py.
162955f
@cliffordgama @adamchainz @sarahboyce
Fixed #36947 -- Avoided altering column type when only db_comment cha…
404428e 
…nges.

Backends that allow altering column comments without altering the column
type should set supports_independent_comment_alteration=True.

Thanks Alex Fischer for the report.

Co-authored-by: Adam Johnson <[email protected]>
@sarahboyce
sarahboyce force-pushed the ticket-36947-db_comment-migration branch from 010c41c to 404428e Compare September 16, 2026 08:49
sarahboyce
sarahboyce approved these changes Sep 16, 2026
View reviewed changes

@sarahboyce sarahboyce left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks good to me ⭐

@sarahboyce
sarahboyce merged commit 64edef3 into django:main Sep 16, 2026
20 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sarahboyce sarahboyce sarahboyce approved these changes
+3 more reviewers
@adamchainz adamchainz adamchainz left review comments
@Credok12 Credok12 Credok12 left review comments
@blighj blighj blighj approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@cliffordgama @jacobtylerwalls @blighj @adamchainz @sarahboyce @Credok12