Fixed #18468 -- Added support for comments on columns and tables.

[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
  ...

[jchubber]

This is great! Very exciting to see the feature almost ready. In addition to the specific comments on specific lines, this PR also needs:

1. Test coverage.
2. An entry added to the version 4.0 release notes

@KimSoungRyoul I would be happy to work with you on any / all of these changes. If you'd like me to put together a PR on your branch to incorporate the recommended changes below, and to include some unit tests and the release note update, please let me know and I'd be happy to put that together this week for you to review / accept / update this PR with.

[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?

[knyghty]

This looks like roughly the right approach to me but I don't know the schema generation code well enough to make much comment otherwise.

For the tests, I'm not sure the test you have is the best way to do it. I think maybe having a look at tests/schema/tests.py and adding something there would be more appropriate, but I'm not sure. Probably it would be good to test the system checks work as well, looking at tests/invalid_models_tests/test_ordinary_fields.py may help. Since you've added introspection code you'll also need to test that, maybe in tests/inspectdb/tests.py.

[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]

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

I think you're right about ManyToManyField, we don't need to add a maintenance burden here. If a user wants to add a comment to the table, this is already supported: they can use a through model or a SQL migration.

[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:

• 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. 🟡
• Documentation: Are there any spelling errors? None. Looks good ✅
• 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 ✅
• 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. ✅
• New Features: Are there tests to “exercise” all of the new code? Yes. Well exercised. ✅
• New Features: Is there a release note in docs/releases/A.B.txt? Yes there is ✅
• New Features: Is there documentation for the feature and is it annotated appropriately with .. versionadded:: A.B or .. versionchanged:: A.B? Yes. ✅
• 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. ✅
• 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)

[rsalmaso]

Seen this PR via @felixxm toot 😄
It seems that this PR doesn't allow to use the verbose_name as comment like django-db-comments does (which we use at $WORK).
Can be db_comment=True solve this?
Or maybe better an Meta option to apply to all fields?

FYI django-db-comments adds comments for all models, which is fancy, maybe an helper method called in project AppConfig.ready() could be fine? 🤔

[felixxm]

Seen this PR via @felixxm toot smile It seems that this PR doesn't allow to use the verbose_name as comment like django-db-comments does (which we use at $WORK). Can be db_comment=True solve this? Or maybe better an Meta option to apply to all fields?
FYI django-db-comments adds comments for all models, which is fancy, maybe an helper method called in project AppConfig.ready() could be fine? thinking

As far as I'm aware, basing db_comment on verbose_name won't work for most of projects, it's too generic. Also, verbose_names are for users and db_comments are for DBAs. You can still use django-db-comments if it works for you, that's why we have 3rd-party packages 📦

[rsalmaso]

As far as I'm aware, basing db_comment on verbose_name won't work for most of projects, it's too generic. Also, verbose_names are for users and db_comments are for DBAs. You can still use django-db-comments if it works for you, that's why we have 3rd-party packages package

Fine, make sense.
I would be happy to remove a dependancy if it is possible 😸

[felixxm]

Added introspection of table comments.

[felixxm]

Added inspectdb support with tests.

[felixxm]

I am 65% satisfied with this patch 📈, so we're making progress 🕐.
I'm going to continue next week 🎅, mostly on docs and tests 🧑‍🏭

[felixxm]

I pushed edits to tests.

[felixxm]

@KimSoungRyoul Thanks for all your efforts 🥇 Welcome aboard ⛵

Thanks y'all for reviews 🌟

It's ready 🚀

[KimSoungRyoul]

Thanks to your reviews, the pullrequest was completed well. 😄

[timgraham]

While looking at CockroachDB support, the following from the PostgreSQL documentation was brought to my attention:

obj_description ( object oid ) → text

Returns the comment for a database object specified by its OID alone.
This is deprecated since there is no guarantee that OIDs are unique
across different system catalogs; therefore, the wrong comment might
be returned.

[felixxm]

Good catch 🎯 We should add a catalog name.