Fixed #18468 Support migrate with db comment

[KimSoungRyoul]

more detail description: https://code.djangoproject.com/ticket/18468

Django

• Field(db_comment="...")
• db_table_comment="...."

class AModel(Model):
  
  a_char = models.Field(db_comment="I Am comment to Column", max_length=32)
  
  class Meta:
      db_table = "a_model"
      db_table_comment = "table comment...."

Converted to postgres dialect

create table a_model
(
    id      bigserial   not null  constraint hello1_amodel_pkey  primary key,
    a_char  varchar(32) not null
    
);

comment on table a_model is 'table comment....';

comment on column a_model.a_char is 'I Am comment to Column';

Converted to mysql dialect

create table a_model33
(
    id     bigint auto_increment primary key,
    a_char varchar(32) not null comment 'I AM Comment for a_char22'
 
)
  comment 'I AM Table Comment22';

sqlite3

• not supported

• warning

⚡ python manage.py makemigrations     
System check identified some issues:

WARNINGS:
hello1.AModel.a_char: (field.W163) sqlite does not support a db_comment on columns
hello1.AModel.a_int: (field.W163) sqlite does not support a db_comment on columns
hello1.AModel: (models.W045) sqlite does not support db_table_comment.
Operations to perform:
  Apply all migrations: admin, auth, contenttypes, hello1, sessions
Running migrations:
  Applying admin.0004_alter_logentry_content_type... OK
  Applying auth.0013_alter_permission_content_type... OK
  Applying hello1.0003_alter_amodel_a_array_alter_amodel_a_char_and_more... OK
  ...

[KimSoungRyoul]

thank you for your feedback 😄

I will add testcase for this feature and reflect your feedback until this weekend

[KimSoungRyoul]

Hi @jchubber It tries to be helpful in making the test case. In this case, the new SQL grammar is reflected in django, so it can not be used with assertTableExists() or assertColumnExists() and the other test utils.
I think make assertion Util like a assertDBComment() In order to make test cases, it is necessary to add information about db_comment to DatabaseWrapper.introspection.get_table_description().
However, I am not sure whether it is the right way to change get_table_description() method to only use in test case.

[jchubber]

@KimSoungRyoul I do not have enough familiarity with migration testing either. I'll write a request into the Django Core Mentorship or Django developers groups to request advice. I'll let you know if/when I hear back.

Quick question: Other than the testing, is there anything else you know needs to be done before this ticket is done? It looks to me like you already reflected every piece of feedback I gave ✅ :)

[KimSoungRyoul]

thank you for your feedback , yes I already reflected every piece of feedback. I'll think about testcase for this new feature without modifying DatabaseWrapper.introspection.get_table_description()

and I will do rollback 1 commit and force push which contained failed useless testcase

~could give me link where the discussion is being requested? if not exist yet~ Django developers Groups: How can I make testcase for new migration feature (DB Comment) #18468

[jchubber]

@KimSoungRyoul You posted to django-developers and I posted to Django Core Mentorship (link to thread). It looks like the feedback from both groups is consistent: extending get_table_description() is ok and it's also recommended to extend inspectdb as well. It's great to have confirmation from both discussion boards about that ✅ ✅ 👍 If you need any help or want to discuss further, please don't hesitate to let me know.

[KimSoungRyoul]

I haven't done much make testcase yet. but soon will be done p.s I would be grateful if you could advise on a test case scenario.

...

by the way Can you "Resolve Conversation" that has already been reflected?

[KimSoungRyoul]

@knyghty thank you for your feedback. It seems more appropriate to remove .. warning::. and supports_db_comments -> supports_comments

@jchubber I add more testcase to cover up new feature(db comment)

• add_db_comment
• alter_db_comment
• delete_db_comment
• add_db_comment_with_default
• alter_db_comment_with_default

---

Is there anything else to think about besides the feedback you suggested?

[knyghty]

What happens if you add db_comment to a field that isn't directly represented as a column, e.g. ManyToManyField? Do we need a system check or something for this? You may need to add something here: https://github.com/django/django/blob/main/django/db/models/fields/related.py#L1230-L1261 ideally with a test.

I'm not sure if any other fields will have this issue.

[KimSoungRyoul]

I didn't consider about that. Just like you said, we need a system check
like a ...

if self.db_comment:
            warnings.append(
                checks.Warning(
                    'db_comment has no effect on ManyToManyField.',
                    obj=self,
                    id='fields.W3xx',
                )
            )

I will add this and consider what happens when ManyToManyField or ~GenericForeignKey~ etc... and db_comment are used together. 😄

Thanks

[jchubber]

(👍 awesome work @KimSoungRyoul and great points @knyghty !)

ManyToManyFields: This was a good catch by @knyghty. Django creates an intermediary join table in the db for every ManyToMany field. In theory, we could support db_comments on ManyToManyFields by putting those comments as table-level comments onto the intermediary join table that Django created. HOWEVER, we'd encounter a problem when the user specified a ManyToManyField with a through model, if they tried to put a db_comment on both the Field and also on the through model definition. I'm not 100% sure how to handle this. Either (1) Django supports db_comments on ManyToManyFields and adds them to the intermediary join table as a table-level comment when there is NO through table, but ignores the field-level comment when there IS a through table, or (2) Django never supports db_comments on ManyToManyFields at all (basically "If you want a db comment on your M2M fields, you HAVE to use a through table and put the db_comment there"). @KimSoungRyoul What do you think?

[jchubber]

Test Cases: The test cases @KimSoungRyoul has now look good. I am trying to think of strange edge cases situations where covering with unit tests could be helpful. I'm not coming up with many.

• ManyToManyField comments: As mentioned above, this special case might require separate unit tests.
• Table-level comments on through models: Is there any difference for through models? I suppose not... Maybe unit tests on the through models is not needed?
• What's the expected result if there is a comment in the db on a column and the Django user does NOT specify any comment on the field? Django should make no change to the pre-existing comment, right?

[jchubber]

I did an additional review of the Django model docs and found three other situations where db_comment support might get complicated:

1) Unmanaged models:

(link to docs). For any model that has...

class Meta:
        managed = False

...Django will not perform create, modify, or delete operations on the model. That means that db_comments cannot be supported for unmanaged models. I recommend that the docs be updated to state that db_comments are ignored when managed = False.

2) Proxy models:

(link to docs). A proxy model doesn't exist in the db in the same way as a regular model. So if a model has...

class Meta:
        proxy = True

...then I think perhaps db_comments should not be supported at the table or column-levels. That should be documented in the docs, should trigger a warning (during migration?), and should have a unit test to ensure that if a Django user specifies a db_comment on a proxy model that Django doesn't error.

3) Abstract Base Classes:

(link to docs) Django doesn't create tables for Abstract Base Classes, so a table-level db_comment on an Abstract Base Class does NOT make sense. But a field-level (or "column-level") db_comment on an Abstract Base Class' fields DOES make sense because those fields will eventually be created for any of the models that inherit from the Abstract Base Class.

So for example, with...

from django.db import models

class CommonInfo(models.Model):
    name = models.CharField(max_length=100)
    age = models.PositiveIntegerField(db_comment="User's age when they registered.")

    class Meta:
        abstract = True

class Student(CommonInfo):
    home_group = models.CharField(max_length=5)

...then a unit test should verify that in the db the student table has a column age with the comment "User's age when they registered.".

[jchubber]

@KimSoungRyoul I was busy the past 2 weeks, but I have more control over my time for the coming 2 weeks. Is there anything here that I can be helpful with? Writing docs? Working on code for some piece of the items mentioned above 👆?

[KimSoungRyoul]

(👍 awesome work @KimSoungRyoul and great points @knyghty !)

ManyToManyFields: This was a good catch by @knyghty. Django creates an intermediary join table in the db for every ManyToMany field. In theory, we could support db_comments on ManyToManyFields by putting those comments as table-level comments onto the intermediary join table that Django created. HOWEVER, we'd encounter a problem when the user specified a ManyToManyField with a through model, if they tried to put a db_comment on both the Field and also on the through model definition. I'm not 100% sure how to handle this. Either (1) Django supports db_comments on ManyToManyFields and adds them to the intermediary join table as a table-level comment when there is NO through table, but ignores the field-level comment when there IS a through table, or (2) Django never supports db_comments on ManyToManyFields at all (basically "If you want a db comment on your M2M fields, you HAVE to use a through table and put the db_comment there"). @KimSoungRyoul What do you think?

I think (2) Django never supports db_comments on ManyToManyFields at all is a good choice. (1) is a possibility that it will become too complicated and I agree with the problem you mentioned.

Logically, it is correct that ManyToManyField does not support db_comment .
ManyToManyField is not a column. It's a relationship mapping between models.

[KimSoungRyoul]

Why fail to connect only py3.9-mysql?

Is it just a temporary issue?

  File "/home/jenkins/workspace/pull-requests-bionic/database/mysql_gis/label/bionic-pr/python/python3.9/tests/.env/lib/python3.9/site-packages/MySQLdb/__init__.py", line 130, in Connect
    return Connection(*args, **kwargs)
  File "/home/jenkins/workspace/pull-requests-bionic/database/mysql_gis/label/bionic-pr/python/python3.9/tests/.env/lib/python3.9/site-packages/MySQLdb/connections.py", line 185, in __init__
    super().__init__(*args, **kwargs2)
django.db.utils.OperationalError: (2002, "Can't connect to local MySQL server through socket '/var/run/mysqld/mysqld.sock' (2)")
Build step 'Execute shell' marked build as failure

[KimSoungRyoul]

@KimSoungRyoul I was busy the past 2 weeks, but I have more control over my time for the coming 2 weeks. Is there anything here that I can be helpful with? Writing docs? Working on code for some piece of the items mentioned above 👆?

thanks for your feedback @jchubber I thought most of your suggestions were appropriate, so I reflected most of them. Are there more areas that need improvement? If not, may I ask who can advice to get approval?

• test_add_comment_with_through_model (testcase)
• test_add_comment_with_abstract_true_option (testcase)
• add migration file to testtest_migrations_with_db_comment.0003_create_proxymodel.py
• add migration file to testtest_migrations_with_db_comment.0004_create_unmanagedmodel.py
• Proxy model and Unmanaged models ignore DB comment options(docs)

[knyghty]

@charettes, would you like to take a look?

[jchubber]

Status update: Patch is Ready for Review

I thought most of your suggestions were appropriate, so I reflected most of them. Are there more areas that need improvement? If not, may I ask who can advice to get approval?

Because this patch now meets every requirement on the development checklist, I've updated the status in the Django ticket tracker so that it shows up in the Patches Needing Review section of the Django Development Dashboard.

The next step is for anyone except the patch author(s) to review the patch using the patch review checklist and either mark the ticket as Ready for checkin on the Django Development Ticket Tracker Entry for Ticket #18468 if everything looks good, or leave comments for improvement and mark the ticket as Patch needs improvement. I'd also request that comments for improvement be left here in Github as well.

[KimSoungRyoul]

I left a few comments, mostly around the docs / grammar. They're all quite subjective.

thanks!

[KimSoungRyoul]

Hi may I know what more I should do? such as requesting reviews from others or reflecting feedback

[jchubber]

@KimSoungRyoul I'm reviewing the using this patch review checklist. Here's the current status of that checklist:

• [x] Documentation: Does the documentation build without any errors (make html, or make.bat html on Windows, from the docs directory)? Yes, it builds with no errors ✅
• [ ] Documentation: Does the documentation follow the writing style guidelines in Writing documentation? There are two documentation issues related to code formatting. See below in the list of remaining changes. 🟡
• [x] Documentation: Are there any spelling errors? None. Looks good ✅
• [x] Bugs: Is there a proper regression test (the test should fail before the fix is applied)? Yes, the tests will fail before the fix is applied ✅
• [x] Bugs: If it’s a bug that qualifies for a backport to the stable version of Django, is there a release note in docs/releases/A.B.C.txt? Bug fixes that will be applied only to the main branch don’t need a release note. Not applicable. No backport needed. ✅
• [x] New Features: Are there tests to “exercise” all of the new code? Yes. Well exercised. ✅
• [x] New Features: Is there a release note in docs/releases/A.B.txt? Yes there is ✅
• [x] New Features: Is there documentation for the feature and is it annotated appropriately with .. versionadded:: A.B or .. versionchanged:: A.B? Yes. ✅
• [x] All code changes: Does the coding style conform to our guidelines? Are there any flake8 errors? You can install the pre-commit hooks to automatically catch these errors. I ran flake8 on all files included in this patch and found no errors. ✅
• [x] If the change is backwards incompatible in any way, is there a note in the release notes (docs/releases/A.B.txt)? Not applicable. This change does not lead to any backwards incompatibility. ✅
• [ ] Is Django’s test suite passing? Not yet... I got one failure, which I'm investigating. I'm not sure if it's related to this patch or not. I'll report back. ⏱ UPDATE: I do see one test failure that is related to this patch. I'll create a separate comment for that.
• [ ] All tickets: Is the pull request a single squashed commit with a message that follows our commit message format? It's not a single squashed commit, but it does have a message that follows our commit message format. I'm not sure how important it is for the patch to be a single squashed commit, but if it's important, then I think KimSoungRyoul is the person who should squash the commit (so you retain authorship). If you are unfamiliar with squashing a commit (without having to create a new PR) then I'll go ahead and mark this as ready for merging and we'll see if the committers are comfortable with not having a single squashed commit. 🟡
• [ ] All tickets: Are you the patch author and a new contributor? Please add yourself to the AUTHORS file and submit a Contributor License Agreement. No. I do not see that @KimSoungRyoul added themselves to the AUTHORS file and I am unsure if they submitted a Contributor License Agreement. @KimSoungRyoul should do both. 🟡 (@KimSoungRyoul here's a link for the contributor license agreement instructions https://www.djangoproject.com/foundation/cla/)

Changes that remain to be made:

• This should be ``help_text`` (not `help_text`)
• This should be ``db_comment`` (not `db_comment `)
• The warning on the db_comment docs here should be removed
• Adding yourself to the AUTHORS file here: https://github.com/django/django/blob/main/AUTHORS .

Once these changes are made, I'll mark this "Ready for checkin" and then a committer will have the chance to review and either accept it or send it back for changes. (I'm not a committer)

[KimSoungRyoul]

all done

• fix documentation follow the writing style ( and remove)
• fix failing testcase
• adding to the AUTHORS (and submit a Contributor License Agreement.)
• squash commit

Thanks again for your guide. @jchubber :bowing_man:

[jchubber]

Updated checklist status:

• [x] Documentation: Does the documentation build without any errors (make html, or make.bat html on Windows, from the docs directory)? Yes, it builds with no errors ✅
• [x] Documentation: Does the documentation follow the writing style guidelines in Writing documentation? Looks good. ✅
• [x] Documentation: Are there any spelling errors? None. Looks good ✅
• [x] Bugs: Is there a proper regression test (the test should fail before the fix is applied)? Yes, the tests will fail before the fix is applied ✅
• [x] Bugs: If it’s a bug that qualifies for a backport to the stable version of Django, is there a release note in docs/releases/A.B.C.txt? Bug fixes that will be applied only to the main branch don’t need a release note. Not applicable. No backport needed. ✅
• [x] New Features: Are there tests to “exercise” all of the new code? Yes. Well exercised. ✅
• [x] New Features: Is there a release note in docs/releases/A.B.txt? Yes there is ✅
• [x] New Features: Is there documentation for the feature and is it annotated appropriately with .. versionadded:: A.B or .. versionchanged:: A.B? Yes. ✅
• [x] All code changes: Does the coding style conform to our guidelines? Are there any flake8 errors? You can install the pre-commit hooks to automatically catch these errors. I ran flake8 on all files included in this patch and found no errors. ✅
• [x] If the change is backwards incompatible in any way, is there a note in the release notes (docs/releases/A.B.txt)? Not applicable. This change does not lead to any backwards incompatibility. ✅
• [x] Is Django’s test suite passing? Passing tests. ✅
• [x] All tickets: Is the pull request a single squashed commit with a message that follows our commit message format? Yes, it's a single squashed commit. ✅
• [x] All tickets: Are you the patch author and a new contributor? Please add yourself to the AUTHORS file and submit a Contributor License Agreement. Yes, @KimSoungRyoul added themselves to the AUTHORS file ✅

👉 Patch Review Checklist is Complete ✅

I have marked this patch Ready for checkin in the Django ticket tracker here. A committer will have the chance to review and either accept it or send it back for changes. (I'm not a committer)

[KimSoungRyoul]

@felixxm

Hi I reflected your comments. 9d769edbbd6e008e4553a18fda4474e5d54fbcf6 I will squash all my commits after approve for your unpainful code review. thank you for your feedback 🙇

summary

• reusable COMMENT SQL

  • define def _comment_sql(self, comment):

  • remove comment related sql or method like a sql_create_table_with_comment and left comment related sql only sql_alter_table_comment and sql_alter_column_comment

• column_comment -> comment

• Each database handles '', None differently. so I define def quote_comment_value(self, value):

[jacobtylerwalls]

Hi @KimSoungRyoul, if this is ready for re-review, please uncheck "Needs improvement" on the Trac ticket, which is how we track waiting-for-author. While here, I noticed the release notes and version annotations need to be moved to 4.1, which is under dev now. Thanks!

[jchubber]

@KimSoungRyoul Thank you for the link to your updated commit! When it's ready to be accepted I can update the issue in trac to remove the Needs improvement flag. That will enable the PR to be re-reviewed by Mariusz (@felixxm) or another maintainer. It's probably not feasible to ask felixxm to do a re-review on a PR that isn't ready to be accepted. Before doing that we should confirm a few things + update a few things in the PR:

• [ ] I did a quick comparison of felixxm's feedback and your new commit, but it was difficult for me to be 100% sure if all of the feedback was reflected yet. Would you be please be willing to please mark conversation(s) as resolved in Github for all of the suggestions that you've already updated in the PR? (If for some reason Github does not give you the option to resolve conversations, then an alternative option is to comment "I've updated the PR based on this feedback. Thanks.")
• [ ] Oracle: felixxm found dozens of failures on Oracle. @KimSoungRyoul were you able to replicate those failures on Oracle, and if so, has your updated patch resolved those failures? If not, I'll try to replicate those failures to see if I can help.
• [ ] felixxm asked "What about introspection of table comments?" and I'm wondering if your updated patch addresses that.
• [ ] Please update release notes and version annotations to 4.1 instead of 4.0 (the current dev version is 4.1 now that 4.0 feature lock has passed)
• [ ] Please accept the changes @smithdc1 proposed
• [ ] Please squash to 1 commit and update the PR with those changes

[claudep]

Hey @KimSoungRyoul, do you plan to continue improving this pull request as per comments?

[KimSoungRyoul]

yes I plan to work again rebased on django4.1 master

[sergun]

Hi @KimSoungRyoul KimSoungRyoul

Thanks a lot for your great job!

I have two questions:

1. Is it safe in your opinion to use your very useful implementation ia a patch (so download https://github.com/django/django/pull/14463.patch and do git apply for my local django installation) for django 4.0.x or 4.1.x?
2. Can you please advise some elegant way to use your implementation but to use verbose_name or help_text as a source for database comments for fields. I try to avoid specifying both things: e.g. help_text + db_comment

[KimSoungRyoul]

I almost forget this pullrequest

thank you for reminding me @sergun

---

Updated checklist status:

https://github.com/django/django/pull/14463#issuecomment-941152900 It's a checklist for the above comment

• [x] (resolve comment which was reflected)
• [x] (Oracle: @felixxm found dozens of failures on Oracle <-- fixed it)
• [x] ( update release notes 4.2)
• [x] (accept feedback)
• [x] (Please squash to 1 commit and update the PR with those changes)