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

Compatibility problems with freezegun #4738

Closed
potiuk opened this issue Aug 4, 2022 · 5 comments
Closed

Compatibility problems with freezegun #4738

potiuk opened this issue Aug 4, 2022 · 5 comments

Comments

@potiuk
Copy link

potiuk commented Aug 4, 2022

Flask 2.2.* session expiry time handling has a difficult relationship with Freezegun + Pendulum + MySQL SQLAlchemy. Yeah I know the combination looks pretty strange, but they seem to have really difficult relation when they are all together :).

I know it's not a Flask isssue, but looking at how popular freezegun (and pendulum) are, this might be somethng you want to consider reverting, especialy that it should have no negative side effects.

You can see more details and discussion in apache/airflow#25511 (comment) but in short Flask 2.2 release started to break our tests in our "canary" tests in Airflow - and after deeper investigation it turned out (besides new deprecation warnings that are fine) that the session expiry handling does not work with certain combinations of FreezeGun and Pendulum datetime (and MySQL SQLAlchemy).

This boiled down to this change:

Flask 2.1.2:

    def get_expiration_time(
        self, app: "Flask", session: SessionMixin
    ) -> t.Optional[datetime]:
        if session.permanent:
            return datetime.utcnow() + app.permanent_session_lifetime
        return None

Flask 2.2.0:

    def get_expiration_time(
        self, app: "Flask", session: SessionMixin
    ) -> t.Optional[datetime]:
        if session.permanent:
            return datetime.now(timezone.utc) + app.permanent_session_lifetime
        return None

While looking as reasonable change, the problem is that when you use Pendulum datetime to freeze time, the datetime.now(timezone.utc) fails with ValueError: fromutc: dt.tzinfo is not self. This is a known (unsolved) Pendulum issue spulec/freezegun#411. And while I was able to fix it using "stdlib" datetime to freeze time, this resulted (only in MySQL - it was fine for Postgres and SQLite) that the session could not be saved to the DB because. FakeDatetime expiry date coerced to '2020-08-06 09:00:00+00:00' (which is invalid date ). This is again a freezegun problem (both stdlib datetime,and pendulum datetime and even frozen by freezegun pendulum datetime work fine with MySQL Sqlalchemy). All those cases coerce to '2020-08-06T09:00:00+00:00' which is isoformat and acceptable by MySQL regardless from the encoding/etc.).

Eventually we got rid of freezegun from the Flask tests (apache/airflow#25511) and our problem is solved but this might cause similar problems for others. And while this is not a Flask problem but combination of other problems in unrelated libraries, seems that Flask can solve it by coming back to "datetime.utcnow()" approach that worked perfectly well for all those combinations.

Unless there is another reason, maybe you can consider switching back to utcnow()? That might save quite some time for investigating similar problems to others who do not have similar canary tests as we have and do not realise they might have similar problems when they upgrade to Flask 2.2.

But also feel free to close that one as "not our problem" - because indeed - it is not yours.

Environment:

  • Python version: 3.7
  • Flask version: 2.2.0, 2.2.1
@potiuk
Copy link
Author

potiuk commented Aug 4, 2022

BTW. Happy to open PR if this is something you consider worthy :)

@davidism
Copy link
Member

davidism commented Aug 4, 2022

I don't plan to switch back to utcnow. Literally every other datetime uses timezone.utc, those were changed last year or earlier. If only this one is failing that makes it even more of a problem with freezgun, not us.

@davidism davidism closed this as not planned Won't fix, can't repro, duplicate, stale Aug 4, 2022
@potiuk
Copy link
Author

potiuk commented Aug 4, 2022

Fair!

@davidism
Copy link
Member

davidism commented Aug 4, 2022

You might also reconsider depending on Pendulum in tests, if anything that starts using timezones might suddenly fail. Or at least using a regular datetime in freezegun calls.

@potiuk
Copy link
Author

potiuk commented Aug 4, 2022

a regular datetime in freezegun calls.

That's what I did, but it failed MySQL SQLAlchemy datetime coercing :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants