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

Limit Flask to <2.3 in the wake of 2.2 breaking our tests #25511

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Aug 3, 2022

Flask 2.2 added a few deprecations and made a few changes that
made our tests stop working. Those were really test problems not
real application problems (there were no breaking changes in 2.2):

  • new deprecation warnings produced
  • Flask app cannot be reused in multiple tests
  • The way how session lifetime is calculated makes test fail
    if freezegun is frozen using Pendulum datetime rather than the
    stdlib ones

This is an early warning for the future as the deprecation
warnings make us aware that Flask 2.3 is breaking. So this PR
fixes the 2.2 compatibility but at the same time limits Flask to
< 2.3 with appropriate information when the limit can be removed.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk potiuk requested review from ashb and mik-laj as code owners August 3, 2022 16:00
@boring-cyborg boring-cyborg bot added area:dev-tools area:webserver Webserver related Issues labels Aug 3, 2022
@potiuk potiuk force-pushed the fix-flask2-2-problems branch from ed3341d to 2842ea8 Compare August 3, 2022 17:42
@potiuk potiuk requested a review from uranusjr August 3, 2022 17:42
Flask 2.2 added a few deprecations and made a few changes that
made our tests stop working. Those were really test problems not
real application problems (there were no breaking changes in 2.2):

* new deprecation warnings produced
* Flask app cannot be reused in multiple tests
* The way how session lifetime is calculated makes test fail
  if freezegun is frozen using Pendulum datetime rather than the
  stdlib ones

This is an early warning for the future as the deprecation
warnings make us aware that Flask 2.3 is breaking. So this PR
fixes the 2.2 compatibility but at the same time limits Flask to
< 2.3 with appropriate information when the limit can be removed.
@potiuk potiuk force-pushed the fix-flask2-2-problems branch from 2842ea8 to e8f5e68 Compare August 3, 2022 17:45
@potiuk
Copy link
Member Author

potiuk commented Aug 3, 2022

@ashb @uranusjr -> I need a second look here.

I think I solved the test problems introduced by Flask 2.2 - I tried to find a better way but actually it turned out that the only way I could make it work is by getting rid of Freezegun from two tests that failed on MySQL only.

This is rather weird but it is fully reproducible. There were two problems:

  1. In test_views_grid.py CURRENT_TIME was pendulum.DateTime. But that caused the problem described here: Error using stdlib datetime when frozen to pendulum datetime instance. spulec/freezegun#411
ValueError: fromutc: dt.tzinfo is not self

I "fixed" it (or so I thought) by changing CURRENT_TIME to stdlib's datetime (does not matter if I used it naive or timezone aware ). And it seemed to work.

Until I saw in https://github.com/apache/airflow/runs/7656424982?check_suite_focus=true#step:10:14266 that while Postgres/Sqlite were working, Two MySQL tests failed with:

tests/www/views/test_views_tasks.py::test_run_ignoring_deps_sets_queued_dttm: 
sqlalchemy.exc.OperationalError: (MySQLdb.OperationalError)
(1292, "Incorrect datetime value: '2020-08-06 09:00:00+00:00'
for column 'expiry' at row 1")
[SQL: UPDATE session SET data=%s, expiry=%s WHERE session.id = %s]

I traced it down to this change in Flask (sessions.py):

Flask 2.1.2:

    def get_expiration_time(
        self, app: "Flask", session: SessionMixin
    ) -> t.Optional[datetime]:
        """A helper method that returns an expiration date for the session
        or ``None`` if the session is linked to the browser session.  The
        default implementation returns now + the permanent session
        lifetime configured on the application.
        """
        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]:
        """A helper method that returns an expiration date for the session
        or ``None`` if the session is linked to the browser session.  The
        default implementation returns now + the permanent session
        lifetime configured on the application.
        """
        if session.permanent:
            return datetime.now(timezone.utc) + app.permanent_session_lifetime
        return None

Seems that this one change from datetime.utcnow() to datetime.now(timezone.utc) connected with FreezeGun and FakeDateTime apparently causes it to produce different results that either real datetime or pendulum datettime.

This is a little 🤯 , but apparently mixing Freezegun, Flask, Pendulum, Stdlib.datatime and MySQL SQLAlchemy is too much for Airflow to 🐻

🤣

I workarounded it with getting rid of the Freezegun from both tests (it's not really "nice" but seems to work fine).

But if you have other ideas, I would love to hear them :D

@potiuk
Copy link
Member Author

potiuk commented Aug 3, 2022

Yep. Seems that my "workarounds" worked :)

@potiuk
Copy link
Member Author

potiuk commented Aug 3, 2022

All green I will merge it to make main back 'green' and unblock @eladkal with #25426 - but I would love comments if you have them @ashb @uranusjr .

@potiuk potiuk merged commit 42ea000 into apache:main Aug 3, 2022
@potiuk potiuk deleted the fix-flask2-2-problems branch August 3, 2022 19:04
@uranusjr
Copy link
Member

uranusjr commented Aug 3, 2022

Patch in this PR looks good to me.

Seems that this one change from datetime.utcnow() to datetime.now(timezone.utc) connected with FreezeGun and FakeDateTime apparently causes it to produce different results that either real datetime or pendulum datettime.

Pendulum tries to be clever and returns a string form with a T separator (instead of a space used by datetime) to make the output. But it misses implementing now(timezone.utc) and returns a bare datetime (instead of a Pendulum DateTime) and clever abstraction crumbles down. This kind of little things caused me a lot of pain when I worked on the test case speedup a while ago.

@potiuk
Copy link
Member Author

potiuk commented Aug 4, 2022

Pendulum tries to be clever and returns a string form with a T separator (instead of a space used by datetime) to make the output. But it misses implementing now(timezone.utc) and returns a bare datetime (instead of a Pendulum DateTime) and clever abstraction crumbles down. This kind of little things caused me a lot of pain when I worked on the test case speedup a while ago.

Yeah. Some clever absractions are just .... too clever :)

@potiuk
Copy link
Member Author

potiuk commented Aug 4, 2022

BTW. I've opened an issue in Flask - maybe that is compelling enough case for them to revert that change (it should have no side-effects)

potiuk added a commit that referenced this pull request Aug 4, 2022
Flask 2.2 added a few deprecations and made a few changes that
made our tests stop working. Those were really test problems not
real application problems (there were no breaking changes in 2.2):

* new deprecation warnings produced
* Flask app cannot be reused in multiple tests
* The way how session lifetime is calculated makes test fail
  if freezegun is frozen using Pendulum datetime rather than the
  stdlib ones

This is an early warning for the future as the deprecation
warnings make us aware that Flask 2.3 is breaking. So this PR
fixes the 2.2 compatibility but at the same time limits Flask to
< 2.3 with appropriate information when the limit can be removed.

(cherry picked from commit 42ea000)
@potiuk potiuk added this to the Airflow 2.3.4 milestone Aug 5, 2022
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants