Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: expose timezone and use_tz params in RegisterTortoise class #1632

Closed
Abeautifulsnow opened this issue May 27, 2024 · 5 comments
Closed

Comments

@Abeautifulsnow
Copy link
Contributor

Is your feature request related to a problem? Please describe.
I use mysql in my project and the I use db_url and RegisterTortoise to initialize the database connection. And then when I execute create on my Model, the new record is inserted into database, while the update_time is 8 hours behind the current time. That means that I need to set my timezone to Asia/Shanghai and use_tz to False cause I am in East 8 TimeZone.

But unfortunately I don't find the appropriate way to do that. I try to add timezone in db_url string, like this: mysql://root:123456@192.100.30.102:3306/energy_dispatch?charset=utf8&timezone=Asia/Shanghai, and the code is reported an error:

File "asyncmy/pool.pyx", line 216, in _create_pool                                                                                                                         
File "asyncmy/pool.pyx", line 217, in asyncmy.pool._create_pool                                                                                                            
File "asyncmy/pool.pyx", line 145, in fill_free_pool                                                                                                                       
File "asyncmy/connection.pyx", line 1278, in asyncmy.connection.connect                                                                                                  
TypeError: connect() got an unexpected keyword argument 'timezone'

I dig into the source code, and I do some changes here:

async def init_orm(self) -> None:  # pylint: disable=W0612
    config, config_file = self.config, self.config_file
    db_url, modules = self.db_url, self.modules
    await Tortoise.init(
        config=config,
        config_file=config_file,
        db_url=db_url,
        modules=modules,
        use_tz=False,    # --------------- here
        timezone="Asia/Shanghai",    # --------------------here
    )
    logger.info("Tortoise-ORM started, %s, %s", connections._get_storage(), Tortoise.apps)
    if self.generate_schemas:
        logger.info("Tortoise-ORM generating schema")
        await Tortoise.generate_schemas()

after that, I tried again and the time in db is correct now.

image

Describe the solution you'd like

I'm not sure this is a good choice to expose timezone and use_tz params to RegisterTortoise class.

class RegisterTortoise(AbstractAsyncContextManager):
    def __init__(
        self,
        app: FastAPI,
        config: Optional[dict] = None,
        config_file: Optional[str] = None,
        db_url: Optional[str] = None,
        modules: Optional[Dict[str, Iterable[Union[str, ModuleType]]]] = None,
        generate_schemas: bool = False,
        add_exception_handlers: bool = False,
        use_tz: bool = False,
        timezone: str = "UTC",
    ) -> None:
        self.app = app
        self.config = config
        self.config_file = config_file
        self.db_url = db_url
        self.modules = modules
        self.generate_schemas = generate_schemas
        self.use_tz = use_tz  # ------------here
        self.timezone = timezone  # ------------here

        if add_exception_handlers:

            @app.exception_handler(DoesNotExist)
            async def doesnotexist_exception_handler(request: "Request", exc: DoesNotExist):
                return JSONResponse(status_code=404, content={"detail": str(exc)})

            @app.exception_handler(IntegrityError)
            async def integrityerror_exception_handler(request: "Request", exc: IntegrityError):
                return JSONResponse(
                    status_code=422,
                    content={"detail": [{"loc": [], "msg": str(exc), "type": "IntegrityError"}]},
                )

    async def init_orm(self) -> None:  # pylint: disable=W0612
        config, config_file = self.config, self.config_file
        db_url, modules = self.db_url, self.modules
        await Tortoise.init(
            config=config,
            config_file=config_file,
            db_url=db_url,
            modules=modules,
            use_tz=self.use_tz,  # ------------here
            timezone=self.timezone,  # ------------here
        )
        logger.info("Tortoise-ORM started, %s, %s", connections._get_storage(), Tortoise.apps)
        if self.generate_schemas:
            logger.info("Tortoise-ORM generating schema")
            await Tortoise.generate_schemas()

    @staticmethod
    async def close_orm() -> None:  # pylint: disable=W0612
        await connections.close_all()
        logger.info("Tortoise-ORM shutdown")

    def __call__(self, *args, **kwargs) -> "RegisterTortoise":
        return self

    async def __aenter__(self) -> "RegisterTortoise":
        await self.init_orm()
        return self

    async def __aexit__(self, *args, **kw):
        await self.close_orm()

and I surely want to know If the db_url and config and config_file is identical each other?

if int(bool(config) + bool(config_file) + bool(db_url)) != 1:
        raise ConfigurationError(
            'You should init either from "config", "config_file" or "db_url"'
        )

If I select db_url to initialize my connection and how to config the timezone and use_tz and others, etc.
I'd be appreciate if anyone could help clarify these confusions.

@Abeautifulsnow
Copy link
Contributor Author

Can someone answer this question? Or point out if there is something unreasonable?

@abondar
Copy link
Member

abondar commented Jun 11, 2024

Hi!

I see no problem with exposing timezone params in RegisterTortoise
Not sure why it wasn't added in the first place, probably contributor just forgot about it

You can contribute by making PR with this change

and I surely want to know If the db_url and config and config_file is identical each other?

Don't really understand issue here, this code just makes sure that you use only one of init methods, as we wouldn't know how to parse it if several are provided at the same time

@Abeautifulsnow
Copy link
Contributor Author

Abeautifulsnow commented Jun 11, 2024

Don't really understand issue here, this code just makes sure that you use only one of init methods, as we wouldn't know how to parse it if several are provided at the same time

My bad, I just don't know why adding timezone to db_url is causing an error, and I'm confused about whether all three are the same.

You can contribute by making PR with this change

Alright, I will create a new pull request for adding time_zone and use_tz in RegisterTortoise .

@waketzheng
Copy link
Contributor

Currently, you can use the config argument:

@asynccontextmanager
async def lifespan(app: FastAPI):
    async with RegisterTortoise(
        app,
        config={
            "connections": {"default": DB_URL},
            "apps": {"models": {"models": ["models"]}},
            "use_tz": True,
            "timezone": "Asia/Shanghai",
            "generate_schemas": True,
        },
    ):
        yield

@Abeautifulsnow
Copy link
Contributor Author

The feature has been extended in pr #1649

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants