tortoise / tortoise-orm

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

Add `table_name_generator` attribute to Meta for dynamic table name generation #1770

Closed Abdeldjalil-H closed 6 days ago

Abdeldjalil-H commented 1 week ago

Add Table Name Generator Feature

Description

This PR adds a new table_name_generator attribute to the Model Meta class. This allows users to dynamically generate table names using a custom function instead of explicitly setting the table name or relying on defaults.

The feature includes:

Motivation and Context

This change provides users with more flexibility in how table names are generated. This should solve #1026 #1181

The feature promotes cleaner code by centralizing table naming logic and reducing repetition across models.

How Has This Been Tested?

The implementation includes a comprehensive test suite that verifies:

  1. Basic table name generation functionality
  2. Precedence of explicit table names over generated names
  3. Default behavior when no generator is provided

Tests are implemented using the existing tortoise.contrib.test.TestCase class and follow the project's testing patterns. All tests pass in the current test environment.

Test scenarios cover:

Checklist:

coveralls commented 1 week ago

Pull Request Test Coverage Report for Build 11914995303

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Files with Coverage Reduction New Missed Lines %
tortoise/contrib/postgres/regex.py 2 75.0%
tortoise/init.py 2 95.76%
tortoise/contrib/pydantic/creator.py 2 98.91%
tortoise/router.py 4 68.42%
tortoise/expressions.py 7 97.25%
tortoise/validators.py 7 81.05%
tortoise/backends/base/executor.py 8 91.37%
tortoise/contrib/postgres/json_functions.py 10 35.71%
tortoise/queryset.py 16 94.37%
tortoise/contrib/mysql/json_functions.py 17 38.71%
<!-- Total: 111 -->
Totals Coverage Status
Change from base Build 11833519941: 0.4%
Covered Lines: 6235
Relevant Lines: 6855

💛 - Coveralls
abondar commented 1 week ago

I am okay with merging it that way - but is it really what users in original issue needed?

Probably would be a little bit easier for them, but looks like they wanted to have ability to override table name generation globally

Although not sure how to do it in clean way - may be make some base method to determine table, and it would by default take table name from meta, so users could override such method? Not a super fan of that idea, as it would go against current logic that such things are determined by model meta, but not sure of better ideas right now

Abdeldjalil-H commented 1 week ago

I agree that it is better to have a gloabal way to change the table names. One approach I can think about is to pass table_name_generator to Tortoise.init. What do you think about this?

abondar commented 1 week ago

I think that could work

Although then we would be back to problem of conflict between Meta.table_name and this init function, which could be confusing

@henadzit @waketzheng may be you have any good ideas how interface for that could look like?

Abdeldjalil-H commented 1 week ago

I think in that case it is not much confusing. The Meta.table will take precidence. It is like overriding an existing value.

abondar commented 1 week ago

I think in that case it is not much confusing. The Meta.table will take precidence. It is like overriding an existing value.

Yeah, I think you are right

Let's try to do it that way

waketzheng commented 1 week ago

Also, we can add an example to show generating snake_case_table_names global.

abondar commented 6 days ago

@Abdeldjalil-H are you planning to work on that in near future?

I was thinking if I should wait for it to be ready to include in 0.22.0 release

henadzit commented 6 days ago

Hi! Sorry, I'm a bit late here.

A few thoughts/questions from me:

Abdeldjalil-H commented 6 days ago

Hi @henadzit. Thank you for your questions.

  1. I think it should be. As far as I know, aerich uses db_table value which is set either from Meta.table or uses the default lower case if not provided. This PR just added the option to use any function for default instead of lowercased model name.
  2. Yes. The issues #1026 mentions one. Also I think sqlalchemy has the option __tablename__ as a function.
  3. This is a good point, I prefer Meta class appoach me too, but the problem this class is not inherited. We can use different patterns even with this approach for example
from tortoise import Model

class ModelBaseA(Model):
    class Meta:
        abstract = True

class ModelBaseB(Model):
    class Meta:
        abstract = True

def generate_table_name(model_cls):
    if isinstance(model_cls, ModelBaseA):
        # first pattern ...
    if isinstance(model_cls, ModelBaseB):
        # second pattern ...

await Tortoise.init(..., table_name_generator=generate_table_name)

another idea is to add table_name_generator to configs.apps, so each app will have its name generator. We may also use both the global one and app level one, so we can set a general default and a custom one.

Please let me know what do you think?

henadzit commented 6 days ago

Hey @Abdeldjalil-H, again, sorry for a late feedback!

  1. It would be great if you can verify that it doesn't break aerich! I think it shouldn't but you never know.
  2. Thanks for checking!
  3. @Abdeldjalil-H @abondar, what do you think about the following approach that would allow for more flexibility, e.g. having different naming patterns for different model groups?

class PrefixMeta:
    @staticmethod
    def table(model):
        return "prefix_" + model.__name__.lower()

class User(Model):
    class Meta(PrefixMeta):
        pass

    id = fields.IntField(pk=True)

This would require adding a callable check in tortoise/__init__.py:

                if callable(model._meta.db_table):
                    model._meta.db_table = model._meta.db_table(model)
                elif not model._meta.db_table:
                    model._meta.db_table = model.__name__.lower()

I'm not a fan of generate_table_name checking isintance and deciding on naming since you have to make it aware of all the models. This might be an issue for large organizations.

Abdeldjalil-H commented 6 days ago

@henadzit I'll check for aerich. The solution you proposed definitely works, but you need to inherit from PrefixMeta on each all models. Of course this is better than defining custom table name for all tables, but I think we can do better. Your solution inspired me about the following solution

from tortoise.models import Model, ModelMeta

class PrefixTableNameMeta(ModelMeta):
    def __new__(mcs, name: str, *args):
        new_class = super().__new__(mcs, name, *args)
        new_class._meta.db_table = "prefix_" + name.lower()
        return new_class

class User(Model, metaclass=PrefixTableNameMeta):
    id = fields.IntField(pk=True)

While this solution will work without the need of any changes in the original code base, it is not user-friendly. I think the simplest solution we can get is to define something that has the following criterias:

  1. Easy to define and use by all users.
  2. Easily inheritable/overridable
  3. Minimum repetition in the code

One example that satisfies this is an optional function on the model itself (not meta). We can call it get_table_name or anything.