Domanda

I'm thinking on the best way to set a relationship with django's built-in User or Group.

By 'or' I mean that the model instances must be exclusively owned by an User or by a Group.

I think the question should be easy to understand by having a look at my model above.

This is my current implementation. I've been looking at GenericRelations but they don't seen appropiate when restricted to such a small number of models.

EDIT: Refactor with an abstract model.

class OwnedModel(models.Model):

    _owner_user = models.ForeignKey(User, null=True, related_name='%(class)s')

    _owner_group = models.ForeignKey(Group, null=True, related_name='%(class)s')

    class Meta:
        abstract = True

    @property 
    def owner(self):
        return self._owner_user or self._owner_group

    @owner.setter
    def owner(self, value):
        if isinstance(value, User):
            self._owner_user = value
            if self._owner_group:
                self._owner_group = None
        elif isinstance(value, Group):
            self._owner_group = value
            if self._owner_user:
                self._owner_user = None
        else:
            raise ValueError

class RemoteAccess(OwnedModel):
    SSH = 's'
    SUPPORTED_PROTOCOLS = (
        (SSH, "ssh"),
    )

    host = models.CharField(_('host name'), max_length=255, validators=[full_domain_validator])

    protocol = models.CharField(_('protocol'), max_length=1, choices=SUPPORTED_PROTOCOLS, default=SSH)

    credential = models.OneToOneField(RemoteCredential)

My main issues with my current implementation are:

  • How to force it to set an User or Group when creating the instance? Could an __init__ override be the way to go?
  • Is there a better/cleaner/more-efficient implementation?

Thanks!

È stato utile?

Soluzione

I would override the save() method instead and define a custom exception

def save(self, *args, **kwargs):
    if self._owner_user is None and self._owner_group is None:
        raise NoOwner
    super(OwnedModel, self).save(*args, **kwargs)

Overriding the init might cause problems in certain cases where you don't actually have the owner until the save (such as in forms etc.)

I don't think there is a cleaner way to do this and stick with the Django User/Group.

Altri suggerimenti

There are several options how you could implement this depending on your program. The one way is to use inheritance.

class Owner(models.Model):

     def create(self,instance):
         raise NotImplementedError('Method create should be called from subclasses of owner')

class UserOnwer(Owner):
      user = models.ForeignKey(User)
      def create(self,instance):
          self.user = instance


class GroupOnwer(Owner):
      group = modelsForeignKey(Group)
      def create(self,instance):
          self.group = instance

class RemoteAccess(models):
      ....
      owner = models.ForeignKey(Owner)


      def get_onwer():  # shortcut implementation of this method must go to Owner.Manager 
          try:
             return UserOnwner.objects.get(pk = self.owner_id) # or what is more common case
          except UserOnwner.DoesNotExist:
             return GroupOnwer.objects.get(pk = self.owner_id)

I need to say a few words about trade off you pay here. There are extra maximum two queries to get GroupOwner in this case, and you will have problems with lists. To solve some problems you need to provide some extra knowledge to Owner about his children(in cost of 3rd normal form and encapsulation principle).

class Owner(models.Model):
     owner_type = models.CharField(choices('user','group')) 

     @classmethod # factory here
     def create(cls,instance):
         if is_instance(instance,User):
             user_owner = UserOnwer()
             return user_owner.user = instance
         if is_instance(instance,User):
             group_owner = GroupOwner()
             return group_owner.group = instance

class UserOnwer(Owner):
     user = models.ForeignKey(User)


class GroupOnwer(Owner):
      group = modelsForeignKey(Group)


class RemoteAccess(models):
      ....
      owner = models.ForeignKey(Owner)

      def get_onwer():  # shortcut implementation of this method must go to Owner.Manager 
          if self.owner.owner_type == 'user':
             return UserOnwner.objects.get(pk = self.owner_id)
          elif self.owner.owner_type == 'group':
             return GroupOnwer.objects.get(pk = self.owner_id)

Ok that is suitable in some cases, but in some it is not. Still extra query, to avoid it We could to sepparate storage level and behaviour level.

 class Owner(models.Model):
     owner_type = models.CharField(choices('user','group')) 
     user = models.ForeignKey(User,null=True,blank=True)
     group = models.ForeignKey(Group,null=True,blank=True)

     @classmethod
     def create(cls,instance):
         owner = Owner()
         owner.set_owner(instance)


     def set_owner(self,instance):
         if is_instance(instance,User):
            self.owner_type = 'user'
         elif is_instance(instance,Group):
            self.owner_type = 'group'
        self.post_init()
        self.owner_behavior.set_instance(instance) 

    def post_init(self): #factory is moved here
        self.owner_behavior = AbstarctOwnerBehavior()
        if self.owner_type == 'user':
            self.owner_behavior = UserOnwerBehaviour(self)
        elif self.owner_type == 'group':
            self.owner_behavior = GroupOnwerBehaviour(self)

     def get_owner(self):
         return self.owner_behaviour.get_owner()

    def title(self):
        self.owner_behavior.printed_title()


class AbstarctOwnerBehavior(object):

    def __init__(self,owner_instance):
        self.owner_instance = owner_instance

    def set_instance(self, instance):
        raise NotImplementedError()

    def get_instance(self):
        raise NotImplementedError()

    def printed_title(self):
        raise NotImplementedError()


class UserOnwerBehaviour(OwnerBehaviour):

    def get_instance(self):
        return self.owner_instance.user

    def set_instance(self, instance):
        self.owner_instance.user = instance

    def printed_title(self):
        return self.owner_instance.user.username # note here



class GroupOnwerBehaviour(OwnerBehaviour):

    def get_instance(self):
        return self.owner_instance.group

    def set_instance(self, instance):
        self.owner_instance.group = group

    def printed_title(self):
        return self.owner_instance.group.name # and here


 #  and finally a sinal if you are afraid of overwriting __init__
from django.db.models.signals import post_init
from models import Owner


def owner_post_init_handler(sender, instance, **kwargs):
    instance.post_init()


post_save.connect(owner_post_init_handler, sender=Owner)

Yo may beautify this a bit, but I think this code is enought to get the idea. This solution also have it's drawbacks you need to bypass calls from Owner model to behavior(this could be shortcuted) and you lose 3nf form of your database structure. From other parts of your app you need to avoid direct calls on User and Group models(in context of ownership), and use abstraction layer, this may some times cause AbstarctOwnerBehavior interface to grow, and from design point of view we better keep interfaces small. You also have problems in case when post_init signal is not sent. But if you use abstarction layer - you get profit, unneseary if-else cases are removed, which simplifies code, makes it more robust, when well designed, such kind of behaviours are easy to test(as they dont have extra if-else paths) and extedndable.

So there is no one 'best' solution that fits every case and it is up to yours engineering judgment.

Autorizzato sotto: CC-BY-SA insieme a attribuzione
Non affiliato a StackOverflow
scroll top