update flask sqlalchemy #158
Conversation
utnapischtim
force-pushed
the
update-flask-sqlalchemy
branch
2 times, most recently
from
8cee2ed
to
8968dc7
Compare
* it is not working anymore with localproxy and it seams not necessary
* use the proxy file instead of creating the _db variable in both files * this removes a DeprecationWarning from flask-sqlalchemy, which states that the support of the use '.db' will be removed
utnapischtim
force-pushed
the
update-flask-sqlalchemy
branch
from
8968dc7
to
7ea66b2
Compare
* the error indicates that db.init_app is not called, but it is and it works in other contexts. It may that the `db = invenio_db = shared.db` line in conftest.py is not working as expected anymore. The only found solution is this. The drawback is, if that is the only solution it has to be done also in the other packages which are using this function. With the default value it is backward compatible but as the tests indicates it may not work without adding the db parameter to the function call
* added D202, pydocstyle and black do not habe the same opinion * remove warning by using StringEncryptedType instead of EncryptedType. This removes a warning and there is further a notice in the code that the base type of EncryptedType will change in the future and it is better to replace it with StringEncryptedType
utnapischtim
force-pushed
the
update-flask-sqlalchemy
branch
from
c141b55
to
cdf6d5d
Compare
utnapischtim
force-pushed
the
update-flask-sqlalchemy
branch
from
cdcc543
to
7a7a4f6
Compare
* the problem is, that assert calls an function which leads to an not moving forward test on mysql8. it works on postgresql and mysql5 but not mysql8.
|
@ntarocco How should we proceed with this? The change of the signature of the |
| @@ -33,7 +33,8 @@ class Demo(db.Model): | |||
| __tablename__ = "demo" | |||
| pk = db.Column(sa.Integer, primary_key=True) | |||
| et = db.Column( | |||
| EncryptedType(type_in=db.Unicode, key=_secret_key), nullable=False | |||
| StringEncryptedType(length=255, type_in=db.Unicode, key=_secret_key), | |||
There was a problem hiding this comment.
It should not be an issue, but I cannot find where the length params comes from in StringEncryptedType. Is it necessary given that it was not there earlier?
There was a problem hiding this comment.
The problem is VARCHAR on mysql. Otherwise the tests are not working. Which leads me to a question: how long do we support mysql?
There was a problem hiding this comment.
I think that we should support MySQL if it does not create great issues, otherwise we might need an ORM layer at all :) There could be organization that are required to use MySQL for example.
invenio_db/utils.py
Outdated
| from .proxies import current_db | ||
| from .shared import db as _db |
There was a problem hiding this comment.
I think this was changed in the wrong way?
The previous db should be the same, no change. The previous _db, should be the current_db:
| from .proxies import current_db | |
| from .shared import db as _db | |
| from .proxies import current_db as _db | |
| from .shared import db |
There was a problem hiding this comment.
here i am a little bit confused, because the from .shared import db as _db was your suggestion. The drop_alembic_version_table function uses current_db. If we change current_db to _db we would have to change that too in the drop_alembic_version_table function. Further, as i understand the two usages should not be the same because in rebuild_encrypted_properties the db imported from shared.py was used and not the _db from the LocalProxy
There was a problem hiding this comment.
I am sorry if I made the wrong suggestion.
What I noticed now is that we have switched the imports, which might lead to issues.
Nothing should be changed, given that the imports will be exactly the same as before the changes. The only needed change in this file is the extra param to rebuild_encrypted_properties
There was a problem hiding this comment.
yes, but the new parameter has to come from .shared not from .proxies. they are not the same. but i will investigate a little bit more into that problem.
It looks like that there are tests there, so we can run the tests and see if they pass. If there is a change in the future, the tests should not pass. |
|
Coordination: This looks like it can be merged: tests are passing and maintainer has approved. I don't know that another outside reviewer could review this well. If @ntarocco wants to merge, go ahead. |
* the new name better reflects the intended use
|
@fenekku thanks, we are still chatting offline on these changes. I would also test these changes before merging. Almost ready :) |
This reverts commit 494bef9.
NOTE: the bugfix has to be discussed before merging!
if we use the change then following two lines have to be updated:
invenio_oauth2server.utils
invenio_oauthclient.utils