Fixed #18468 -- Added support for SQL comments in tables and columns. #12605

KimSoungRyoul · 2020-03-22T10:03:42Z

why don't you support SQL COMMENT ?

help_text is useful for Developer

BUT

DBA can not understand Django Models
They want to see db_comment with SQL not Python

This Issue causes a loss of communication costs between Developer to DBA

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

p.s. Only mysql has been implemented yet

mysql(mariadb) oracle , postgresql has been implemented

Summary

CREATE TABLE with comment
ALTER TABLE with comment
ADD FIeld with comment
ALTER Field with comment

KimSoungRyoul · 2020-03-23T11:47:16Z

close this pullRequest

and i will reopen after modification

…ered_db_table()

KimSoungRyoul · 2020-03-29T11:40:42Z

@felixxm
By the way i want to use this feature in version 2.2.x also

if i pull request to 2.2.2 , is it able to merge?

felixxm · 2020-03-30T05:09:01Z

if i pull request to 2.2.2 , is it able to merge?

No, we don't accept new features to the Django 2.2 or 3.0. This should be targeted to the Django 3.1.

KimSoungRyoul · 2020-04-05T06:29:41Z

@felixxm
Adding a new setting is always a bit controversial
i agree your opinion

(help_text is not relevant to db_comment)

KimSoungRyoul · 2020-04-05T10:46:02Z

2020.04.05

support postgresql db_comment

support oracle db_comment

KimSoungRyoul · 2020-04-30T06:49:31Z

@felixxm
would you mind if you could feedback for this PullRequest?

atombrella

I've left some comments that should relieve some of the review burden from the fellows, and hopefully point this in the right direction. Documentation and tests are missing, please address this! Please see the contribution documentation, and in particular the sections about test coverage for your patch. The feature freeze for Django 3.1 is approaching, so it's likely that you'll need to target this for 3.2.

It may be nice to include support for comments in the introspection of the different databases. Again, SQLite does not support this, so it's likely that another feature flag would come in handy. Although SQLServer is a third party backend, I didn't really see ways to extract this with the database views. I left some links for you to explore this:

Another option would be to use those queries in tests that show that the database actually changes and stores the comments.

Things that should ultimately be in the final documentation are .. versionadded annotations, documentation (with code examples) and tests.

It's not necessary to ping me whenever a part of the feedback has been addressed :)

atombrella · 2020-05-03T08:02:51Z

django/db/backends/oracle/schema.py

    sql_create_column_inline_fk = 'CONSTRAINT %(name)s REFERENCES %(to_table)s(%(to_column)s)%(deferrable)s'
    sql_delete_table = "DROP TABLE %(table)s CASCADE CONSTRAINTS"
    sql_create_index = "CREATE INDEX %(name)s ON %(table)s (%(columns)s)%(extra)s"
+    sql_create_table_with_comment = "CREATE TABLE %(table)s (%(definition)s); " \


Hanging indent is discouraged according to the coding style. See the vertical alignment example.

atombrella · 2020-05-03T08:05:19Z

django/db/backends/base/schema.py

                sql.rename_table_references(old_db_table, new_db_table)

+    def alter_db_table_comment(self, model, old_db_table_comment, new_db_table_comment):
+        logger.info('option "db_table_comment" is supported only mysql ....')


The base class should not have database names in it. logger.info is meant more for local debugging, rather than using it in the core of Django. And this appears to be for an early draft.

atombrella · 2020-05-03T08:10:35Z

django/db/backends/base/schema.py

-            'definition': ', '.join(constraint for constraint in (*column_sqls, *constraints) if constraint),
-        }
+
+        if model._meta.db_table_comment and getattr(self, 'sql_create_table_with_comment', None):


I think the generic self.sql_create_table could easily be adapted for this new scenario. Adding something like %(extra)s. Then you also need to check only if the Model defines that attribute. Please check how the index SQL construction is performed.

atombrella · 2020-05-03T08:12:00Z

django/db/backends/mysql/schema.py

+
+        if field.db_column_comment:
+            sql += " COMMENT '%(db_column_comment)s' " % {
+                'db_column_comment': field.db_column_comment.replace('\'', '"').replace('\n', ' ')


Add trailing comma.

atombrella · 2020-05-03T08:14:48Z

django/db/migrations/operations/__init__.py


 __all__ = [
-    'CreateModel', 'DeleteModel', 'AlterModelTable', 'AlterUniqueTogether',
+    'CreateModel', 'DeleteModel', 'AlterModelTable', 'AlterModelTableComment', 'AlterUniqueTogether',


isort should have handled this. Wrap imports at 79 chars.

atombrella · 2020-05-04T12:24:30Z

django/db/backends/postgresql/schema.py


    sql_delete_procedure = 'DROP FUNCTION %(procedure)s(%(param_types)s)'

+    sql_create_table_with_comment = "CREATE TABLE %(table)s (%(definition)s); " \


Please see my comment about a generic template.

atombrella · 2020-05-04T12:29:24Z

django/db/backends/base/schema.py

+            sql = self.sql_create_table_with_comment % {
+                'table': self.quote_name(model._meta.db_table),
+                'definition': ', '.join(constraint for constraint in (*column_sqls, *constraints) if constraint),
+                "table_comment": model._meta.db_table_comment.replace('\'', '').replace('\n', ' ')


Please add trailing comma.

atombrella · 2020-05-04T21:24:12Z

django/db/backends/base/schema.py

+        if old_field.db_column_comment != new_field.db_column_comment:
+            fragment = self._alter_column_comment_sql(model, new_field, new_type, new_field.db_column_comment)
+            if fragment:
+                if self.connection.vendor not in ['postgresql', 'oracle']:


I'm not really sure what you're doing here. I'm not a fan of vendor names in the base schema class.

atombrella · 2020-05-04T21:24:59Z

django/db/backends/base/schema.py

        )

+    def _alter_column_comment_sql(self, model, new_field, new_type, new_db_comment):
+        raise NotImplementedError("column comment sql depend on Database Dialect, implement or return None")


You're defining the SQL in the backends, so I'm not sure subclasses need to do anything. I think it's worth investigating how you can make this completely generic, and have subclasses implement methods that you call from that method.

felixxm · 2020-11-11T19:52:17Z

Closing due to inactivity.

SoungRyoul Kim added 11 commits March 22, 2020 18:53

add function "alter_sql_comment" to base_schema_editor

a507d0f

declare AlterModelTableComment

399bebe

implements AlterModelTableComment

1ba1840

add fun " generate_altered_db_table_comment" to autodetector.py

045b619

declare db_table_comment field

e22861e

add SQL(ALTER COMMENT) to MySQL SchemaEditor

bfa81b6

fix wrong sql param key

39a1e33

remove useless param

5ab2eb3

add sql param db_table_comment

2436ffc

sort import

e6d1980

set init self.db_table_comment = ''

c23f918

KimSoungRyoul changed the title ~~Refers #31393 -- Support sql comment with django help_text~~ Refs #31393 -- Support sql comment with django help_text Mar 23, 2020

felixxm changed the title ~~Refs #31393 -- Support sql comment with django help_text~~ Fixed #18468 -- Added support for SQL comments in tables and columns. Mar 23, 2020

KimSoungRyoul closed this Mar 23, 2020

KimSoungRyoul reopened this Mar 29, 2020

SoungRyoul Kim added 13 commits March 29, 2020 15:31

fix import order with isort

b51f105

fix help_text.replace('\'','')

204e462

db)table_comment should replace ' -> empty

f9266e3

add self.generate_altered_db_table_comment() before self.generate_alt…

e403c1f

…ered_db_table()

add settings.ENABLE_DB_COMMENT_WITH_HELP_TEXT

0a93e72

modify sql does not execute with comment when db_table_comment is None

70d84f9

separate help_text & db_column_comment

68a100e

add log 'db_table_comment is support only mysql'

32dca9b

remove local settting to .gitignore

237ea1a

remove blank

406e845

reformatting sql line

a8fc3b3

modify sql with db_comment

6fc65e6

remove debugging log

22aeff1

SoungRyoul Kim added 10 commits April 5, 2020 15:35

remove settings.ENABLE_DB_COMMENT_WITH_HELP_TEXT

a9f4de9

remove blank space

d56ec74

remove useless import

09bbf58

prevent no attribute error

8c1027d

remove warning log "only support mysql" & rename sql_param

b8a8b77

support postgresql db_comment

4fba268

support oracle db_comment

7b2f697

fix typo

a1cabc4

add indent

826fccd

fix indent

e8b4297

use double quote

9f0dc83

atombrella reviewed May 6, 2020

View reviewed changes

felixxm closed this Nov 11, 2020

KimSoungRyoul deleted the support-sql-comment-with-django-help-text branch September 20, 2021 10:33


		sql_delete_procedure = 'DROP FUNCTION %(procedure)s(%(param_types)s)'

		sql_create_table_with_comment = "CREATE TABLE %(table)s (%(definition)s); " \

Uh oh!

Fixed #18468 -- Added support for SQL comments in tables and columns. #12605

Fixed #18468 -- Added support for SQL comments in tables and columns. #12605

Uh oh!

Conversation

KimSoungRyoul commented Mar 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

p.s. Only mysql has been implemented yet

mysql(mariadb) oracle , postgresql has been implemented

Summary

CREATE TABLE with comment

ALTER TABLE with comment

ADD FIeld with comment

ALTER Field with comment

Uh oh!

KimSoungRyoul commented Mar 23, 2020

Uh oh!

KimSoungRyoul commented Mar 29, 2020

Uh oh!

felixxm commented Mar 30, 2020

Uh oh!

KimSoungRyoul commented Apr 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KimSoungRyoul commented Apr 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

support postgresql db_comment

support oracle db_comment

Uh oh!

KimSoungRyoul commented Apr 30, 2020

Uh oh!

atombrella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixxm commented Nov 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KimSoungRyoul commented Mar 22, 2020 •

edited

Loading

KimSoungRyoul commented Apr 5, 2020 •

edited

Loading

KimSoungRyoul commented Apr 5, 2020 •

edited

Loading