Why does Organization have the user ManyToManyField?

[jacobg]

It seems that the related_name on OrganizationUser (i.e., organization_users) which points back to Organization accomplishes the same thing, and in fact self.organization_users is used in some places in the code in place of self.users (see Organization.is_admin). Sorry, I have not gone through actually fully testing what I'm saying, as I am using Django-nonrel and ManyToManyField is not supported at all. But it does appear that using related_name on a ForeignKey is identical to using through on ManyToManyField from the other side of the relationship.

Can you eliminate ManyToManyField to make it nonrel-friendly, and in general to remove redundant fields?

Thanks!

[jacobg]

Ok, I discovered the difference between Organization.users and Organization.organization_users. The users ManyToManyField actually selects the User model, and queries "through" a join to the OrganizationUser model, whereas organization_users just selects OrganizationUser model without retrieving the User model directly.

So I retract my statement that it is redundant. However, it is still not compatible with Django-NonRel. A lot of popular Django packages are designed to work with both regular Django and the nonrel version. Are you willing to have django-organizations also be compatible?

The existing usage scenarios of users is limited, so I think any possible performance impact is negligible. It's used in 4 places:

1. models.py - Organization.add_user - Gets count of organization users. It doesn't matter whether you count User objects or OrganizationUser objects.
2. models.py - Organization.get_or_add_user - Same use as 1.
3. models.py - Organization.is_member - Change it to this:

    def is_member(self, user):
        return next((ou for ou in self.organization_users.all() if ou.user_id == user.id), False) and True

4. forms.py - OrganizationUserAddForm.clean_email - Checks for email uniqueness. I'm not sure why you need to do this check. The save() method already checks for email uniqueness globally, so all the more so it will be unique within the organization. I think instead of doing clean_email, there should be a clean_user to make sure the user does not have already have a OrganizationUser object for the same organization.

[bennylope]

Are you willing to have django-organizations also be compatible?

I don't think it's a question of whether I'm willing for it to be compatible. The answer to that is 'certainly'.

If the question is whether I'm willing to break the interface for existing projects to make it compatible with non-relational databases then the answer right now is "no". The ManyToManyField is pretty useful and I know of a few projects, including those I've worked on, that make use of that field in the project code itself. It's an exposed interface. I'm not yet willing to break that to support non-relational databases.

[jacobg]

Thanks, I totally understand. What if models.py only adds the ManyToManyField if it's not nonrel, and then remove internal usage of it?

[jacobg]

How do you feel with adding the following to models.py to solve this issue?

from django.db import connection, models

if connection.features.supports_joins:
    # this is the case for typical relational sql db
    class AbstractOrganizationBase(TimeStampedModel):

        users = models.ManyToManyField(USER_MODEL, through="OrganizationUser")

        class Meta:
            abstract = True
else:
    # this is typically the NoSQL Django-nonrel case (e.g., mongodb or gae)
    AbstractOrganizationBase = TimeStampedModel

It would also require replacing the existing internal references to the users field with alternative solutions that work without the users field. I already implemented the alternative solutions in my fork; it's no big deal.

Developers can still continue to use the users field as normal, assuming they are using a relational db.

[bennylope]

It seems like a reasonable way of doing it. I'd need try it and see some tests before including it.

[jacobg]

Thanks. Here is a fork diff: https://github.com/jacobg/django-organizations/compare

[bennylope]

I'm curious about the support_joins check. Testing this against each version of Django using the project's tox setup throws this error:

AttributeError: 'DatabaseFeatures' object has no attribute 'supports_joins'

I cannot find any reference to supports_joins in the Django codebase either locally or on GitHub.

What indication do you have that this works?

[jacobg]

Hi Ben,

You're right. I realize now that support_joins was added in the default db backend only for django-nonrel. I mistakenly assumed it is in the main django repository as well, because nonrel makes very minimal changes to the django source code. The primary changes are in separate apps.

In any event, the code check in models.py should probably read:

if getattr(connection.features, 'supports_joins', True):

That is, if the supports_joins attribute does not exist, this means it's regular django and joins are supported. If it exists, then it's django-nonrel and then we check its value. It could be True, because you can use a regular relational db backend with nonrel.

[bennylope]

Closing this - at this point it looks like allowing non-rel functionality is going to take a lot more than toggling this one field.