About: Feature request: Comments in Django models saved to database schema
200 views
Skip to first unread message
Andrei Antoukh
Jul 7, 2012, 3:25:13 PM7/7/12
to django-d...@googlegroups.com
Hello!
I am writing about the issues:
I am not the author of these issues, but I think this feature is interesting to have in the ORM.
I agree that we should not do things just because we can make!
In the source code often put comments to clarify things. These comments for the same project
developer or someone who looks at the source code are often useful to know how
something works or understand something that is not entirely clear.
In SQL's absolutely the same case. Sometimes you need to make comments on both tables to clarify
the purpose and functionality, and in columns. All assume that if you want to understand something,
look at the source code, but this may not always be so.
Right now, the behavior to make comments can be emulated using sql fixtures. And it has these drawbacks:
* Is not portable between different database backends, 3 files need to write redundant for this in mysql,
postgresql and sqlite (sqlite does not natively support comments, but you can emulate sql
http://stackoverflow.com/questions/7426205/sqlite comments -adding-comments-to-tables-and-columns )
* It is more tedious to write SQL statements and you end up not making the comments.
Is a low priority feature (obviously) but I think it's something that could facilitate the ORM.
My first proposal is:
Comments to model tables (for mysql, postgresql and sqlite3) as the first patch.
Without any kind of magic by django, a simple attribute "comment" on object "Meta".
My initial proposal for postgresql implementation: https://github.com/niwibe/django/tree/issue_18468
Andrei Antoukh
PD: sorry for my poor inglish
Andrei Antoukh - <ni...@niwi.be>
http://www.niwi.be/page/about/"Linux is for people who hate Windows, BSD is for people who love UNIX"
"Social Engineer -> Because there is no patch for human stupidity"
"Social Engineer -> Because there is no patch for human stupidity"
Anssi Kääriäinen
Jul 9, 2012, 2:56:28 AM7/9/12
to Django developers
> I am writing about the issues:https://code.djangoproject.com/ticket/13867https://code.djangoproject.com/ticket/18468
I am not sure this is something we want to support. To me a generic
"get_post_sync_sql(connection)" is a better approach. In addition to
comments you could add indexes, constraints and so on in the hook. It
should be called even for non-managed tables. That way you could use
views using the hook. Trivial example:
class UserPostCount(models.Model):
user = models.OneToOneKey(User, primary_key=True,
db_column="user_id", on_delete=models.DO_NOTHING,
related_name='post_count')
post_count = models.IntegerField()
class Meta:
db_table = 'user_post_count_view'
managed = False
def get_post_sync_sql(connection):
if connection.vendor != 'postgresql':
raise NotImplementedError
return [
"""
CREATE OR REPLACE VIEW user_post_count_view AS (
SELECT user.id AS user_id, count(*) AS post_count
FROM user
JOIN user_posts ON user.id = user_posts.user_id
);""",
"""
COMMENT ON VIEW user_post_count_view IS 'A trivial view returning
post count for users.';
"""]
There could also be a get_post_flush_sql(connection) hook.
It seems best if the hooks are called in a second pass - that is, the
whole schema is already installed into the DB before running any of
the post_sync SQL.
Why not use the initial SQL files we already have? They are ran every
time after flush - which means they are useless for defining views for
example - and at least I like to keep the SQL together with model
definition. In addition (IIRC) there are some problems with backends
which do not support executing multiple statements in one go. Still,
one can do logic in the hook, while the initial SQL files are flat
files.
- Anssi
"get_post_sync_sql(connection)" is a better approach. In addition to
comments you could add indexes, constraints and so on in the hook. It
should be called even for non-managed tables. That way you could use
views using the hook. Trivial example:
class UserPostCount(models.Model):
user = models.OneToOneKey(User, primary_key=True,
db_column="user_id", on_delete=models.DO_NOTHING,
related_name='post_count')
post_count = models.IntegerField()
class Meta:
db_table = 'user_post_count_view'
managed = False
def get_post_sync_sql(connection):
if connection.vendor != 'postgresql':
raise NotImplementedError
return [
"""
CREATE OR REPLACE VIEW user_post_count_view AS (
SELECT user.id AS user_id, count(*) AS post_count
FROM user
JOIN user_posts ON user.id = user_posts.user_id
);""",
"""
COMMENT ON VIEW user_post_count_view IS 'A trivial view returning
post count for users.';
"""]
There could also be a get_post_flush_sql(connection) hook.
It seems best if the hooks are called in a second pass - that is, the
whole schema is already installed into the DB before running any of
the post_sync SQL.
Why not use the initial SQL files we already have? They are ran every
time after flush - which means they are useless for defining views for
example - and at least I like to keep the SQL together with model
definition. In addition (IIRC) there are some problems with backends
which do not support executing multiple statements in one go. Still,
one can do logic in the hook, while the initial SQL files are flat
files.
- Anssi
Andrei Antoukh
Jul 9, 2012, 10:58:04 AM7/9/12
to django-d...@googlegroups.com
2012/7/9 Anssi Kääriäinen <anssi.ka...@thl.fi>
Hi Anssi!
I had submitted a proposal very simplistic. But your idea seems great! In fact I love it! And I would solve much more. As you've said: indexes, constrains, etc..
I will present a proposal for implementation with this in mind.
Andrei Antoukh
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Andrei Antoukh
Jul 22, 2012, 4:01:35 PM7/22/12
to django-d...@googlegroups.com
Hello!
I started working on what had been proposed. In the commit https://github.com/niwibe/django/commit/af887029integeintege614ca53d8bdab23f0a40ee7420b9bc20 is the initial implementation. It is based on the Anssi idea.
I have some doubts about how to make things better. The proposed method should return a list of sql statements:
["COMMENT ON TABLE fooapp_sampletable IS 'Foo comment';"]
or a list of tuples with sql statements with possible arguments:
[("COMMENT ON TABLE fooapp_sampletable IS %s", ["Foo comment"])]
Currently, the initial implementation, is a list of tuples, but I'm wondering if the best option. I think that this is not usabe for commands such as sqlall because the returned sql is not complete, if only partial, failing to place them arguments.
Thank you very much!
Andrey Antukh
2012/7/9 Andrei Antoukh <ni...@niwi.be>
Anssi Kääriäinen
Jul 24, 2012, 7:40:57 AM7/24/12
to Django developers
> I started working on what had been proposed. In the commithttps://github.com/niwibe/django/commit/af887029integeintege614ca53d8...<https://github.com/niwibe/django/commit/af887029614ca53d8bdab23f0a40e...>
return value should be list of strings. This might mean that one has
to create the SQL in a way that is susceptible to SQL injection
attacks, but I don't see that as a problem. The post_sync method's SQL
can't be ran or controlled by users, so there is no attack vector.
I have a couple of questions:
1) How does this work in model inheritance situations? If the base
model defines post_sync_sql(), wouldn't that be called again for the
child models? Maybe it is possible to run the method only if it is
defined in the current class directly. This should be possible by
inspecting the model's __dict__ for the method instead of using
hasattr.
2) What is the order in which the post_sync_sql methods are called?
This could be important for some use cases (one view depends on
another view for example). I guess the order is app for app in
INSTALLED_APPS, models in the order they are found from app's
models.py? The order should be tested and documented.
- Anssi
> is
> the initial implementation. It is based on the Anssi idea.
>
> I have some doubts about how to make things better. The proposed method
> should return a list of sql statements:
> ["COMMENT ON TABLE fooapp_sampletable IS 'Foo comment';"]
>
> or a list of tuples with sql statements with possible arguments:
>
> [("COMMENT ON TABLE fooapp_sampletable IS %s", ["Foo comment"])]
>
> Currently, the initial implementation, is a list of tuples, but I'm
> wondering if the best option. I think that this is not usabe for commands
> such as sqlall because the returned sql is not complete, if only partial,
> failing to place them arguments.
I do think the sqlall output should be usable as is. That is, the
> the initial implementation. It is based on the Anssi idea.
>
> I have some doubts about how to make things better. The proposed method
> should return a list of sql statements:
> ["COMMENT ON TABLE fooapp_sampletable IS 'Foo comment';"]
>
> or a list of tuples with sql statements with possible arguments:
>
> [("COMMENT ON TABLE fooapp_sampletable IS %s", ["Foo comment"])]
>
> Currently, the initial implementation, is a list of tuples, but I'm
> wondering if the best option. I think that this is not usabe for commands
> such as sqlall because the returned sql is not complete, if only partial,
> failing to place them arguments.
return value should be list of strings. This might mean that one has
to create the SQL in a way that is susceptible to SQL injection
attacks, but I don't see that as a problem. The post_sync method's SQL
can't be ran or controlled by users, so there is no attack vector.
I have a couple of questions:
1) How does this work in model inheritance situations? If the base
model defines post_sync_sql(), wouldn't that be called again for the
child models? Maybe it is possible to run the method only if it is
defined in the current class directly. This should be possible by
inspecting the model's __dict__ for the method instead of using
hasattr.
2) What is the order in which the post_sync_sql methods are called?
This could be important for some use cases (one view depends on
another view for example). I guess the order is app for app in
INSTALLED_APPS, models in the order they are found from app's
models.py? The order should be tested and documented.
- Anssi
Andrei Antoukh
Jul 26, 2012, 11:55:11 AM7/26/12
to django-d...@googlegroups.com
2012/7/24 Anssi Kääriäinen <anssi.ka...@thl.fi>
Hello!
>> I started working on what had been proposed. In the commithttps://github.com/niwibe/django/commit/af887029integeintege614ca53d8...<https://github.com/niwibe/django/commit/af887029614ca53d8bdab23f0a40e...>
> isI do think the sqlall output should be usable as is. That is, the
> the initial implementation. It is based on the Anssi idea.
>
> I have some doubts about how to make things better. The proposed method
> should return a list of sql statements:
> ["COMMENT ON TABLE fooapp_sampletable IS 'Foo comment';"]
>
> or a list of tuples with sql statements with possible arguments:
>
> [("COMMENT ON TABLE fooapp_sampletable IS %s", ["Foo comment"])]
>
> Currently, the initial implementation, is a list of tuples, but I'm
> wondering if the best option. I think that this is not usabe for commands
> such as sqlall because the returned sql is not complete, if only partial,
> failing to place them arguments.
return value should be list of strings. This might mean that one has
to create the SQL in a way that is susceptible to SQL injection
attacks, but I don't see that as a problem. The post_sync method's SQL
can't be ran or controlled by users, so there is no attack vector.
OK!
I have a couple of questions:
1) How does this work in model inheritance situations? If the base
model defines post_sync_sql(), wouldn't that be called again for the
child models? Maybe it is possible to run the method only if it is
defined in the current class directly. This should be possible by
inspecting the model's __dict__ for the method instead of using
hasattr.
Good observation!
2) What is the order in which the post_sync_sql methods are called?
This could be important for some use cases (one view depends on
another view for example). I guess the order is app for app in
INSTALLED_APPS, models in the order they are found from app's
models.py? The order should be tested and documented.
I'll think about it. Currently uses the same order as syncdb, even if not the most appropriate for this.
We could think of a way to define the order of execution by the user.
I created the asociated ticket for this feature: https://code.djangoproject.com/tidcket/18672
Andrey.
- Anssi
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Anssi Kääriäinen
Jul 26, 2012, 12:16:02 PM7/26/12
to Django developers
On 26 heinä, 18:55, Andrei Antoukh <n...@niwi.be> wrote:
> 2012/7/24 Anssi Kääriäinen <anssi.kaariai...@thl.fi>
> I created the asociated ticket for this feature:https://code.djangoproject.com/tidcket/18672<https://code.djangoproject.com/ticket/18672>
>
> Andrey.
Before proceeding too far in the morel pre/post sync hooks, lets see
if the other current thread about a similar issues yields a better
API. The other thread is about raw SQL on pre/post sync, and while it
isn't dealing with the exact same issue as this thread, there is a lot
of overlap. See http://groups.google.com/group/django-developers/browse_thread/thread/c4dd1a333ae60356
- Anssi
> 2012/7/24 Anssi Kääriäinen <anssi.kaariai...@thl.fi>
>
> Andrey.
Before proceeding too far in the morel pre/post sync hooks, lets see
if the other current thread about a similar issues yields a better
API. The other thread is about raw SQL on pre/post sync, and while it
isn't dealing with the exact same issue as this thread, there is a lot
of overlap. See http://groups.google.com/group/django-developers/browse_thread/thread/c4dd1a333ae60356
- Anssi
Reply all
Reply to author
Forward
0 new messages