Opened 19 years ago

Closed 4 weeks ago

Last modified 2 weeks ago

#373 closed New feature (fixed)

Add support for multi-column primary keys.

Reported by: Jacob Owned by: Csirmaz Bendegúz
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: database
Cc: onelson@…, stava@…, erik.engbrecht@…, knyght+django@…, Marinho Brandão, David Larlet, artagnon, Tobu, tristan@…, blyth, vvinet, Joey Wilhelm, stef@…, wprins, jfishman, alexis_m, Wonlay, joejasinski, hector@…, vinilios, broderboy, chouquette, spaceriqui, Marco Bazzani, alexlewin, timbo, Mikhail Korobov, lau@…, rupa108, ludo@…, scott.hebert@…, rogelio.dominguez@…, drdee, davidhalter88@…, Chris Streeter, gezuru@…, andrewford55139@…, garen.p@…, Sergiy Kuzmenko, pawel.suwala@…, cuboci, Ben Finney, kitsunde@…, kkumler, thisgenericname@…, lars@…, trbs@…, cmawebsite@…, Natt Piyapramote, toracle@…, zerks0@…, sky@…, David Sanders, Solomon Ucko, Golan Bar, Michael Schmidt, Damien, Alexandr Artemyev, Charlie Denton, Peter Thomassen, raydeal, Lily Foote, Wilson E. Husin, bcail, şuayip üzülmez, Csirmaz Bendegúz Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the interest of being able to use Django as an admin interface for any db schema, it really should support multiple column primary keys.

Currently, you can "fake" it by declaring one of the keys to be primary in Django and adding a unique constraint to the model. You'd have to do without the auto-generated SQL that Django gives you, but if you're smart enough to know how and why to use multiple primary keys, you can probably write the schema by hand. Still, this is less than ideal.

This is likely to be very difficult.

Attachments (1)

test-m2m (827 bytes ) - added by anonymous 13 years ago.
related field in composite

Download all attachments as: .zip

Change History (219)

comment:1 by Adrian Holovaty, 19 years ago

Component: Core frameworkMetasystem

comment:2 by Adrian Holovaty, 19 years ago

Resolution: wontfix
Status: newclosed

comment:3 by Jacob, 18 years ago

Resolution: wontfix
Status: closedreopened

This is something that gets asked about a lot, so I'm going to reopen this ticket as a place to keep track of it.

Personally I don't have time to work on this, but if the LazyWeb can work up a patch, it would be quite nice to have. From an email I wrote a while back, here are the issues (that I'm aware of) that would need to be solved to make this work:

There's three basic problems in dealing with composite primary keys in Django.

The first is that a number of APIs use "obj._meta.pk" to access the primary key field (for example, to do "pk=whatever" lookups). A composite PK implementation would need to emulate this in some way to avoid breaking everything.

Second, a number of things use (content_type_id, object_pk) tuples to refer to some object -- look at the comment framework, or the admin log API. Again, a composite PK system would need to somehow not break this.

Finally, there's the issue of admin URLs; they're of the form "/app_label/module_name/pk/"; there would need to be a way to map URLs to objects in the absence of a primary key.

comment:4 by brantley, 18 years ago

It seems to me that multiple primary keys could be represented simply as tuples. The admin could function as: "/app_label/module_name/key1,key2/"

comment:5 by Adrian Holovaty, 18 years ago

Brantley: In your URL example, what happens when the key value contains a comma?

comment:6 by anonymous, 18 years ago

Adrian: what if in the current setup, a primary key has a "/" in it?

comment:7 by Adrian Holovaty, 18 years ago

The current setup works with primary keys with slashes in 'em, because the admin-site regular expression is greedy. Try it yourself -- and if there's a problem, please do let us know.

comment:8 by brantley, 18 years ago

I think that might be a fringe case that could be overcome in many ways.

comment:9 by anonymous, 18 years ago

It also seems like the admin can't recognize any unique_together foreign key pair.

comment:10 by C8E, 18 years ago

What about /app_label/module_name/key1/key2/? That way we can think to access to partial key query /app_label/module_name/key1/, like in the date based generic views.

comment:11 by anonymous, 18 years ago

in regards to: "/app_label/module_name/key1,key2/". though not as efficient, primary keys can be text. what happens if they contain a comma, where pk1 = "i do, sometimes" and pk2 = "value of key 2"? /app_label/module_name/i do, sometimes,value of key 2/. or would this not happen?

comment:12 by Adrian Holovaty, 18 years ago

Summary: Django lacks support for multiple-column primary keysAdd support for multiple-column primary keys

comment:13 by Gary Wilson <gary.wilson@…>, 18 years ago

Triage Stage: UnreviewedDesign decision needed

Seems like the details for this still need to be decided.

comment:14 by buriy <yuri@…>, 18 years ago

so why not do escaping for such keys?
Add class ComplexKey...
add some str method or something like this for getting escaped representation of the key.
and what's the best escape sequence for ','... please discuss that without me:)

in reply to:  14 comment:15 by anonymous, 18 years ago

Replying to buriy <yuri@buriy.com>:

and what's the best escape sequence for ','

A second comma?

from django.db.vapourware import urlencode_pk
assert urlencode_pk(("ab","c") == "ab,f"
assert urlencode_pk(("a,b","c") == "a,,b,f"
assert urlencode_pk(("a,,b","c") == "a,,,,b,f"
assert urlencode_pk(("i do, sometimes","value of key 2") == "i do,, sometimes,value of key 2"

...although I'm trying to think of a simple RE that would code this, and I can't think of anything that would work.

comment:16 by ilatypov, 17 years ago

Does the issue of encoding a composite key into URL prevent from solving this ticket?

To address the URL encoding issue, may I suggest to consider comma an unsafe character when encoding parts of the composite primary key? Then an unprotected comma can serve a clear mark of part separator. According to the URI syntax at W3C web site,

http://www.w3.org/Addressing/URL/5_URI_BNF.html ,

the comma character is allowed to appear unprotected in URIs.

So encoding a composite primary key should be as simple as

pk = [url_encode(pk_part, more_unsafe=",") for pk_part in pk_list]
url = ",".join(pklist)

The major roadblock is still the issue of supporting composite primary keys in the models.

comment:17 by ilatypov, 17 years ago

I meant ",".join(pk).

comment:18 by Jacob, 17 years ago

Triage Stage: Design decision neededAccepted

comment:19 by David Cramer, 17 years ago

Owner: changed from nobody to David Cramer
Status: reopenednew

I'm currently working up some support for this -- the work will depend and need to wait on the queryset refactoring code.

The currently plan is to maintain backwards compatibility on instance._meta.pk, but it will throw an exception if accessed when there are multiple pks, and it should be deprecated. There will be a new attributes, pks on the meta class, which will return a tuple of the primary key fields.

comment:20 by David Cramer, 17 years ago

Status: newassigned

in reply to:  description comment:21 by ulvinge@…, 17 years ago

Sorry if this isn't the right place to post this, but I'm a noob... I've only used Python and Django for a week...

Currently, you can "fake" it by declaring one of the keys to be primary in Django and adding a unique constraint to the model. You'd have to do without the auto-generated SQL that Django gives you, but if you're smart enough to know how and why to use multiple primary keys, you can probably write the schema by hand. Still, this is less than ideal.

No you really can't "fake" it... You can't even with custom sql do it.

The problem is in the save function, because when you save an object it overrides all values with the declared primary key.

An example:

class book():
    shelf  = IntegerField(primary_key=True)
    level  = IntegerField()
    index  = IntegerField()

    class Meta:
        unique_together = ('shelf', 'level', 'index')
    # don't mind the following line right now
    primary = ('shelf', 'level', 'index')

Currently you can only have one book per shelf with the current django code... (I actually use 0.96, but I don't think it has changed...)

That was the problem, now to the solution! (Note, as I said, I'm a noob, and I just hacked an afternoon and came up with this)

It needs to have a field called primary specified as shown in the above model.

import django.db.models.manipulators 
import django.db.models.manager 
from django.core import validators 
from django.core.exceptions import ObjectDoesNotExist 
from django.db.models.fields import AutoField, ImageField, FieldDoesNotExist 
from django.db.models.fields.related import OneToOneRel, ManyToOneRel 
from django.db.models.query import delete_objects 
from django.db.models.options import Options, AdminOptions 
from django.db import connection, backend, transaction, models 
from django.db.models import signals 
from django.db.models.loading import register_models, get_model 
from django.dispatch import dispatcher 
from django.utils.datastructures import SortedDict 
from django.utils.functional import curry 
from django.conf import settings 
from itertools import izip 
import types 
import sys 
import os 
 
class Model(models.Model): 
    def save(self): 
        dispatcher.send(signal=signals.pre_save, sender=self.__class__, instance=self) 
 
        non_pks = [f for f in self._meta.fields if not (f.primary_key or (f.name in self.primary))] 
        otpk = [f for f in self._meta.fields if ((not f.primary_key) and (f.name in self.primary))] 
        where_and_clause = '' 
        for f in otpk: 
            where_and_clause = where_and_clause.join(' AND %s=%s ' % \ 
                (backend.quote_name(f.attname), 
                 str(getattr(self, f.attname)))) 
        # TODO: the last value in the above % thingy is probbably unsafe... 
         
        cursor = connection.cursor() 
         
        # First, try an UPDATE. If that doesn't update anything, do an INSERT.
        pk_val = self._get_pk_val() 
        pk_set = bool(pk_val) 
        record_exists = True 
        if pk_set: 
            # Determine whether a record with the primary key already exists. 
            cursor.execute("SELECT 1 FROM %s WHERE %s=%%s %s LIMIT 1" % \ 
                (backend.quote_name(self._meta.db_table), 
                 backend.quote_name(self._meta.pk.column), 
                 where_and_clause), [pk_val]) 
            # If it does already exist, do an UPDATE. 
            if cursor.fetchone(): 
                db_values = [f.get_db_prep_save(f.pre_save(self, False)) for f in non_pks] 
                if db_values: 
                    cursor.execute("UPDATE %s SET %s WHERE %s=%%s %s" % \ 
                        (backend.quote_name(self._meta.db_table), 
                        ','.join(['%s=%%s' % backend.quote_name(f.column) for f in non_pks]),
                        backend.quote_name(self._meta.pk.column),
                        where_and_clause),
                        db_values + [pk_val])
            else:
                record_exists = False
        if not pk_set or not record_exists:
            field_names = [backend.quote_name(f.column) for f in self._meta.fields if not isinstance(f, AutoField)]
            db_values = [f.get_db_prep_save(f.pre_save(self, True)) for f in self._meta.fields if not isinstance(f, AutoField)]
            # If the PK has been manually set, respect that.
            if pk_set:
                field_names += [f.column for f in self._meta.fields if isinstance(f, AutoField)]
                db_values += [f.get_db_prep_save(f.pre_save(self, True)) for f in self._meta.fields if isinstance(f, AutoField)]
            placeholders = ['%s'] * len(field_names)
            if self._meta.order_with_respect_to:
                field_names.append(backend.quote_name('_order'))
                # TODO: This assumes the database supports subqueries.
                placeholders.append('(SELECT COUNT(*) FROM %s WHERE %s = %%s %s)' % \
                    (backend.quote_name(self._meta.db_table), backend.quote_name(self._meta.order_with_respect_to.column), where_and_clause))
                db_values.append(getattr(self, self._meta.order_with_respect_to.attname))
            if db_values:
                cursor.execute("INSERT INTO %s (%s) VALUES (%s)" % \
                    (backend.quote_name(self._meta.db_table), ','.join(field_names),
                    ','.join(placeholders)), db_values)
            else:
                # Create a new record with defaults for everything.
                cursor.execute("INSERT INTO %s (%s) VALUES (%s)" %
                    (backend.quote_name(self._meta.db_table),
                     backend.quote_name(self._meta.pk.column),
                     backend.get_pk_default_value()))
            if self._meta.has_auto_field and not pk_set:
                setattr(self, self._meta.pk.attname, backend.get_last_insert_id(cursor, self._meta.db_table, self._meta.pk.column))
        transaction.commit_unless_managed()

        # Run any post-save hooks.
        dispatcher.send(signal=signals.post_save, sender=self.__class__, instance=self)

As some will see, this is a copy and paste of the original save function, but with additional constraints in the where clause everywhere.

Just import and inherit from this class where you have multiple primary keys and everything will work.

Also you need some sql like this: (for postgres 8.2)

ALTER TABLE app_book DROP CONSTRAINT app_book_pkey;
ALTER TABLE app_book ADD CONSTRAINT app_book_pkey PRIMARY KEY ("shelf", "level", "index")

This is a fix that works now for the impatient (like me), but I would really have it work natively, without this hackish code, so keep the good work up dcramer!

Happy programming wishes Niklas Ulvinge

comment:22 by anonymous, 17 years ago

Thanks, exactly what i was looking for.

Chris, B.
Phim Online

comment:23 by Tom Carrick <knyght@…>, 17 years ago

Cc: knyght+django@… added

in reply to:  description comment:24 by Ravi Gidwani <Ravi.Gidwani@…>, 17 years ago

Replying to jacob:

In the interest of being able to use Django as an admin interface for any db schema, it really should support multiple column primary keys.

Currently, you can "fake" it by declaring one of the keys to be primary in Django and adding a unique constraint to the model. You'd have to do without the auto-generated SQL that Django gives you, but if you're smart enough to know how and why to use multiple primary keys, you can probably write the schema by hand. Still, this is less than ideal.

This is likely to be very difficult.

################################################################################################
# Classes for models that need a composite key. Django as of 0.96 does not support composite keys
# To have composite keys effect/workarround in your model follows the steps:
# Step1: Paste the below classes in your models.py (before your composite models)
# Step2: Extend you class from CompositeKeyModel
# Step3: Define 'unique_together=(("field1","field2","foreignkey__field"),) in your models Meta class
# Step4: additionally you can define unique index on the database
#
# Thats it! The save() on the instances of these models should have the composite key effect.
#
# Send feedback to: Ravi.Gidwani@gmail.com
##################################################################################################

class CompositeKeyModel(models.Model): pass

class IntermediateModelBase(ModelBase):
  def __new__(cls, name, bases, attrs):
    if CompositeKeyModel in bases:
      if bases == (CompositeKeyModel,):
        # create the class via the super method
        newclass = ModelBase.__new__(ModelBase, name, (models.Model,), attrs)
        # but then make it inherit from our model class
        newclass.__bases__ = (CompositeKeyModel,)
        return newclass
      else: raise Exception, "IntermediateModelBase does not support more than one base"
    else:
      return type.__new__(cls, name, bases, attrs)


class CompositeKeyModel(models.Model):
    __metaclass__ = IntermediateModelBase
    def save(self):
         filter = {}
         #contruct model filter on the fly for the fields that listed in the
         # 'unique_together' meta attribute
         for field_name_list in self._meta.unique_together:
             for field in field_name_list:
                 filter[field]='%s' % (self.getFieldValue(field))     

         #use the generated filter to check if the object already exist
         # if so fetch it
         fetched = self.__class__.objects.complex_filter(filter)
         #if fetched, then get its primary key and set it into the object instance
         #that is being saved.
         if(len(fetched) > 0):
             pk = self.getPrimaryKey()
             self.__setattr__(pk.name,fetched[0].__getattribute__(pk.name))
    
         #finally call the super class i.e Model save() method   
         models.Model.save(self)
         
    def getFieldValue(self,fieldName):
        separator = fieldName.find('__')
        if(separator > -1): 
            tokens = fieldName.partition('__')
            foriegnObj = getattr(self, tokens[0])
            return getattr(foriegnObj,tokens[2])
        else:
            return getattr(self, fieldName)

    def getPrimaryKey(self):                                
        for field in self._meta.fields:
            if (field.primary_key):
                return field
        
        raise Exception('No primary key')
         
#################################################################################################         

comment:25 by David Cramer, 17 years ago

Neat hack and all, but we should stick to the topic on trac.

comment:26 by Ravi Gidwani <Ravi.Gidwani@…>, 17 years ago

I agree, but I am sure it can be useful to someone,like me, until this bug is fixed. What say ?

comment:27 by mrts, 17 years ago

milestone: post-1.0

Non-essential for 1.0.

comment:28 by David Cramer, 17 years ago

Status: assignednew

I don't currently have time to reimplement this in qs-rf so if someone else wants to volunteer go for it.

comment:29 by David Cramer, 16 years ago

Status: newassigned

I'm going to be working up a patch in the next few days, please see http://groups.google.com/group/django-developers/browse_thread/thread/4b2370a0652d9135 for discussion.

comment:30 by Marinho Brandão, 16 years ago

Cc: Marinho Brandão added

comment:31 by Adrian Holovaty, 16 years ago

Component: MetasystemDatabase wrapper

comment:32 by Dan Crosta, 16 years ago

Cc: Dan Crosta added

Is the intention to add support only for composite primary keys? What about composite, non-unique indices?

#5805 was closed as a duplicate of this ticket, but I think the intention there was different. If the fix to this ticket will not add non-unique, composite indexing to Django's model API, I suggest re-opening #5805.

comment:33 by David Cramer, 16 years ago

I don't want this to go back and forth as a patch, simply due to how large the changeset is, but I've forked the "unofficial django git repo" and put my patch there: http://github.com/dcramer/django-compositepks/tree/master#

So far it allows declaration and creation of primary key models. You declare them in class Meta: primary_key = ('field', 'field', 'field').

There is no ForeignKey/Relational handlers as of right now, but the admin is mostly working.

comment:34 by Maciej Bliziński, 16 years ago

Cc: Maciej Bliziński added

comment:35 by David Larlet, 16 years ago

Cc: David Larlet added

comment:36 by (none), 16 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:37 by artagnon, 16 years ago

Cc: artagnon added

comment:38 by Tobu, 16 years ago

Cc: Tobu added

comment:39 by anonymous, 16 years ago

Cc: tristan@… added

comment:40 by blyth, 16 years ago

Cc: blyth added

in reply to:  32 comment:41 by fongandrew, 16 years ago

Replying to dcrosta:
Second that. This appears to add primary-key support only. It would be nice to have non-unique composite-key indexing. It would be nice for things like the generic relations in the contenttypes app for instance.

Suggested syntax:

class MyModel(models.Model):
    field_a = models.IntegerField()
    field_b = models.IntegerField()

    class Meta:
        index_together = (('field_a', 'field_b'),)

comment:42 by bohney, 16 years ago

Correct me if I'm wrong, but this seems to be a serious issue for those of us that are using a pre-existing database managed by a DBA that won't change the schema for the table(s) with a Composite Primary Key.

comment:43 by eengbrec, 16 years ago

Cc: erik.engbrecht@… added

comment:44 by vvinet, 15 years ago

Cc: vvinet added

comment:45 by Joey Wilhelm, 15 years ago

Cc: Joey Wilhelm added

+1 on the generic multi-column field. To add to the complexity of the issue (or perhaps this is a different issue?) I have a DBA who has begun implementing multi-part foreign keys, and I would love to see these implemented as well.

Of course, the multi-part primary key is by far a higher priority for me, as I have quite a large number of tables using these, and this renders the Django admin essentially unusable.

comment:46 by anonymous, 15 years ago

Cc: stava@… added

comment:47 by Stef Walter <stef@…>, 15 years ago

Cc: stef@… added

comment:48 by wprins, 15 years ago

Cc: wprins added

comment:49 by jfishman, 15 years ago

Cc: jfishman added

+1

comment:50 by anonymous, 15 years ago

Cc: alexis_m added

comment:51 by Wonlay, 15 years ago

Cc: Wonlay added

comment:52 by joejasinski, 15 years ago

Cc: joejasinski added

comment:53 by Hector Lecuanda, 15 years ago

Cc: hector@… added

comment:54 by Martin Paquette, 15 years ago

Cc: Martin Paquette added

comment:55 by anonymous, 15 years ago

Cc: vinilios added

comment:56 by Oroku Saki, 15 years ago

Sorry to be a rail bird (ie, I'm not skilled enough to help), but what is the rough ETA for this solution?

comment:57 by broderboy, 15 years ago

Cc: broderboy added

comment:58 by chouquette, 15 years ago

Cc: chouquette added

comment:59 by spaceriqui, 15 years ago

Cc: spaceriqui added

comment:60 by Marco Bazzani, 15 years ago

Cc: Marco Bazzani added

comment:61 by unho, 15 years ago

I was very excited about using Django to develop a new web app, but this particular issue makes impossible to use Django for developing it.

What we need is a way to define a primary key for a model which consist on two foreign keys, one from another model and a second from another different model than the first two I mentioned. Of course we will be referring to the first model (the one with a multi-part primary key made of two foreign keys) from a fourth model, in other words, the fourth model has a foreign key pointing to the first model.

I hope you can fix this for other projects since we won't use Django for developing our app.

comment:62 by unho, 15 years ago

And of course it is also needed for several to several relationships which have extra attributes, because they also have compound primary keys that also are foreign keys.

comment:63 by alexlewin, 14 years ago

Cc: alexlewin added

comment:64 by timbo, 14 years ago

Cc: timbo added

comment:65 by mikmani, 14 years ago

Lack of support for composite primary key fields is a major drawback of Django and should be dealt with as a priority. Most applications use databases with tables having composite primary keys. Such applications cannot be migrated to Django which reduces Django's usability.

comment:66 by anonymous, 14 years ago

Cc: onelson@… added

comment:67 by anonymous, 14 years ago

Cc: Mikhail Korobov added

comment:68 by pjroberts, 14 years ago

I'd like to add my voice to those that want this. If you need a linking table between two tables in a many-to-many relationship, it seems absurd to have to add a redundant integer primary key to an already-unique row.

comment:69 by Mike Fogel, 14 years ago

Cc: Mike Fogel added

comment:70 by anonymous, 14 years ago

Cc: lau@… added

comment:71 by Thomas Güttler, 14 years ago

Cc: hv@… added

comment:72 by rupa108, 14 years ago

Cc: rupa108 added

comment:73 by Ludovico Magnocavallo, 14 years ago

Cc: ludo@… added

comment:74 by Erik Allik, 14 years ago

Cc: eallik@… added

comment:75 by Łukasz Rekucki, 14 years ago

Severity: normalNormal
Type: defectNew feature

comment:76 by slaptijack, 14 years ago

Cc: scott.hebert@… added

comment:77 by Antti Kaihola, 14 years ago

Easy pickings: unset

Adding support for composite fields and primary keys into the Django model layer seems to be part of Google's Summer of Code 2011 (see http://www.djangoproject.com/weblog/2011/apr/25/gsoc/ announcement on the Django weblog).

comment:78 by akseli.palen@…, 14 years ago

I really hope this feature will be implemented as soon as possible..

comment:79 by rogelio.dominguez@…, 14 years ago

Cc: rogelio.dominguez@… added

comment:80 by Michal Petrucha, 14 years ago

Owner: changed from David Cramer to Michal Petrucha
Status: assignednew

I'll try to post progress updates here from time to time.

I just kicked off a thread on django-developers@ to discuss the final API (https://groups.google.com/d/topic/django-developers/rF79c8z65cQ/discussion). I'll appreciate comments from anyone interested. (I'm posting a note in here because I realize not everyone reads the mailing list.)

comment:81 by Michal Petrucha, 14 years ago

Status: newassigned

comment:82 by Erik Allik, 14 years ago

Cc: eallik@… removed

comment:83 by drdee, 14 years ago

Cc: drdee added

comment:84 by davidhalter88@…, 13 years ago

Cc: davidhalter88@… added
UI/UX: unset

comment:85 by Michal Petrucha, 13 years ago

Okay, a progress update:
there's a branch on github: https://github.com/koniiiik/django/

Currently, you can define a composite field, set it as the primary key and basically that's it. No support in ForeignKey or any other relationship fields yet, that's what I'll work on starting with the next week. Model validation is missing as well, i. e. CompositeFields are not yet checked for correctness.

The admin should just work, try to explore its functionality and please let me know if you find something broken. I tried to test everything I saw but of course, I may have forgotten about a few details.

All in all, feel free to clone the repo and try it out, any feedback I get will push me forward.

comment:86 by Chris Streeter, 13 years ago

Cc: Chris Streeter added

comment:87 by gezuru@…, 13 years ago

Cc: gezuru@… added

by anonymous, 13 years ago

Attachment: test-m2m added

related field in composite

comment:88 by anonymous, 13 years ago

I've tried using this when one of the component fields is a relationship field, but it doesn't seem to work. I might be doing it wrong. I've attached a small example that can be applied on top of koniiiik's existing tests.

comment:89 by Dan Crosta, 13 years ago

Cc: Dan Crosta removed

comment:90 by anonymous, 13 years ago

Ignore test-m2m for the time being, I didn't mean the FK target to have a composite PK. I'll try to get a useful test case out of my real model tomorrow.

comment:92 by Andrew J Ford, 13 years ago

Cc: andrewford55139@… added

comment:93 by garen.p@…, 13 years ago

Cc: garen.p@… added

comment:94 by Erik Allik, 13 years ago

Why am I getting notifications about this issue even though I'm not in the CC? Or at least Trac only allows me to Add myself, not Remove.

comment:95 by Erik Allik, 13 years ago

Cc: eallik@… added

comment:96 by Erik Allik, 13 years ago

Cc: eallik@… removed

comment:97 by Sergiy Kuzmenko, 13 years ago

Cc: Sergiy Kuzmenko added

comment:98 by Erik Allik, 13 years ago

From now on I'm reporting code.djangoproject.com e-mails as spam as removing myself from CC has no effect whatsoever.

comment:99 by Aymeric Augustin, 13 years ago

(off-topic for this ticket)

RaceCondition, given the comment you left on ticket 16763, it seems that you're aware of the situation. Whenever you contribute to a ticket, you will be notified of further updates by email.

We don't intend to change this behavior because many contributors rely on it. However, we have a manual procedure to remove people who no longer want to receive these emails. Just follow the instructions!.

And of course, don't post while logged-in, or your email will be added to the CC list again. As an alternative, I suggest posting without logging in and signing your messages.

comment:100 by Erik Allik, 13 years ago

So, um, what is then the purpose of the CC list if you get messages anyway?
Basically, still, code.djangoproject.com is sending out spam; it's irrelevant if it's a feature for some people.

comment:101 by pawel.suwala@…, 13 years ago

Cc: pawel.suwala@… added

comment:102 by Lau Bech Lauritzen, 13 years ago

We're still a bunch of hopeful people holding our breaths for this feature to enter django. What's the status? Looks like there's lots of activity on github, is the code at a state where it's reasonably stable? In which case I'd be happy to start testing on some real-life data.

comment:103 by Michal Petrucha, 13 years ago

Well, the current status is, the soc2011/composite-fields branch is stable and I'm just keeping it up-to-date with master. There is some documentation, though I'm afraid it is not complete. Anyway, it only works with non-relationship fields.

As for relationship fields and the auxiliary_fk_fields branch, it is still only in the refactoring phase. I had to modify ForeignKey extensively and it's still not there but I'm trying to fix the bugs one by one. Once the whole test suite succeeds (except for CompositeField tests) I can resume working on composite fields themselves.

Note that this is quite a tedious process since most of the remaining failing test cases require individual handling, therefore I'll appreciate any help with this. Feel free to fork me on github and send pull requests. I don't always find enough free time to work on this myself, I've still got some exams to do and also a Bc. thesis to work on...

comment:104 by Pawel Suwala <pawel.suwala@…>, 13 years ago

I have bugfixed and ported the CompositeKey base class posted by Ravi (4 years ago!) to work with current django version.

Here comes the snippet for people who can't wait for real-ck support in django.

################################################################################################
# Base Class for weak-emulation of composite primary key. 
# To have composite keys workaround in your model, follow these steps: 
# Step1: Paste this snippet before your models in models.py
# Step2: Extend you class from CompositeKeyModel
# Step3: Define 'unique_together=(("field1","field2","field3"),) in your models Meta class
# Step4: Create db-table by hand.
#
# That's it! The save() on the instances of these models should have the composite key effect.
#
# Send feedback to: pawel.suwala@fsfe.org
##################################################################################################
from django.db import models
from django.db.models.base import ModelBase

class CompositeKeyModel(models.Model): pass

class IntermediateModelBase(ModelBase):
  def __new__(cls, name, bases, attrs):
    if CompositeKeyModel in bases:
      if bases == (CompositeKeyModel,):
        # create the class via the super method
        newclass = ModelBase.__new__(ModelBase, name, (models.Model,), attrs)
        # but then make it inherit from our model class
        newclass.__bases__ = (CompositeKeyModel,)
        return newclass
      else: raise Exception, "IntermediateModelBase does not support more than one base"
    else:
      return type.__new__(cls, name, bases, attrs)


class CompositeKeyModel(models.Model):
    __metaclass__ = IntermediateModelBase
    def save(self, *args, **kwargs):
         filter = {}
         #contruct model filter on the fly for the fields that listed in the
         # 'unique_together' meta attribute

         if not self._meta.unique_together:
             raise ValueError('Specify Meta.unique_together to emulate Composite pk')

         for field_name_list in self._meta.unique_together:
             for field in field_name_list:
                 filter[field]='%s' % (self.get_field_value(field))

         #use the generated filter to check if the object already exist
         # if so fetch it
         fetched = self.__class__.objects.complex_filter(filter)
         #if fetched, then get its primary key and set it into the object instance
         #that is being saved.
         if(len(fetched) > 0):
             pk = self.get_primary_key()
             self.__setattr__(pk.name,fetched[0].__getattribute__(pk.name))

         #finally call the super class i.e Model save() method
         models.Model.save(self, *args, **kwargs)

    def _is_foreign_key(self, field): # there must be a better way to do this
        meta_class = getattr(field.__class__, '__metaclass__', None)
        return meta_class == ModelBase

    def get_field_value(self, fieldName):
        field_value = getattr(self, fieldName)
        if self._is_foreign_key(field_value):
            return field_value.pk
        return field_value

    def get_primary_key(self):
        for field in self._meta.fields:
            if (field.primary_key):
                return field

        raise Exception('Your model must have a dummy primary key (id)')
#################################################################################################
## example:
#class MyModel(CompositeKeyModel):
#    id                 = models.AutoField(primary_key=True) # dummy-for-django
#    user               = models.ForeignKey(User)
#    mess               = models.ForeignKey(Message)
#    atta               = models.ForeignKey(Attachment)
#
#    class Meta:
#        unique_together = (('user', 'mess', 'atta'),)
#
# -- You will need to make this table by hand. For postgresql:
#CREATE TABLE app_mymodel (
#    id integer NOT NULL, -- dummy, NO PrimaryKey CONSTRAINT
#    user_id integer NOT NULL, -- Foreign key to some table
#    mess_id integer NOT NULL, -- Foreign key to some table
#    atta_id integer NOT NULL  -- Foreign key to some table
#);

# -- create sequence for ID field (like MySQL auto-increment, but without pk)
# CREATE SEQUENCE app_mymodel_id_seq
#    START WITH 1
#    INCREMENT BY 1
#    NO MINVALUE
#    NO MAXVALUE
#    CACHE 1;
# ALTER SEQUENCE app_mymodel_id_seq OWNED BY app_mymodel.id;

# -- Add real composite Primary Key
# ALTER TABLE ONLY app_mymodel
# ADD CONSTRAINT comp_key PRIMARY KEY (user_id, mess_id, atta_id);


>>> MyModel.objects.create(user_id=1, mess_id=5, atta_id=7)
<MyModel: 1_5_7>
>>> MyModel.objects.create(user_id=1, mess_id=5, atta_id=7)
IntegrityError: duplicate key value violates unique constraint
DETAIL:  Key (user_id, mess_id, atta_id)=(1, 8383, 1321) already exists.

comment:105 by cuboci, 12 years ago

Cc: cuboci added

comment:106 by Ben Finney, 12 years ago

Cc: Ben Finney added

comment:107 by Kit Sunde, 12 years ago

Cc: kitsunde@… added

comment:108 by kkumler, 12 years ago

Cc: kkumler added

comment:109 by thisgenericname@…, 12 years ago

Cc: thisgenericname@… added

comment:110 by thisgenericname@…, 12 years ago

From skimming over the above comments, it seems that one of the biggest issues is the representation of composite primary keys in URLs and in references to model.pk

How feasible would it be to have a set of model methods that:

1) Given a set of primary key fields, return a string representation of the composite key with the requirement that this mapping must be 1:1

2) Given a string representation of a primary key, return some sort of structure that identifies what fields should be at which values... Essentially the inverse of the above.

1 allows the generation of URLs, and allows things that examine model.pk to continue to function.

2 creates the framework necessary to support queries that dereference the pk, including lookups and possibly foreign keys.

The one requirement is that the composite-key-to-string bit must produce output that has only one possible interpretation... e.g does escaping of commas/slashes/whatever as needed. In the vast majority of cases using integer ids, composites can probably be in the form of 1.5 or 1-5 or such and escaping is a non-issue.

comment:111 by Michal Petrucha, 12 years ago

Not at all, this is an issue that has been brought up years ago and it simply ignited the biggest discussions because this is one of the easiest things about this project and practically anyone can give their opinions on that. Personally I spent about a few hours thinking about the string representation, most of which was spent reading the comments on this ticket and then concluding that this is not really an issue.

More precisely, to implement a working string representation suitable for the admin, this is just about everything you need: https://github.com/koniiiik/django/blob/soc2011/composite-fields/django/db/models/fields/composite.py#L151 and https://github.com/koniiiik/django/blob/soc2011/composite-fields/django/db/models/fields/composite.py#L135

There are much more difficult aspects. The work is currently stalled on a refactor of ForeignKey and friends. I haven't had much time to work on this lately because it is the end of semester here and school's been keeping me busy but I still intend to work on this when I get a bit of free time. Unless someone wants to take over in which case I would be glad to assist in any way I can.

However, one more thing about the string representation, there is one nontrivial issue that hasn't been discussed in this ticket at all – GenericForeignKey. Using GenericForeignKey you can create SQL joins between the model containing a GFK and an arbitrary model. The value of a GFK pointing to a composite primary key should probably be a string, at least I don't have any better ideas. However, to be able to create a SQL join between the GFK and the composite field, there needs to be a way to reproduce the string encoding of the composite value in SQL. Something like

blog_posts.objects.annotate(Max('comments__date'))

should generate roughly this SQL query:

SELECT blog_posts.date, blog_posts.title, blog_posts.content, MAX(comments.date)
FROM blog_posts INNER JOIN comments ON (comments.object_id = PERFORM_ESCAPE(blog_posts.date, blog_posts.title))
WHERE comments.content_type = '''the content type of blog_posts'''
GROUP BY (blog_posts.date, blog_posts.title)

The important bit is PERFORM_ESCAPE(blog_posts.date, blog_posts.title) which should probably be a rather complex SQL expression that takes all the constituents of a composite field, turns them into strings, escapes them appropriately and generates one output string out of them. This expression will necessarily be strongly backend-dependent but the encoding should also be consistent across backends.

The bad thing about this is that we need to get this right on the first try. Once we release this feature with a certain encoding, we'll have to stick to that because of backwards compatibility, we can't require all users of this feature to migrate their data each time we change the encoding. With admin URLs this is not an issue as changing the string representation there would just result in slightly different URLs but the admin would still work just fine.

Actually, the last sentence is not entirely true with my current implementation because the admin also uses parts of the contenttypes framework to store the log of admin activity and currently I put the same value in there as the one used in the URL, which means that if I changed the representation, all of my logged admin activity would point to non-existing objects.

comment:112 by jeremyt, 12 years ago

Where does this issue stand on the Django timeline? Have we settled on a solution (I like the approach of the composite field koniiiik has done). I would like to see a primary_key option being placed in the Meta class as well.

comment:113 by Michal Petrucha, 12 years ago

Adding a primary_key option to Meta would be redundant, both the current API for single-column PKs and the proposed API for composite PKs use the primary_key boolean flag on a Field instance. And as far as I understand, attempts to add a new Meta option are usually turned down by the core team (similar to project-level settings). However, if you have good arguments, feel free to state them and this may still be considered.

As far as the progress goes, there is no timeline. The work is stalled once again for lack of time (school and other projects requiring immediate attention) but if the core team agrees, I'd like to take this to the final stages this summer as part of GSoC.

comment:114 by jeremyt, 12 years ago

I think the primary_key option in the Meta makes since from the ease of use/flow prospective. I'm not saying that we should remove the option from the field either but it should only be specified in one space. Just like a CREATE TABLE statement really. If I have a primary key of a single column, I can specify it on that one column directly but if I have multiple, I place the primary key explanation as a constraint and not a column/field itself.

In the end, the model would be the same except the user doesn't have to manually declare the composite field(they still can if they want to though), but system would generate the composite field for them and make it the pk. But if the primary_key is missing everywhere, it continues to create the AutoField pk just as it does today.

Does this disagree with the current schema?

comment:115 by Michal Petrucha, 12 years ago

Well… The Meta.primary_key option was the first proposed API and the outcome of this discussion was that the BDFL disliked this option and preferred an explicit CompositeField. The arguments for either option were mostly a matter of personal preference, therefore we settled on the one chosen by Jacob.

Personally, I don't really care. I can deliver the code for either one, there's not that much of a difference as far as the implementation is concerned. I'll start a thread on django-developers@ in the upcoming weeks regarding the status of this project and my plans, that could be a good opportunity for you to bring this up. Just keep in mind that this feature has a history of attracting lots of bikeshedding discussions while the more serious architectural issues are mostly being ignored. To me, this smells more like the former. So, whatever the final decision on django-developers@ is, I'll just accept it and write it that way.

comment:116 by jeremyt, 12 years ago

Yeah.. the ideas on this have been plentiful as shown by that thread. The optional primary_key Meta option definitely goes with the DRY principle that Django is based upon but as long as the scenario is enabled, then I'm ok with spelling it out every time.

This patch plus the work I've done on multi-column joins https://code.djangoproject.com/ticket/19385 should open up some major use cases. I lend a helping hand to this issue, as well, to handle any concerns.

comment:117 by Andrew J Ford, 12 years ago

Jeremy makes a good point about placing primary_key as a constraint within Meta. However, for the interest of time IMO, sticking with the current plan is the best path. Adding primary_key to Meta later as an afterthought (assuming the rest of the community finds a need for it) should be simple enough without breaking (future) existing code.

What is the difference between the auxillary_fk_fields and soc2011/composite-fields branches? How often is master merged? Obviously, this isn't a simple change. Maybe the community could help out a little if they had some status of what they could do to help.

comment:118 by Michal Petrucha, 12 years ago

I apologize for the lack of timely response.

#19385 looks like it implements the last big piece that is missing from my work. I'll need to have a more thorough look at it though.

The current status of my work is kinda sad. The repo is badly out of date because it has become a real pain to keep it in sync with master. I won't get into much detail about why here, but I think it isn't worth the effort to try to sync it up. Basically, the problem is that I first implemented CompositeField (in branch soc2011/composite-fields) without ForeignKey support and then I started to refactor ForeignKey on top of that (auxiliary_fk_fields). This has become a real nightmare to maintain. I managed to get the branches through the initial Py3k changes but that required carefully merging each upstream commit one by one, after that there were numerous cleanups in the ORM which I didn't get around to merging.

However, that doesn't mean I'm giving up, I've got other plans. The idea is to do it the other way around, first refactor ForeignKey and only after it works with simple fields, port CompositeField on top of it. This should still make it possible to reuse most of the code I wrote throughout the past two years and make the changes easier to follow.

I'd like to apply for the upcoming Google Summer of Code with this to finally put this to rest, so keep an eye out on django-developers@ for an email in the near future.

comment:119 by lars@…, 11 years ago

Cc: lars@… added

comment:120 by trbs, 11 years ago

Cc: trbs@… added

comment:121 by Collin Anderson, 10 years ago

Cc: cmawebsite@… added

comment:122 by Aron Podrigal, 10 years ago

Cc: Aron Podrigal added

comment:123 by Natt Piyapramote, 10 years ago

Cc: Natt Piyapramote added

comment:124 by Jeongsoo, Park, 9 years ago

Cc: toracle@… added

comment:125 by Asif Saifuddin Auvi, 9 years ago

these are the draft dep to implement composite primary key on django orm

https://github.com/django/deps/blob/master/draft/0191-composite-fields.rst

and

https://github.com/django/deps/blob/master/draft/0192-standalone-composite-fields.rst

anyone interested to completed this feature could have some look on them.

comment:126 by Asif Saifuddin Auvi, 9 years ago

comment:127 by Asif Saifuddin Auvi, 9 years ago

Version: master

comment:128 by Thomas Güttler, 9 years ago

Cc: hv@… removed

comment:129 by Asif Saifuddin Auvi, 8 years ago

As per conversation in IRC the existing Deps should also be revised based on other works and also https://gist.github.com/koniiiik/5408673

comment:130 by Asif Saifuddin Auvi, 8 years ago

#16508 could be related

Last edited 8 years ago by Tim Graham (previous) (diff)

comment:131 by Fernando Gutiérrez, 8 years ago

Cc: zerks0@… added

comment:132 by Schuyler Duveen, 8 years ago

Cc: sky@… added

comment:133 by David Sanders, 7 years ago

Cc: David Sanders added

comment:134 by Solomon Ucko, 6 years ago

Cc: Solomon Ucko added

It's been about 13 years since this issue was created and about 8 years since work on it has begun, and it still hasn't been implemented! Does the basic functionality at least work? FWIW, AFAIK the main motivation for this feature is "relationship tables" (i.e. ones that link 2 or more tables together), which is relatively common. Also, a better API might be allowing primary_key to be set on multiple fields. For my use case, I don't need to reference the set of columns directly from the database, just in the view logic. (The item specified by the URL and the current user are my two primary keys.) I could just use unique_together, but I'd rather not unnecessarily add an extra column. It's not that much of a problem, but I'd prefer not to need to.

comment:135 by Golan Bar, 6 years ago

Cc: Golan Bar added

comment:136 by Asif Saifuddin Auvi, 6 years ago

Owner: Michal Petrucha removed
Status: assignednew

comment:137 by Michael Schmidt, 5 years ago

Cc: Michael Schmidt added
Owner: set to Michael Schmidt
Status: newassigned

comment:138 by Damien, 4 years ago

Cc: Damien added

comment:139 by Thomas van Gulick, 4 years ago

Cc: Thomas van Gulick added

comment:142 by Alexandr Artemyev, 3 years ago

Cc: Alexandr Artemyev added

comment:143 by Charlie Denton, 3 years ago

Cc: Charlie Denton added

comment:144 by Thomas van Gulick, 3 years ago

Cc: Thomas van Gulick removed

comment:145 by Mike Fogel, 3 years ago

Cc: Mike Fogel removed

comment:146 by Aron Podrigal, 3 years ago

Cc: Aron Podrigal removed

comment:147 by Mariusz Felisiak, 3 years ago

Owner: Michael Schmidt removed

comment:148 by Mariusz Felisiak, 3 years ago

Status: assignednew

comment:149 by Peter Thomassen, 3 years ago

Cc: Peter Thomassen added

comment:150 by raydeal, 3 years ago

Cc: raydeal added
Owner: set to raydeal
Status: newassigned

I would like to dive into this issue and see how can I help.

comment:151 by אורי, 3 years ago

Cc: אורי added

comment:152 by raydeal, 3 years ago

I am digging in Dj ORM code and trying understand how FK works underneath.

comment:153 by raydeal, 3 years ago

Owner: raydeal removed
Status: assignednew

comment:154 by elonzh, 3 years ago

Cc: elonzh added

comment:156 by Mariusz Felisiak, 2 years ago

Needs documentation: set
Owner: set to Clouds
Patch needs improvement: set
Status: newassigned

comment:157 by Clouds, 2 years ago

Hello, the main work of CompoisteField has been done and I'm now working on primary_together/PrimaryKeyConstraint.
While I've got several questions:

  1. How to write test for migration? (or witch existing test as example)
    1. test if the model generating correct models.State
    2. test if from OldModel to NewModel would success migrated
  2. Shall we have PrimaryKeyConstraint and CompositeField in one PR?
    1. primary (without multicolumn) first, then composite
    2. composite (cannot set as PK) first, then primary

The main blocker split it into 2 PR might be hard to add meaningful tests in first PR.

comment:158 by Lily Foote, 2 years ago

Cc: Lily Foote added

comment:159 by Wilson E. Husin, 21 months ago

Cc: Wilson E. Husin added

comment:160 by Appuchia, 20 months ago

Cc: Appuchia added

comment:161 by Appuchia, 20 months ago

Cc: Appuchia added

comment:162 by Mariusz Felisiak, 13 months ago

Summary: Add support for multiple-column primary keysAdd support for multi-columns fields.

comment:163 by Mariusz Felisiak, 13 months ago

#5929 was marked as a duplicate of this ticket.

comment:164 by HAMA Barhamou, 13 months ago

Dear Members of Tickets #5929 and #373, I invite you to join a new discussion to clarify the differences and overlaps between these two tickets and address why #373 was marked as a duplicate

https://forum.djangoproject.com/t/request-for-clarification-and-guidance-on-the-status-of-ticket-5929-following-closure-of-ticket-373/26075

comment:165 by bcail, 13 months ago

Cc: bcail added

comment:166 by אורי, 13 months ago

Cc: אורי removed

comment:167 by şuayip üzülmez, 13 months ago

Cc: şuayip üzülmez added

comment:168 by Csirmaz Bendegúz, 9 months ago

Last edited 9 months ago by Csirmaz Bendegúz (previous) (diff)

comment:169 by Martin Paquette, 8 months ago

Cc: Martin Paquette removed

comment:170 by Csirmaz Bendegúz, 7 months ago

Needs documentation: unset
Owner: changed from Clouds to Csirmaz Bendegúz
Patch needs improvement: unset

comment:171 by Csirmaz Bendegúz, 7 months ago

Patch needs improvement: set

comment:172 by Csirmaz Bendegúz, 7 months ago

Patch needs improvement: unset

comment:173 by Csirmaz Bendegúz, 6 months ago

Cc: Csirmaz Bendegúz added

comment:174 by Sarah Boyce, 6 months ago

Patch needs improvement: set

comment:175 by Csirmaz Bendegúz, 5 months ago

Patch needs improvement: unset

I have a patch but it's quite large.
I separated the tuple lookups to another PR for easier review, please check.

https://github.com/django/django/pull/18404

comment:176 by Sarah Boyce <42296566+sarahboyce@…>, 5 months ago

In 1eac690d:

Refs #373 -- Added tuple lookups.

comment:177 by Sarah Boyce, 5 months ago

Patch needs improvement: set

comment:178 by Csirmaz Bendegúz, 5 months ago

Patch needs improvement: unset

I separated another small refactoring for easier review.
This PR introduces the Field.is_set() function, please check.

https://github.com/django/django/pull/18450

comment:179 by Natalia Bidart, 4 months ago

Patch needs improvement: set

Setting patch needs improvement following feedback in PR 18450

in reply to:  179 comment:180 by Csirmaz Bendegúz, 4 months ago

Patch needs improvement: unset

Replying to Natalia Bidart:

Setting patch needs improvement following feedback in PR 18450

Thank you for the thorough review Natalia!
You're completely right. I adjusted my PR.

comment:181 by Natalia Bidart, 4 months ago

Patch needs improvement: set

I found a few more candidates for replacement in PR 18450, setting the flag until Ben confirms (or rejects :-)).

in reply to:  181 comment:182 by Csirmaz Bendegúz, 4 months ago

Patch needs improvement: unset

Replying to Natalia Bidart:

I found a few more candidates for replacement in PR 18450, setting the flag until Ben confirms (or rejects :-)).

Thank you! I had some reasons for not changing those but now I actually think you're right, most of these can be refactored as well.

comment:183 by GitHub <noreply@…>, 4 months ago

In 5865ff5:

Refs #373 -- Added Model._is_pk_set() abstraction to check if a Model's PK is set.

comment:184 by Maciej Bliziński, 4 months ago

Cc: Maciej Bliziński removed

in reply to:  183 comment:185 by Csirmaz Bendegúz, 4 months ago

Thanks Natalia!

Once again, I found two tiny adjustments that I separated into their own PRs for easier review, they should be a quick win:

  1. Removed outdated comment from RelatedIn. (#18564)
  2. Added ColPairs.__repr__. (#18565)

comment:186 by Sarah Boyce <42296566+sarahboyce@…>, 3 months ago

In 347ab72:

Refs #373 -- Improved test coverage of tuple lookup checks.

This also removed unreachable checks.

comment:187 by Sarah Boyce <42296566+sarahboyce@…>, 3 months ago

In 727587c0:

Refs #373 -- Refactored variable assignments in ColPairs and tuple lookups.

comment:188 by GitHub <noreply@…>, 3 months ago

In 96c9907:

Refs #373 -- Removed outdated comment in RelatedIn lookup.

comment:189 by Csirmaz Bendegúz, 3 months ago

I have made some improvements to the tuple lookups.

My goal is to make tuple lookups easier to use.
Once these improvements are merged, I can separate the supports_tuple_in_subquery feature into its own PR for easier review.

Last edited 3 months ago by Csirmaz Bendegúz (previous) (diff)

comment:190 by elonzh, 3 months ago

Cc: elonzh removed

comment:191 by Sarah Boyce <42296566+sarahboyce@…>, 3 months ago

In 9ca1f6ef:

Refs #373, Refs #24121 -- Added ColPairs.repr().

comment:192 by Sarah Boyce <42296566+sarahboyce@…>, 3 months ago

In c2c7dbb:

Refs #373 -- Updated TupleIsNull lookup to check if any is NULL rather than all.

Regression in 1eac690d25dd49088256954d4046813daa37dc95.

comment:193 by Sarah Boyce <42296566+sarahboyce@…>, 3 months ago

In 5ed72087:

Refs #373 -- Added support for using tuple lookups in filters.

comment:194 by Csirmaz Bendegúz, 3 months ago

Thank you Sarah!

I have submitted a follow-up to add the tuple lookup validations that got removed previously, this time with extensive test coverage:
Refs #373 -- Added additional validations to tuple lookups.

comment:195 by Sarah Boyce <42296566+sarahboyce@…>, 2 months ago

In 97c05a6:

Refs #373 -- Added additional validations to tuple lookups.

comment:196 by Csirmaz Bendegúz, 2 months ago

Thank you Sarah!

I have submitted the follow-up: Refs #373 -- Added TupleIn subqueries.
It separates the supports_tuple_in_subquery database feature from the main PR (#18056) for easier review.

Cheers

Last edited 2 months ago by Csirmaz Bendegúz (previous) (diff)

in reply to:  162 ; comment:197 by Csirmaz Bendegúz, 2 months ago

Replying to Mariusz Felisiak:

Summary: Add support for multiple-column primary keys → Add support for multi-columns fields.

It would be best to limit the scope of this ticket to the original goal of implementing composite primary keys.

We can include admin support + generic foreign key support as requested by Jacob, but we shouldn't extend the scope beyond that. Alternatively we could handle admin + generic fk support in their own follow-up tickets.

Last edited 2 months ago by Csirmaz Bendegúz (previous) (diff)

comment:198 by Sarah Boyce <42296566+sarahboyce@…>, 8 weeks ago

In 03c0a3d:

Refs #373 -- Used a feature flag to disable composite subquery test on MySQL.

comment:199 by Sarah Boyce <42296566+sarahboyce@…>, 7 weeks ago

In f7601ae:

Refs #373 -- Added TupleIn subqueries.

comment:200 by Csirmaz Bendegúz, 7 weeks ago

Thanks Sarah!

I can't find anything else that can be merged separately.
I rebased Fixed #373 -- Added CompositePrimaryKey.

It looks big but there are only 442 lines of code changes in django, the rest are docs and tests.
Any review is appreciated.

in reply to:  197 ; comment:201 by Ben Finney, 7 weeks ago

Replying to Csirmaz Bendegúz:

It would be best to limit the scope of this ticket to the original goal of implementing composite primary keys.

I agree with this. Can we please re-title this issue, back to “Add support for multiple-column primary keys”?

Csirmaz has an implementation of a composite primary key, and it correctly refers to this issue (#373) requesting composite primary key.

The implementation of an *individual field* with multiple columns, is not the same thing, and in principle is not required. (In practice it may be one feasible implementation, but it is not necessarily related to this one.) There is an existing issue (#5929) requesting multi-column fields, which I argue should be re-opened since it is a distinct issue.

I ask that the title change, made last year (see https://code.djangoproject.com/ticket/373?replyto=197#comment:162 ) should be reversed, as Csirmaz describes.

in reply to:  201 comment:202 by Ben Finney, 7 weeks ago

Replying to Ben Finney:

The implementation of an *individual field* with multiple columns, is not the same thing, and in principle is not required. (In practice it may be one feasible implementation, but it is not necessarily related to this one.) There is an existing issue (#5929) requesting multi-column fields, which I argue should be re-opened since it is a distinct issue.

(Yes, I'm aware there was a discussion opened on a different platform to address this. That discussion is not open to anyone who can't use a GitHub account to authenticate, so I raise the matter here where I can participate.)

comment:203 by Ülgen Sarıkavak, 7 weeks ago

Cc: Ülgen Sarıkavak added

comment:204 by Sarah Boyce, 6 weeks ago

We have received several approvals to https://github.com/django/django/pull/18056 and plan to land this soon
I would like to scope this ticket to composite/multi-column primary keys and create the following new tickets:

  • ticket for composite/multi-column FK support (related existing PR: https://github.com/django/django/pull/18205)
  • ticket for Generic FKs to support composite primary keys
  • ticket for the admin to support Models with multi-column fields (esp. CompositePrimaryKeys)

Unless someone disagrees, I will rename this to Add support for multi-columns primary key fields. and create other tickets shortly

in reply to:  204 comment:205 by Ben Finney, 6 weeks ago

Thank you for moving on this, Sarah.

Replying to Sarah Boyce:

I would like to scope this ticket to composite/multi-column primary keys and create the following new tickets:

  • ticket for composite/multi-column FK support (related existing PR: https://github.com/django/django/pull/18205)
  • ticket for Generic FKs to support composite primary keys
  • ticket for the admin to support Models with multi-column fields (esp. CompositePrimaryKeys)

Yes, those are great.

Unless someone disagrees, I will rename this to Add support for multi-columns primary key fields. and create other tickets shortly

Slight disagreement: a composite key is not a field, it's a key (composed of multiple fields). I suggest "Add support for multi-column keys" is accurate.

comment:206 by Simon Charette, 6 weeks ago

Slight disagreement: a composite key is not a field, it's a key (composed of multiple fields). I suggest "Add support for multi-column keys" is accurate.

It's more than a key in this case Ben as the proposed changes lays down the foundation to refer to the primary key as a field itself. So in this case it is a field in the Django sense even if it's not a field/column in the SQL sense.

comment:207 by Appuchia, 6 weeks ago

Cc: Appuchia removed

comment:208 by Csirmaz Bendegúz, 6 weeks ago

Yes, my proposal is to implement composite PKs as a virtual field. This has been discussed before I came along, many believe it's the most natural way to implement this feature in Django. I pushed against it initially, but after working on the issue for a long time I had to admit it's the best approach.

Last edited 6 weeks ago by Csirmaz Bendegúz (previous) (diff)

comment:209 by Sarah Boyce, 4 weeks ago

Summary: Add support for multi-columns fields.Add support for multi-column primary keys.
Triage Stage: AcceptedReady for checkin

comment:210 by Appuchia, 4 weeks ago

Cc: Appuchia removed

comment:211 by Sarah Boyce <42296566+sarahboyce@…>, 4 weeks ago

Resolution: fixed
Status: assignedclosed

In 978aae43:

Fixed #373 -- Added CompositePrimaryKey.

Thanks Lily Foote and Simon Charette for reviews and mentoring
this Google Summer of Code 2024 project.

Co-authored-by: Simon Charette <charette.s@…>
Co-authored-by: Lily Foote <code@…>

comment:212 by Claude Paroz, 4 weeks ago

Thanks a lot to all people involved in fixing this!! I think a special price could be granted to those contributing to closing the oldest Django ticket :-)

Csirmaz, you are our hero of the day (and even more)!

comment:213 by Peter Thomassen, 4 weeks ago

Great achievement indeed!

https://github.com/django/django/blob/978aae4334fa71ba78a3e94408f0f3aebde8d07c/docs/topics/composite-primary-key.txt#L129-L132 says:

ForeignObject is an internal API. This means it is not covered by our
:ref:deprecation policy <internal-release-deprecation-policy>.

However, release notes mention various deprecation stuff around ForeignObject, such as ForeignObject.get_reverse_joining_columns() in release 5.0.

So, it seems like this note in the docs is in contradiction to what's practiced. Is this a docs bug? (Then, perhaps it should be fixed before shipping it.)

(My preference would be for ForeignObject to be public API, as it's becoming more and more important -- e.g., when using a GeneratedField with a related manager, and now with composite PK.)

comment:214 by Sarah Boyce, 4 weeks ago

However, release notes mention various deprecation stuff around ForeignObject, such as ForeignObject.get_reverse_joining_columns() in release 5.0.

Sometimes we decide to follow a deprecation process for things that are not documented. It's decided case by case. However, when it is part of the public api and we have to follow the deprecation policy - I think the statement is still correct

comment:215 by Csirmaz Bendegúz, 4 weeks ago

Thanks! :)
I opened 4 follow-ups:

  1. #35941 - Add composite GenericForeignKey support
  2. #35953 - Add composite PK admin support
  3. #35956 - Add composite foreign keys
  4. #35957 - Allow AutoFields in composite primary keys

I'm planning to work on the first two in the coming weeks.

Last edited 2 weeks ago by Csirmaz Bendegúz (previous) (diff)

comment:216 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

In 81cf690:

Refs #373 -- Fixed CompositePKChecksTests.test_composite_pk_cannot_include_generated_field() test crash on databases with no GeneratedField support.

comment:217 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

In 49761ac9:

Refs #373 -- Simplified DatabaseIntrospection.get_constraints() tests for composite primary keys.

comment:218 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

In 28f81a1:

Refs #373 -- Fixed CompositePrimaryKey tests if yaml isn't installed.

comment:219 by Csirmaz Bendegúz, 2 weeks ago

#5929 was marked as a duplicate of this ticket.
I think we should re-open it and treat it as a follow-up to #373 (#5929 - Add generic CompositeFields).
I'm not planning to work on this, but maybe someone else can pick it up.

Last edited 2 weeks ago by Csirmaz Bendegúz (previous) (diff)

comment:220 by Ülgen Sarıkavak, 2 weeks ago

Cc: Ülgen Sarıkavak removed
Note: See TracTickets for help on using tickets.
Back to Top