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.

Sign up for GitHub

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

Jump to bottom

remove flask upper pin #35

Merged
merged 12 commits into from
Nov 3, 2024
Merged

remove flask upper pin #35

merged 12 commits into from
Nov 3, 2024

Conversation

utnapischtim
Copy link
Contributor

  • fix: add compatibility layer to move to flask>=3
  • wip: fix versioning, but maybe should be fixed in invenio-db

@utnapischtim
fix: add compatibility layer to move to flask>=3
7450325 
* flask-sqlalchemy moved pagination.

* this change has been added to have a smooth migration to flask>=3.0.0
  without creating a hard cut and major versions release.
@utnapischtim utnapischtim mentioned this pull request Sep 26, 2024
Open
@utnapischtim utnapischtim force-pushed the remove-flask-upper-pin branch from 8da0d89 to 3cd34db Compare October 1, 2024 21:27
@utnapischtim
fix: SAWarning
5921711 
* SAWarning:
  "This declarative base already contains a class with the "
  "same class name and module name as %s.%s, and will "
  "be replaced in the string-lookup table."

* if a model should not be versioned the __versioned__ should not be
  there at all

* if it is there, it will be added to
  version_manager.pending_classes (which seems like a bug, because it is
  versioning set to false)

* those pending_classes will be checked against declared classes in
  invenio_db.ext and since the banner model should not have a versioned
  table it is not in the declared_classes list. this starts then a try
  of the version_manager to create the missing tables which causes then
  a duplicate error which is shown in the SAWarning
@utnapischtim
fix: SADeprecationWarning
fef01c8 
* SADeprecationWarning: Invoking or_() without arguments is deprecated,
  and will be disallowed in a future release.   For an empty or_()
  construct, use 'or_(false(), *args)' or 'or_(False, *args)'.
    BannerModel.query.filter(or_(*filters))

* sqlalchemy/sqlalchemy#5054
@utnapischtim
tests: move to reusable workflows
00f35b7
@utnapischtim
chore: fix black, isort
a2d6c4b
@utnapischtim utnapischtim force-pushed the remove-flask-upper-pin branch 2 times, most recently from 7e83403 to 8134cce Compare October 2, 2024 21:42
@utnapischtim utnapischtim marked this pull request as ready for review October 2, 2024 21:43
@utnapischtim
tests: make it run again
acf12be 
* not sure why it ever worked

* the requirement for test_create_banner and
  test_disable_expired_after_create_action can't be fulfilled at the
  same time.

* to create the  banner1 in test_create_banner the enddate has to be in
  the future otherwise the disable_expired service method kicks in and
  sets to active == False

* but in test_disable_expired_after_create_action banner1 should be in
  the past so that active expires by creating the banner2 over the
  resource endpoint. using BannerModel.create() doesn't expire old
  entries
@utnapischtim
tests: remove SAWarning
526f01a 
* SAWarning: nested transaction already deassociated from connection

* the context manager does the commit automatically
@utnapischtim
chore: change .format() to f-string
5ca1b0c
@utnapischtim
global: apply new SQLAlchemy rules
6492e27 
* change from model Model.query to db.session.query(Model)

* without an update in pytest-invenio and without this commit it
  produces following errors: "sqlalchemy.exc.InvalidRequestError: Can't
  operate on closed transaction inside context manager. Please complete
  the context manager before emitting further commands." and
  'sqlalchemy.exc.IntegrityError: (psycopg2.errors.UniqueViolation)
  duplicate key value violates unique constraint "uq_accounts_user_email"'

* with the updated db fixture in pytest-inveniothis this commit fixes
  following TypeError: 'Session' object is not callable
@utnapischtim
fix: tests
eeb91b7 
* test_banner_creation uses the active banner. the service method which
  is tested validates the data over the schema where start_datetime and
  end_datetime is a required field and it has to look like:
  '%Y-%m-%d %H:%M:%S'

* the problem is that test_read_banner uses the same banner to test the
  read service. the change in "active" has the effect that the stored
  value in the database is not like "2024-07-10 12:04:03.348391" which
  it is due the strftime function is not used. db.DateTime could only
  bring back the date in form of a datetime object which is then used in
  isoformat if the value in the database contains the ".34383" part.

* therefore the easiest solution is to use for test_read_banner the
  "other" banner

* ATTENTION: be cautious during the migration from utcnow to
  now(timezone.utc)
@utnapischtim utnapischtim force-pushed the remove-flask-upper-pin branch from 5e517ad to eeb91b7 Compare October 3, 2024 06:58
@utnapischtim utnapischtim mentioned this pull request Oct 3, 2024
Closed
@Samk13 Samk13 mentioned this pull request Oct 22, 2024
Open
10 tasks
ntarocco
ntarocco reviewed Oct 25, 2024
View reviewed changes
invenio_banners/records/models.py
@@ -62,22 +63,21 @@ def create(cls, data):
)
db.session.add(obj)

db.session.commit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add the unit of work decorator in each service method that is modifying a record, and therefore calling these create/update/delete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this db.session.commit is not necessary. it is also not necessary to change something in the services.

tried in a instance. the banner is stored in the database also without that db.session.commit()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It cannot be magically happen... a decorator must be added here
If it happens, we need to understand why.
Maybe flask-sqlalchemy is auto-committing and the end of the HTTP request?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add a bit to the confusion: Here it seems that they move towards explicit db.session.commit's.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ntarocco you are totally right, it cannot magically happen. it happened the first time i tested it but i couldn't reproduce it. so i added now the unit_of_work decorator to all service methods which create/update/delete models

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice thanks! In case you want to be 100% sure, you could change the resources tests and add something like this:

id = client.put(...)  # create banner
client.get(.../<id>)  # get the created banner

This should fail without the uow, and pass after adding it ☺️

@utnapischtim
refactor: use session.get where possible
3df8f94 
* recomended sqlalchemy syntax for sqlalchemy >= 2.0
@ntarocco
Copy link
Contributor

ntarocco commented Nov 1, 2024

When you are ready to merge, it will unblock this one too (tests not passing)

@utnapischtim utnapischtim force-pushed the remove-flask-upper-pin branch from 949a5f4 to 67b9b35 Compare November 1, 2024 21:01
@utnapischtim utnapischtim merged commit 97ba61c into inveniosoftware:master Nov 3, 2024
2 checks passed
@utnapischtim utnapischtim deleted the remove-flask-upper-pin branch November 3, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zzacharo zzacharo zzacharo left review comments

@ntarocco ntarocco ntarocco left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@utnapischtim @ntarocco @zzacharo