[Proposal] unpin flask >= 3.0.0

[utnapischtim]

Motivation

remove a lot of deprecation warnings

Summary

migrate to flask>=3.0.0

open discussion points

pytest-invenio

• Problem
  • the way how pytest-invenio.fixtures creates a db fixture doesn't work anymore. the reason is that flask-sqlalchemy has changed how the get_bind method returns the binding. this means that this has no effect this leads to the problem that in sqlalchemy.Session the should_commit is always true and this means that the transaction will be commited here which is then a problem, because the session.rollback couldn't rollback what is already stored in the database.
  • the new way doesn't work either because it needs this commit because otherwise the transaction parent here of the session.rollback will also be rolledback which removes to much! and the reason why this change doesn't work is because the flask-sqlalchemy.session has no _transaction property.

itsdangerous.jws

• Problem:
  • https://github.com/pallets/itsdangerous/blob/2.0.1/src/itsdangerous/jws.py has been removed with version 2.1.0
• Solution:
  • copy-pasted into invenio-base: inveniosoftware/invenio-base@931a013
• alternative solution:
  • rewrite functionality of jws with authlib library, as suggested by the deprecation warning https://github.com/pallets/itsdangerous/blob/2.0.1/src/itsdangerous/jws.py#L54
• open questions:
  • is copy-paste a feasible approach?
  • how to solve the license question to incorporate the BSD-3-clause license code into MIT licensed package

https://github.com/lepture/flask-oauthlib

• Problem:
  • the library is a dependency which is deprecated and should not used anymore
  • but the library has to be updated otherwise we can't migrate to flask>=3.0.0
• Solution:
  • clone the library to inveniosoftware
  • and fix the open problems (NOTE: done the work locally to make it work locally, but not created any commits or PR without decision how to solve the problem)
• alternative solution:
  • refactore the code in inveniosoftware by using the suggested https://github.com/lepture/authlib library

unpin various dependencies

• Problem:
  • to upgrade Flask some other package have to be upgraded too
• Solution:
  • unpin the packages
  • flask, werkzeug, itsdangerous, wtforms, oauthlib, request-oauthlib, webargs, flask-sqlalchemy, sqlalchemy, sqlalchemy-continuum, sqlalchemy-utils, alembic, flask-alembic
• alternative solution:
  • don't migrate to flask>=3.0.0

versioned table set to True instead of False invenio-banners

• Problem:
  • remove SAWarnings like:
    SAWarning: On mapper Mapper[RDMFileRecordMetadataVersion(rdm_records_files_version)], primary key column 'rdm_records_files_version.transaction_id' is being combined with distinct primary key column 'rdm_records_files_version.transaction_id' in attribute 'transaction_id'. Use explicit properties to give each column its own mapped attribute name. (This warning originated from the configure_mappers() process, which was invoked automatically in response to a user-initiated operation.)
    return cls.query_class(
• Solution:
  • set versioned to True in invenio-banners see commit
• alternative solutions:
  • stay with SAWarnings
  • find out that the warnings are hiding some other problem
  • find out the real problem (probably time consuming)
• !!!Solved!!!:
  • see commit inveniosoftware/invenio-banners@5921711

invenio-banners not clear how test could ever worked

• Problem:
  • two tests are expecting different things
  • the test_create_banner needs a banner_1 which has enddate in the future so that active == active because otherwise the disable_expired call would set active to False.
  • but the test_disable_expired_after_create_action needs banner_1 in the past (relative to now) because otherwise disable_expired will not set banner_1 active to False
• Solution:
  • introduce banner_4 which fulfills the second case (BUT why has this ever worked?)
• alternative Solution:
  • find out why this ever worked
• !!!Solved!!!:
  • inveniosoftware/invenio-banners@4aa1c3d

invenio-banners stored datetime format

• have a look to inveniosoftware/invenio-banners@eeb91b7

invenio-db

• Problem:
  • check every commit if it introduces problems
• Note:
  • tests are green with the invenio-base PR

invenio-vocabularies

• Problem
  • some tests are not easy migrate to sqlalchemy >= 2.0
• Note:
  • remove flask upper pin invenio-vocabularies#411

PR's

open

inveniosoftware/flask-iiif#102
inveniosoftware/flask-kvsession#8
inveniosoftware/flask-menu#89
inveniosoftware/flask-security-fork#63

inveniosoftware/invenio-access#208
inveniosoftware/invenio-app-rdm#2856
inveniosoftware/invenio-banners#35
inveniosoftware/invenio-base#182
inveniosoftware/invenio-db#174
inveniosoftware/invenio-files-rest#311
inveniosoftware/invenio-github#174
inveniosoftware/invenio-oauth2server#265
inveniosoftware/invenio-oauthclient#336
inveniosoftware/invenio-rest#139

sqlalchemy compatibility PR's which are not included above

inveniosoftware/invenio-records-resources#597
inveniosoftware/invenio-records#326
inveniosoftware/invenio-pidstore#158

already finished and released packages

• https://github.com/inveniosoftware/invenio-jobs/commits/master/
• https://github.com/inveniosoftware/invenio-pages/commits/master/

waiting for release

• https://github.com/inveniosoftware/invenio-rdm-records/commits/master/

some more detailed information of the PR's

inveniosoftware/invenio-base#182

since itsdangerous >= 2.1.0 has removed the jws functionality i copy pasted the code into invenio-base to use it as it is. i have removed the deprecation warning from the code. further i added a copyright notice to reference where the code comes from. the open question is: is this enough? the itsdangerous is licensed under BSD-3-Clause license

the license unproblematic way would be to reimplement it with the suggested Authlib but that would take more time

NOTE: i pinned watchdog to ==2.3.1. the newest version has a problem with reloading on http requests which i didn't understand

inveniosoftware/invenio-banners#35

• flask-sqlalchemy.pagination
• inveniosoftware/invenio-banners@02a2f85
  • see SAWarnings at the end of this RFC

inveniosoftware/invenio-db#174

go through all commits of this PR

unpin deps

• setup: remove flask upper pin invenio-github#174
  • remove flask,werkzeug upper pins
• remove flask upper pin invenio-oauth2server#265
  • unpin wtforms
• remove flask upper pin invenio-oauthclient#336
  • oauthlib, request-oauthlib
• WIP invenio-rest#139
  • webargs

forward compatibility with flask,werkzeug >= 3.0.0.

• setup: remove werkzeug pin flask-iiif#102
  • like moving to urllib.parse imports
• fix: compatibility with flask >= 3.0.0 flask-kvsession#8
  • cookie_data.decode("utf-8")
• setup: remove flask upper pin flask-menu#89
  • remove decorator because of before_first_request decorator
• remove flask upper pin flask-security-fork#63
  • use markupsafe for Markup import instead of flask
• fix: compatibility with werkzeug >= 3.0.0. invenio-files-rest#311
  • copy pasted code from werkzeug.formparser
• remove flask upper pin invenio-oauth2server#265
  • like moving to urllib.parse imports
• remove flask upper pin invenio-oauthclient#336
  • use markupsafe for Markup import instead of flask

invenio-admin

• global: remove invenio-admin invenio-access#208
  • make invenio-admin install optional
• global: remove invenio-admin invenio-app-rdm#2628
  • remove invenio-admin completely

sqlalchemy changed api

• remove flask upper pin invenio-app-rdm#2856

waiting for release

invenio-rdm-records

external libraries which seem out of support

https://github.com/pallets-eco/flask-caching

• https://github.com/lepture/flask-oauthlib
  • on the github page it states itself that lepture/oauthlib should be used instead, but it doesn't seem a drop in replacement
  • changes: remove upper pin for oauthlib and requests-oauthlib (dependency resolution not possible)
  • use urllib.parse imports instead of the werkzeug.url ones. this would make the invenio_oauth2server._compat obsolet (at least the part with the werkzeug.url overwrites)

SAWarnings which appear if inveniosoftware/invenio-banners@02a2f85 is kept the old way. to be explicit __versioned__ = {"versioning": False} produces following SAWarnings

• SAWarning: On mapper Mapper[RDMFileRecordMetadataVersion(rdm_records_files_version)], primary key column 'rdm_records_files_version.transaction_id' is being combined with distinct primary key column 'rdm_records_files_version.transaction_id' in attribute 'transaction_id'. Use explicit properties to give each column its own mapped attribute name. (This warning originated from the configure_mappers() process, which was invoked automatically in response to a user-initiated operation.)
  return cls.query_class(

• SAWarning: relationship 'RDMMediaFileRecordMetadataVersion.transaction' will copy column transaction.id to column rdm_records_media_files_version.transaction_id, which conflicts with relationship(s): 'RDMMediaFileRecordMetadataVersion.transaction' (copies transaction.id to rdm_records_media_files_version.transaction_id). If this is not the intention, consider if these relationships should be linked with back_populates, or if viewonly=True should be applied to one or more if they are read-only. For the less common case that foreign key constraints are partially overlapping, the orm.foreign() annotation can be used to isolate the columns that should be written towards. To silence this warning, add the parameter 'overlaps="transaction"' to the 'RDMMediaFileRecordMetadataVersion.transaction' relationship. (Background on this warning at: https://sqlalche.me/e/20/qzyx) (This warning originated from the configure_mappers() process, which was invoked automatically in response to a user-initiated operation.)
  return cls.query_class(

• SAWarning: relationship 'PageListVersion.transaction' will copy column transaction.id to column pages_pagelist_version.transaction_id, which conflicts with relationship(s): 'PageListVersion.transaction' (copies transaction.id to pages_pagelist_version.transaction_id). If this is not the intention, consider if these relationships should be linked with back_populates, or if viewonly=True should be applied to one or more if they are read-only. For the less common case that foreign key constraints are partially overlapping, the orm.foreign() annotation can be used to isolate the columns that should be written towards. To silence this warning, add the parameter 'overlaps="transaction"' to the 'PageListVersion.transaction' relationship. (Background on this warning at: https://sqlalche.me/e/20/qzyx) (This warning originated from the configure_mappers() process, which was invoked automatically in response to a user-initiated operation.)
  return cls.query_class(

• SAWarning: On mapper Mapper[PageModelVersion(pages_page_version)], primary key column 'pages_page_version.transaction_id' is being combined with distinct primary key column 'pages_page_version.transaction_id' in attribute 'transaction_id'. Use explicit properties to give each column its own mapped attribute name. (This warning originated from the configure_mappers() process, which was invoked automatically in response to a user-initiated operation.)
  return cls.query_class(

• flask_sqlalchemy/model.py:144: SAWarning: This declarative base already contains a class with the same class name and module name as sqlalchemy_continuum.model_builder.RDMRecordMetadataVersion, and will be replaced in the string-lookup table. (multiple of those with different *Version tables)
  super().init(name, bases, d, **kwargs)

additional fixed deprecation warnings after sqlalchemy migration

• fix: LegacyAPIWarning of sqlalchemy invenio-users-resources#145

biggest problem and showstopper. it is solved!

ERROR tests/resources/test_resources.py::test_update_non_existing_banner - sqlalchemy.exc.IntegrityError: (psycopg2.errors.UniqueViolation) duplicate key value violates unique constraint "uq_accounts_role_name"

or better said the problem with transaction.rollback() not working as expected. it seems that there are some autoflushes somewhere in the code. it seems that session.begin_nested() flushes whats added to the transaction to have a clean savepoint. which is not good because then the transaction.rollback() in the db fixture can't do what it is supposed to do!

i tested with a custom session object:

    connection = database.engine.connect()
    transaction = connection.begin()
    options = dict(
        bind=connection,
        binds={},
        class_=FlaskSqlalchemySession,
        # class_=NoFlushSession,
        query_cls=database.Query,
        join_transaction_mode="control_fully",
        expire_on_commit=False,
        autocommit=False,
        autoflush=False,
        **db_session_options,
    )
    Session = sessionmaker(db=database, **options)
    session = Session(
        bind=connection,
        join_transaction_mode="control_fully",
        expire_on_commit=False,
        autocommit=False,
        autoflush=False,
    )
    old_session = database.session
    database.session = session
    yield database
    session.close()
    transaction.rollback()
    connection.close()
    database.session = old_session

it does not what you think. it doesn't revert the transaction. it seems that the changes commited are written to the database and therefore can't be rollback.

ATTENTION: it seems i fixed that problem with inveniosoftware/pytest-invenio@6f04238 and applied in inveniosoftware/invenio-banners@8da0d89 . ok, since the tests are green now on invenio-banners this change works also with the old pytest-invenio fixture code