tortoise / tortoise-orm

Familiar asyncio ORM for python, built with relations in mind
https://tortoise.github.io
Apache License 2.0
4.37k stars 355 forks source link

unique indexes (for unique_together) on nullable columns need to be optionable for nulls not distinct #1641

Open omidekz opened 3 weeks ago

omidekz commented 3 weeks ago

assume that i have two models shop, customer. each shop can have only one customer with an unique phone number till deletes it. please pay attention that customer's phone_number is not unique in column. that is unique together with its shop and delete state.

from tortoise import Model, fields

class Base(Model):
  deleted_at = fields.DateTimeField(null=True)
  class Meta:
    abstract = True

class Shop(Base):
  title = fields.CharField(...)

class Customer(Base):
  phone_number = fields.CharField(...) # not used unique here as you expect
  shop = fields.ForeignKeyField('models.Shop', 'customers', fields.CASCADE)
  class Meta:
    unique_together = ('shop_id', 'phone_number', 'deleted_at')

but this simple code is not work as i expected in postgres that i tested, created index did'nt follow by nulls not distinct when not follow. unique index considers nulls not equal. tortoise version : 0.21.3

abondar commented 3 weeks ago

Hi!

The fact that it does not use nulls not distinct is intentional (as much intentional it can be for the case of default behaviour)

And I am pretty sure it shouldn't be default behaviour

Although adding support would be reasonable, I think it should be done by allowing it as param in tortoise.contrib.postgres.indexes.PostgreSQLIndex, not by changing behaviour in unique_together

You can contribute this change in PR, or may be I'll be able to take it on somewhere later

I think you can bypass this problem pretty easily by modifying your migration scripts if you have one, or creating index manually if you don't use migration system, as indexes declared on models doesn't affect runtime behaviour

omidekz commented 2 weeks ago

Hi that was great idea. by changes at this PR i solved my problem with following approach:

from tortoise... import PostgresUniqueIndex as PUI, SqliteUniqueIndex as SUI

def unique_index_depends_on_current_db_type(fields, null_fields):
  """this function just support sqlite/postgres unique index and nulls will be consider equal"""
  is_postgres = envs.db_url.lower().startswith('postgres')
  unique_index_class = PUI if is_postgres else SUI
  return unique_index_class(fields=fields, nulls='not distinct' if is_postgres else null_fields)

class Customer(BaseModel):
  class Meta:
    indexes = [unique_index_depends_on_current_db_type(fields=['shop_id','phone_number', 'deleted_at'], null_fields=['deleted_at'])]
omidekz commented 2 weeks ago

another solution is using nonnull field

class BaseModel(Model):
  created_at = fields.DateTimeField(auto_now_add=True)
  updated_at = fields.DateTimeField(auto_now=True)
  is_deleted = fields.BooleanField(default=False)