Opened 11 years ago

Closed 9 years ago

#785 closed enhancement (duplicate)

many2many table and legacy databases.

Reported by: Ian@… Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

hi.

I have the following table in a legacy database.

CREATE TABLE `hostgroup_membership` (
  `hostid` int(11) NOT NULL default '0',
  `groupid` int(11) NOT NULL default '0',
  KEY `hostid` (`hostid`),
  KEY `groupid` (`groupid`),
  CONSTRAINT `hostgroup_membership_ibfk_1` FOREIGN KEY (`hostid`) REFERENCES `host` (`id`) ON DELETE CA
SCADE,
  CONSTRAINT `hostgroup_membership_ibfk_2` FOREIGN KEY (`groupid`) REFERENCES `hostgroup` (`id`) ON DEL
ETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=latin1 COMMENT='Hostgroup membership';
  

and i would like to use a many2many field in my model to represent this.

right now I have 2 limitations.

  1. I can not specify the table name.
  2. I can not specify the field names and what they map too.

what I would like to be able to do is somehow specify the tablename, as well as how the fields map to their respective column names.

something like

class Host(meta.model):
   Groups = meta.manyTomanyfield(Groups, db_table='foo', columns={self_col:'hostid', to_col:'groupid' })

and have the manytomany code do the right thing.

Attachments (1)

m2m_column_names.patch (5.4 KB) - added by surajbarkale@… 10 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 11 years ago by anonymous

Component: Admin interfaceCore framework

comment:2 Changed 10 years ago by Malcolm Tredinnick

Owner: changed from Adrian Holovaty to Malcolm Tredinnick

comment:3 Changed 10 years ago by Chris Beaven

Component: Core frameworkDatabase wrapper
Triage Stage: UnreviewedDesign decision needed

comment:4 Changed 10 years ago by Malcolm Tredinnick

Triage Stage: Design decision neededAccepted

This is needed functionality. I'm not wrapped in the proposed syntax for column naming, though. Since there are only going to be two columns, we can probably get away with just making them each an attribute (but you have to specify both or none, I guess). Something like self_col_name and remote_col_name would work.

I can't see any reason not to implement this if somebody feels so enthused. It shouldn't be too hard: mostly some fiddling around in django/core/management.py for the initial table creation and django/db/models/related.py for setting up the related class correctly at model creation time, I would guess.

comment:5 in reply to:  4 ; Changed 10 years ago by Russell Keith-Magee

Replying to mtredinnick:

For the benefit of anybody approaching this problem; I'm agreed that two attributes is better than a tuple. Two columns isn't a burden, and there is always the possiblity that one of the default names will be correct.

However, all the other database column/table naming fields in Django are prefixed with db_ (db_column, db_table). It's probably a good idea to keep this convention. db_m2m_column and db_m2m_reverse, perhaps?

comment:6 in reply to:  5 Changed 10 years ago by Malcolm Tredinnick

Replying to russellm:

Replying to mtredinnick:

For the benefit of anybody approaching this problem; I'm agreed that two attributes is better than a tuple. Two columns isn't a burden, and there is always the possiblity that one of the default names will be correct.

Agreed.

However, all the other database column/table naming fields in Django are prefixed with db_ (db_column, db_table). It's probably a good idea to keep this convention. db_m2m_column and db_m2m_reverse, perhaps?

Where "reverse" means "moving further away from this model to the one on the other side of the intermediate table"? Hmmm. :-)

I agree with your real point, though: names matching the db_* pattern would be better. Let's see what the patch creator comes up with.

comment:7 Changed 10 years ago by surajbarkale@…

Has patch: set
Version: SVN

Ok so here is my first patch for Django. I have tested this against a legacy database I am trying to write app for. I have updated 'custom_columns' testcase to use custom columns for m2m relation. However I was unable run the testcase (getting some error with readline probably ipython 8.0 problem). I apologize for the half-backed patch. Can somebody please verify the testcase for me?
I have used db_self_col and db_remote_col as column names.

Index: django/db/models/fields/related.py
===================================================================
--- django/db/models/fields/related.py	(revision 5233)
+++ django/db/models/fields/related.py	(working copy)
@@ -634,6 +634,8 @@
             raw_id_admin=kwargs.pop('raw_id_admin', False),
             symmetrical=kwargs.pop('symmetrical', True))
         self.db_table = kwargs.pop('db_table', None)
+        self.db_self_col = kwargs.pop('db_self_col', None)
+        self.db_remote_col = kwargs.pop('db_remote_col', None)
         if kwargs["rel"].raw_id_admin:
             kwargs.setdefault("validator_list", []).append(self.isValidIDList)
         Field.__init__(self, **kwargs)
@@ -664,7 +666,9 @@
     def _get_m2m_column_name(self, related):
         "Function that can be curried to provide the source column name for the m2m table"
         # If this is an m2m relation to self, avoid the inevitable name clash
-        if related.model == related.parent_model:
+        if self.db_self_col:
+            return self.db_self_col
+        elif related.model == related.parent_model:
             return 'from_' + related.model._meta.object_name.lower() + '_id'
         else:
             return related.model._meta.object_name.lower() + '_id'
@@ -672,7 +676,9 @@
     def _get_m2m_reverse_name(self, related):
         "Function that can be curried to provide the related column name for the m2m table"
         # If this is an m2m relation to self, avoid the inevitable name clash
-        if related.model == related.parent_model:
+        if self.db_remote_col:
+            return self.db_remote_col
+        elif related.model == related.parent_model:
             return 'to_' + related.parent_model._meta.object_name.lower() + '_id'
         else:
             return related.parent_model._meta.object_name.lower() + '_id'
Index: tests/modeltests/custom_columns/models.py
===================================================================
--- tests/modeltests/custom_columns/models.py	(revision 5233)
+++ tests/modeltests/custom_columns/models.py	(working copy)
@@ -30,7 +30,8 @@
 
 class Article(models.Model):
     headline = models.CharField(maxlength=100)
-    authors = models.ManyToManyField(Author, db_table='my_m2m_table')
+    authors = models.ManyToManyField(Author, db_table='my_m2m_table',
+                db_self_col='m_article_id', db_remote_col='m_author_id')
 
     def __str__(self):
         return self.headline

comment:8 Changed 10 years ago by Russell Keith-Magee

Needs documentation: set
Needs tests: set
Patch needs improvement: set

Three comments:

1) This should be attached as a file to the ticket, rather than copied into a comment
2) The m2m test at the end is currently testing db_table. By changing that option, you are removing the regression test for the db_table setting. Don't remove an existing test, just add a new one (e.g., another m2m relation in that model, with some logic taking the relation for a walk).
3) Documentation! There needs to be an entry on the db-api page on these settings. This documentation should be part of your patch.

comment:9 in reply to:  8 Changed 10 years ago by surajbarkale@…

Needs documentation: unset
Needs tests: unset

Replying to russellm:

Three comments:

Thank you for reviewing my patch

1) This should be attached as a file to the ticket, rather than copied into a comment

Sorry won't happen again. I missed it because I was looing for attach button near comment entry field.

2) The m2m test at the end is currently testing db_table. By changing that option, you are removing the regression test for the db_table setting. Don't remove an existing test, just add a new one (e.g., another m2m relation in that model, with some logic taking the relation for a walk).

Added new test case as suggested. Walked the relation from both tables.

3) Documentation! There needs to be an entry on the db-api page on these settings. This documentation should be part of your patch.

Thank you for reminding me that. I have added doc entries in model-api page.

Attached patch file : m2m_column_names.patch

Changed 10 years ago by surajbarkale@…

Attachment: m2m_column_names.patch added

comment:10 Changed 9 years ago by Jacob

Resolution: duplicate
Status: newclosed

#6095 is a more general purpose way of solving this problem, so I'm marking this a duplicate of that one.

Note: See TracTickets for help on using tickets.
Back to Top