pallets-eco / flask-session

Server side session extension for Flask
https://flask-session.readthedocs.io
BSD 3-Clause "New" or "Revised" License
488 stars 236 forks source link

Changelog for Flask-session 0.6.0 missing #210

Closed potiuk closed 3 months ago

potiuk commented 5 months ago

Flask 0.6.0 broke Airflow installation (which is not a huge issue as we use constraints and recommend our users to use them). Temporarily - for the upcoming version we also added < 0.6.0 limit for Flask-Session, but we would like to be able to support 0.6.0 and beyond, but seems that we miss changelog:

It's not present in any of those:

We track this in Airflow here https://github.com/apache/airflow/issues/36897 - would be great to get it out so that we can add support for it

Lxstr commented 5 months ago

HI @potiuk thanks for the issue. Apologies for missing the rc1 at the end of the version number as I intended only a release candidate as there have been a lot of changes There is a change log here https://github.com/pallets-eco/flask-session/blob/development/CHANGES.rst and the updated docs are also there, just not published yet.

        config.setdefault("SESSION_ID_LENGTH", 32)
        # SQLAlchemy settings
        config.setdefault("SESSION_SQLALCHEMY", None)
        config.setdefault("SESSION_SQLALCHEMY_TABLE", "sessions")
        config.setdefault("SESSION_SQLALCHEMY_SEQUENCE", None)
        config.setdefault("SESSION_SQLALCHEMY_SCHEMA", None)
        config.setdefault("SESSION_SQLALCHEMY_BIND_KEY", None)

You rightly noted in the airflow issue that the parameters are now all mandatory for extending the session interfaces. I did remove the parameter defaults in the class definitions for maintainability so that the defaults are not being defined twice in different places in our source code. There may be a more elegant way to do this, let me know if you have any input.

Taking a quick look at your code it seems as though you may be attempting to fix a historical flask-session issue where sessions were created before having added anything to the session (in the case of permanent session which is default in flask-session) such as #80. In 0.6.0 an empty session should now not be set. If that is what you were trying to fix, are you able to check if that is no longer required?

potiuk commented 5 months ago

HI @potiuk thanks for the issue. Apologies for missing the rc1 at the end of the version number as I intended only a release candidate as there have been a lot of changes There is a change log here https://github.com/pallets-eco/flask-session/blob/development/CHANGES.rst and the updated docs are also there, just not published yet.

Just for your information - PyPI will not let you re-publish the 0.6.0 version - PyPI is write-once-only. If you published 0.6.0 then you won't be able to publish updated 0.6.0 again. So if you think you will need a new rc, it will have to be 0.6.1, and I'd recommend you to yank the 0.6.0 if you know it is problematic. Yanking will also prevent users to install 0.6.0 by simply pip install apache-airflow or any other application using flask session also having similar problems.

You rightly noted in the airflow issue that the parameters are now all mandatory for extending the session interfaces. I did remove the parameter defaults in the class definitions for maintainability so that the defaults are not being defined twice in different places in our source code. There may be a more elegant way to do this, let me know if you have any input.

The problem with it is that it is - by-definition - backwards incompatible if someone creates the sessions manually and skips those default parameters. Of course it's debatable whether you "intended" fort those sessions to be created outside and leave those parameters unset, but I'd say they were there as defaults,so it was intention to get those values set as defaults.

Easy way will be to simply

DEFAULT_USE_SIGNER = False
DEFAULT_PERMANENT = True
DEFAULT_SID_LENGTH = ....

And use them in all your __init__'s :

def __init__(self, cache_dir, threshold, mode, key_prefix,
                 use_signer=DEFAULT_USE_SIGNER, permanent=DEFAULT_PERMANENT, sid_length=DEFAULT_SID_LENGTH):

Sounds easy-enough and backwards-compatible.

Taking a quick look at your code it seems as though you may be attempting to fix a historical flask-session issue where sessions were created before having added anything to the session (in the case of permanent session which is default in flask-session) such as https://github.com/pallets-eco/flask-session/pull/80. In 0.6.0 an empty session should now not be set. If that is what you were trying to fix, are you able to check if that is no longer required?

Possibly @jedcunningham and @Taragolis might comment on that, but I think it was more than that - we really wanted to avoid any sessions being created at all for those API calls.

But it also does not matter, the bit of a problem that you have now that 0.6.0 introduced breaking change and all released Airflow versions will have this issue (unless our users would use constraints as we recommend them of course).

So my recommendation would be to:

Happy to test RC of 0.6.1 if you follow that route

Lxstr commented 3 months ago

I have gone with a 0.7.0rc1 as there were a few other changes I'd also been working on. There should be backward compatibility with the parameter defaults as you requested. However, I did change the path of the interface to flask_session.sqlalchemy.SqlAlchemySessionInterface because there was no way I could find to make that available at the top level without sacrificing maintainability and we are technically both below 1.0.0 and in beta lifecycle stage on pypi so I think I could justify it. I also noted in the docs that this is officially part of the API and should be stable and quickly move towards 1.0.0 pending your responses.

SQLAlchemy interface has been the most difficult to deal with for me as it requires a very different implementation to all of the other backends which are more or less the same.

Lxstr commented 3 months ago

Closing this in favour of https://github.com/apache/airflow/issues/36897