citusdata / django-multitenant

Python/Django support for distributed multi-tenant databases like Postgres+Citus
MIT License
712 stars 117 forks source link

.exclude not fully supported but fails with confusing error message #54

Closed ledburyb closed 4 years ago

ledburyb commented 5 years ago

Example error:

>>> CampaignEmailOptions.objects.exclude(campaign_email_shot__isnull=False)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/usr/local/lib/python3.7/site-packages/django/db/models/query.py", line 250, in __repr__
    data = list(self[:REPR_OUTPUT_SIZE + 1])
  File "/usr/local/lib/python3.7/site-packages/django/db/models/query.py", line 274, in __iter__
    self._fetch_all()
  File "/usr/local/lib/python3.7/site-packages/django/db/models/query.py", line 1242, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/usr/local/lib/python3.7/site-packages/django/db/models/query.py", line 55, in __iter__
    results = compiler.execute_sql(chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 1087, in execute_sql
    sql, params = self.as_sql()
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 489, in as_sql
    where, w_params = self.compile(self.where) if self.where is not None else ("", [])
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 405, in compile
    sql, params = node.as_sql(self, self.connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/where.py", line 81, in as_sql
    sql, params = compiler.compile(child)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 405, in compile
    sql, params = node.as_sql(self, self.connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/where.py", line 81, in as_sql
    sql, params = compiler.compile(child)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 405, in compile
    sql, params = node.as_sql(self, self.connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/lookups.py", line 355, in as_sql
    return super().as_sql(compiler, connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/lookups.py", line 163, in as_sql
    rhs_sql, rhs_params = self.process_rhs(compiler, connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/lookups.py", line 346, in process_rhs
    return super().process_rhs(compiler, connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/lookups.py", line 220, in process_rhs
    return super().process_rhs(compiler, connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/lookups.py", line 92, in process_rhs
    sql, params = compiler.compile(value)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 405, in compile
    sql, params = node.as_sql(self, self.connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/query.py", line 1018, in as_sql
    return self.get_compiler(connection=connection).as_sql()
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 489, in as_sql
    where, w_params = self.compile(self.where) if self.where is not None else ("", [])
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 405, in compile
    sql, params = node.as_sql(self, self.connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/where.py", line 81, in as_sql
    sql, params = compiler.compile(child)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 405, in compile
    sql, params = node.as_sql(self, self.connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/fields/related_lookups.py", line 130, in as_sql
    return super().as_sql(compiler, connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/lookups.py", line 163, in as_sql
    rhs_sql, rhs_params = self.process_rhs(compiler, connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/lookups.py", line 260, in process_rhs
    return super().process_rhs(compiler, connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/lookups.py", line 92, in process_rhs
    sql, params = compiler.compile(value)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 405, in compile
    sql, params = node.as_sql(self, self.connection)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/expressions.py", line 734, in as_sql
    return "%s.%s" % (qn(self.alias), qn(self.target.column)), []
  File "/usr/local/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 396, in quote_name_unless_alias
    r = self.connection.ops.quote_name(name)
  File "/usr/local/lib/python3.7/site-packages/django/db/backends/postgresql/operations.py", line 105, in quote_name
    if name.startswith('"') and name.endswith('"'):
AttributeError: 'NoneType' object has no attribute 'startswith'

I've traced this back to https://github.com/citusdata/django-multitenant/blob/master/django_multitenant/fields.py#L72. This code is assuming that both alias and related_alias have a value which isn't true for this kind of exclude query.

This doesn't seem particularly easy to fix but I would suggest at least catching this condition and raising a more useful error:

e.g.

        if not (alias and related_alias):
            raise ValueError(
                """
                Subquery pushdowns are not currently supported by django-multitenant.
                If you're using .exclude() then you may need to split out your subquery manually.

                e.g. Model.objects.exclude(foo__bar=1) could become Model.objects.exclude(id__in=Model.objects.filter(foo__bar=1))
                """
            )
louiseGrandjonc commented 5 years ago

Hi Ben,

thank you for the issue and the PR, I will review all of this during this week !

ledburyb commented 5 years ago

Hi Louise,

Sounds good. The PR is a bit messy, and mostly just to see if we can agree on changes my team needs or if we should just carry on with our own fork. You can message me on the citus slack channel if you want to discuss anything we've done and why.

louiseGrandjonc commented 4 years ago

Hi Ben,

I'm looking into this issue and can't reproduce it. Could you by any chance share with me the CampaignEmailOptions model?

Thanks

ledburyb commented 4 years ago

@louiseGrandjonc - I'm struggling a little with your test setup, but I've added a failing test case which I believe demonstrates the issue.

The way I resolved this in my fork was to move the join constraint into the get_joining_columns method instead (https://github.com/MachineLabsLtd/django-multitenant/blob/master/django_multitenant/fields.py#L45), therefore ensuring it's part of all subqueries. I then simplified the get_extra_restrictions_check: (https://github.com/MachineLabsLtd/django-multitenant/blob/master/django_multitenant/fields.py#L98).

I'm not sure if this approach has unintended downsides but it doesn't seem to have caused any problems in our project as yet.