Conversation
c0620a6 to
dce938a
Compare
54477dc to
5ce4949
Compare
providers/fab/src/airflow/providers/fab/auth_manager/cli_commands/utils.py
Outdated
Show resolved
Hide resolved
providers/fab/src/airflow/providers/fab/www/extensions/init_appbuilder.py
Outdated
Show resolved
Hide resolved
providers/fab/src/airflow/providers/fab/www/extensions/init_session.py
Outdated
Show resolved
Hide resolved
d4699d4 to
74be7ec
Compare
70af44c to
7f7611d
Compare
|
All tests are green 🟢 🥳 After a bit of a battle I could successfully use the official |
|
Please see a plan for adding dual SQLA 1.4/2.0 compatibility drafted based on the observed mypy errors (with the help of gemini and chatgpt): Note that a small part of the above already exists in the codebase from previous attemps of adding SQLA2 compat. |
|
@eladkal . This PR means the next FAB provider release needs to be a major version release. Is there anything I can do in this PR to help you? (Updating the changelog?) |
Add main entry to the changelog describing steps users needs to migrate. |
eladkal
left a comment
There was a problem hiding this comment.
Overall LGTM but needs more reviewers on this one
Great work!
|
I found out a bug, I'll work on it |
|
Bug fixed |
Switch to using flask-sqlalchemy db session management, and include Auth Manager Provider Test Isolation and Reliability - Refactor test fixtures to always use Flask app contexts for DB and app operations. - Add a global pytest fixture to clear SQLAlchemy metadata before each test, reducing test flakiness. - Standardize session access and cleanup patterns across all tests. - Refactor user/role creation and deletion to ensure proper isolation. - Update test logic to use new app and auth manager creation utilities. - Remove or update redundant or fragile test code. - Normalize test constants and improve code readability. Note: Clearing SQLAlchemy metadata and explicit app context management are workarounds for test isolation issues. The root cause of metadata and pool persistence between tests should be investigated further for a more robust solution. General Theme: The main strategies are: * Ensuring proper use of Flask app contexts in tests * Cleaning up database state and metadata between tests * Using the correct SQLAlchemy session and interface patterns * Refactoring test fixtures for better isolation and reliability * Removing or updating code that is no longer needed or that could cause test flakiness Key File-by-File Changes 1. providers/fab/tests/unit/fab/conftest.py (New file) What: Adds a global pytest fixture that clears SQLAlchemy metadata before each test. Why: This is to prevent metadata leakage between tests, which can cause flaky or non-deterministic test results. Uncertainty: Clearing metadata is a workaround; the root cause of metadata persistence between tests may need deeper investigation. 2. Test files for API endpoints, CLI commands, models, schemas, and views What: * Many test fixtures now use with app.app_context(): to ensure all DB and app operations are performed within a Flask application context. * User and role creation/deletion is now always wrapped in an app context. * Some teardown logic is moved into the fixture's context manager to ensure cleanup happens after the test. * Session access is standardized to use appbuilder.session instead of get_session. * Some test constants (e.g., default times) are normalized (e.g., removing timezone info). * Some test logic is refactored for clarity and reliability (e.g., using addfinalizer for logout in user endpoint tests). Why: Ensures that tests do not leak state or context, which can cause failures when running tests in parallel or in different environments. Using the correct session and context patterns is more robust and future-proof. Uncertainty: While these changes improve test isolation, the need to clear metadata and manage app contexts so explicitly suggests there may be deeper issues with how the test environment is set up or torn down. Further investigation into the test infrastructure may be warranted. 3. test_fab_auth_manager.py and related files What: * Switches to using the new create_app and get_auth_manager utilities for creating Flask apps and auth managers. * Updates test logic to use the new app and session patterns. * Fixes some test assertions to compare user IDs instead of user objects directly. * Moves some permission synchronization logic to after DAG creation and session commit. Why: These changes align the tests with the latest best practices and APIs in Airflow and Flask AppBuilder. They also fix subtle bugs where tests could pass or fail depending on object identity rather than value. Uncertainty: The need to manually commit and close sessions, and to synchronize permissions, may indicate that the test setup/teardown is not fully robust. 4. test_security.py What: * Removes some unused or redundant code (e.g., a test for DAG permission views). * Updates session and app context usage. Why: Cleans up the test suite and ensures all tests are using the correct patterns. Uncertainty: The removal of some tests may need to be reviewed to ensure no loss of coverage. 5. Miscellaneous What: * Minor formatting and import cleanups. * Some test parameters and constants are updated for consistency. Why: Improves code readability and maintainability. Summary of Uncertainties and Next Steps * Clearing SQLAlchemy metadata and disposing pools: These are workarounds for test isolation issues. The root cause (why metadata and pools persist between tests) should be investigated further. Ideally, the test infrastructure should handle this automatically. App context management: The need for explicit app contexts in so many places may indicate that the test setup could be improved to provide a more consistent environment. Session and teardown logic: Manual session management and teardown in tests can be error-prone. Consider centralizing this logic or using more robust fixtures. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@vincbeck I think we can merge. We can always fix stuff later |
|
Though I am not sure about the core related code. Is the core parts planned to be released in 3.2 or 3.1.1 cc @kaxil |
I re-reviewed the code and I am merging it to unblock it (still on holidays - just wanted to unblock it). Yes it should be working with Airflow 3.0 as well. The changes in "core" are just in "default config" - removal of OID auth from it (which should not be used by anyone anyway already - this is an old OAUTH 1.0 protocol that no-one should even be able to use any more as it's been removed by all OAUTH providers years ago. All the rest is in tests, so no need to release anything in 3.1 or 3.2 with it - the defailt OID is "OK" to be there in 3.1 as no-one is using it anyway. I think that default_webserver config should be removed anyway (?). |
|
🎉 🥳 |
Absolutely!
No we need to keep it, it is still used in Fab provider here. And I actually remember adding it because some users were complaining it was breaking their setup. See #48898. |
Switch to using flask-sqlalchemy db session management, and include Auth
Manager Provider Test Isolation and Reliability
operations.
test, reducing test flakiness.
Note: Clearing SQLAlchemy metadata and explicit app context management
are workarounds for test isolation issues. The root cause of metadata
and pool persistence between tests should be investigated further for a
more robust solution.
General Theme:
The main strategies are:
test flakiness
Key File-by-File Changes
What: Adds a global pytest fixture that clears SQLAlchemy metadata
before each test.
Why: This is to prevent metadata leakage between tests, which can cause
flaky or non-deterministic test results.
Uncertainty: Clearing metadata is a workaround; the root cause of
metadata persistence between tests may need deeper investigation.
What:
and app operations are performed within a Flask application context.
context.
ensure cleanup happens after the test.
get_session.
removing timezone info).
addfinalizer for logout in user endpoint tests).
Why:
Ensures that tests do not leak state or context, which can cause
failures when running tests in parallel or in different environments.
Using the correct session and context patterns is more robust and
future-proof.
Uncertainty:
While these changes improve test isolation, the need to clear metadata
and manage app contexts so explicitly suggests there may be deeper
issues with how the test environment is set up or torn down. Further
investigation into the test infrastructure may be warranted.
What:
for creating Flask apps and auth managers.
directly.
session commit.
Why:
These changes align the tests with the latest best practices and APIs in
Airflow and Flask AppBuilder. They also fix subtle bugs where tests
could pass or fail depending on object identity rather than value.
Uncertainty:
The need to manually commit and close sessions, and to synchronize
permissions, may indicate that the test setup/teardown is not fully
robust.
What:
views).
Why:
Cleans up the test suite and ensures all tests are using the correct patterns.
Uncertainty:
The removal of some tests may need to be reviewed to ensure no loss of coverage.
What:
Why:
Improves code readability and maintainability.
Summary of Uncertainties and Next Steps
Clearing SQLAlchemy metadata and disposing pools:
These are workarounds for test isolation issues. The root cause (why
metadata and pools persist between tests) should be investigated
further. Ideally, the test infrastructure should handle this
automatically.
App context management:
The need for explicit app contexts in so many places may indicate that
the test setup could be improved to provide a more consistent
environment.
Session and teardown logic:
Manual session management and teardown in tests can be error-prone.
Consider centralizing this logic or using more robust fixtures.
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.