Opened 9 years ago

Closed 7 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@… 8 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 9 years ago by anonymous

  • Component changed from Admin interface to Core framework

comment:2 Changed 9 years ago by mtredinnick

  • Owner changed from adrian to mtredinnick

comment:3 Changed 8 years ago by SmileyChris

  • Component changed from Core framework to Database wrapper
  • Triage Stage changed from Unreviewed to Design decision needed

comment:4 follow-up: Changed 8 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

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 ; follow-up: Changed 8 years ago by 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.

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 8 years ago by mtredinnick

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 8 years ago by surajbarkale@…

  • Has patch set
  • Version set to 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 follow-up: Changed 8 years ago by russellm

  • 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 8 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 8 years ago by surajbarkale@…

comment:10 Changed 7 years ago by jacob

  • Resolution set to duplicate
  • Status changed from new to closed

#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