[PP-1358] add new events #2174

dbernstein · 2024-11-19T17:13:40Z

Description

This update captures five new circulation events:

circulation_manager_hold_expired
circulation_manager_hold_ready
circulation_manager_hold_converted_to_loan
circulation_manager_loan_expired
circulation_manager_loan_converted_to_hold

I have a couple of questions/comments to inform the review:

We are capturing hold expirations where the CM is and is not the source of truth. However for loans I believe we are only handling expired loans where the CM is not source of truth. Did I miss something here? Or should we be doing something for loan expirations where CM is not the source of truth?
When we convert holds to loans I am both capturing the conversion event as well as recording a "checkout". Because it is a checkout event that happens to be a conversion. I think that's what we want but others may disagree.
I put a note in the code, but I'll call it out again here: there appears to be a case where a loan can be converted to a hold. I'm not sure what the scenario is, but I've captured it is a distinct event in addition to the placing of the hold.
I ran into some mypy issues with the collect_event method. That led me to observe that all loans and holds have patrons and the intention seems to be that all patrons must have loans and holds but the database does not enforce that constraint. I included the migration here, but I am happy to move it to a separate PR is that seems cleaner.

Motivation and Context

https://ebce-lyrasis.atlassian.net/browse/PP-1804

How Has This Been Tested?

New unit tests added. Older unit tests amended.

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

codecov · 2024-11-19T18:08:44Z

Codecov Report

Attention: Patch coverage is 98.55072% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.10%. Comparing base (ec00a8e) to head (94d8de0).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/palace/manager/api/circulation.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2174      +/-   ##
==========================================
+ Coverage   91.09%   91.10%   +0.01%     
==========================================
  Files         363      363              
  Lines       41201    41260      +59     
  Branches     8827     8834       +7     
==========================================
+ Hits        37531    37591      +60     
  Misses       2406     2406              
+ Partials     1264     1263       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jonathangreen

I'm a bit concerned about the performance impacts of this change. I'd like to understand that a bit better before this one goes in.

alembic/versions/20241119_58b0ae7f5b67_make_loan_patron_id_and_hold_patron_id_.py

src/palace/manager/celery/tasks/opds_odl.py

src/palace/manager/core/monitor.py

src/palace/manager/sqlalchemy/model/patron.py

tests/manager/api/test_authenticator.py

jonathangreen · 2024-12-09T14:56:18Z

@dbernstein looks like mypy is failing on this one

dbernstein · 2024-12-09T18:50:39Z

This one has is ready to go. I fixed the mypy issues a couple of bugs, and reset the migration target.

dbernstein · 2024-12-09T18:51:02Z

@jonathangreen ^^

jonathangreen

A couple more things to look at here @dbernstein. I'd like to get the race condition when deleting holds sorted out, and make sure we're not going to run into issues referencing a closed session before this gets merged.

jonathangreen · 2024-12-10T13:25:34Z

alembic/versions/20241204_58b0ae7f5b67_make_loan_patron_id_and_hold_patron_id_.py

@@ -0,0 +1,43 @@
+"""Make Loan.patron_id and Hold.patron_id non-nullable.
+
+Revision ID: 58b0ae7f5b67


Sorry @dbernstein, I meant to ping you on this PR. This migration can just go away as part of this PR, since the migration in #2197 covers it.

jonathangreen · 2024-12-10T13:27:32Z

src/palace/manager/celery/tasks/opds_odl.py

+
+    # a separate query is required to get around the
+    # "FOR UPDATE cannot be applied to the nullable side of an outer join" issue when trying to use with_for_update
+    # on the Hold object.


Minor: If that is the case, I think its better to just drop the lock query (lines 28 - 46) all together.

jonathangreen · 2024-12-10T13:34:43Z

src/palace/manager/celery/tasks/opds_odl.py

@@ -31,11 +75,12 @@ def remove_expired_holds_for_collection(db: Session, collection_id: int) -> int:
        .execution_options(synchronize_session="fetch")
    )
    result = db.execute(query)


I'm still uncomfortable with the race condition that can happen here, since the delete and the event are two separate queries. If a hold expires between the select on line 54 and the delete query on line 67 we will never log an event for it.

It would be better to do this all in one query, either by iterating over the expired holds and running session.delete on them or doing something like:

query = ( delete(Hold) .where( Hold.id.in_(h.id for h in expired_holds) ) ) result = db.execute(query)

that way we are sure the set of objects that we log events for, and the set that we delete from the database are the same.

jonathangreen · 2024-12-10T13:35:52Z

src/palace/manager/celery/tasks/opds_odl.py

    # We need the type ignores here because result doesn't always have
    # a rowcount, but the sqlalchemy docs swear it will in the case of
    # a delete statement.
    # https://docs.sqlalchemy.org/en/20/tutorial/data_update.html#getting-affected-row-count-from-update-delete
-    return result.rowcount  # type: ignore[attr-defined,no-any-return]
+    return result.rowcount, expired_hold_events  # type: ignore


Minor: We can probably simplify the return here to expired_hold_events, since len(expired_hold_events) gets us result.rowcount

jonathangreen · 2024-12-10T13:49:04Z

src/palace/manager/celery/tasks/opds_odl.py

+
+    # publish events only after successful commit
+    for event in events:
+        analytics.collect_event(**event)


This looks unsafe to me, since events contains references to library and patron which are bound to the transactions session, but after exiting the transactions context manager the session is closed. Are you sure this will work?

I think this is likely going to result in session errors, that may not appear when running in tests, since we never actually close the session in tests, but we do when running the real application.

jonathangreen · 2024-12-10T13:51:22Z

src/palace/manager/celery/tasks/opds_odl.py

@@ -211,6 +288,10 @@ def recalculate_hold_queue_collection(
                    f"{updated} holds out of date."
                )

+            # fire events after successful database update
+            for event in events:
+                analytics.collect_event(**event)


I've got the same concern here about objects being referenced after their session is closed.

…voked events.

converted to hold.

Also use with_for_update to lock expired holds before deletion Also ensure that transaction is closed before collecting analytics events.

…to analytics collector.

dbernstein · 2024-12-11T19:51:23Z

@jonathangreen : Hopefully I've addressed your concerns here. I wanted to keep the event collection outside the transaction as you initially suggested to limit the duration of any row level locks in the database. To accomplish this requirement I am starting a new session and reattaching the objects from the closed session. I also removed the migration.

jonathangreen

Thanks for working through the changes on this one. Looks good to me!

dbernstein added the feature New feature label Nov 19, 2024

dbernstein force-pushed the PP-1358-add-new-events branch 2 times, most recently from 4020690 to a1700c8 Compare November 19, 2024 18:10

dbernstein requested a review from a team November 19, 2024 18:28

dbernstein added the DB migration This PR contains a DB migration label Nov 19, 2024

jonathangreen reviewed Nov 21, 2024

View reviewed changes

dbernstein force-pushed the PP-1358-add-new-events branch 2 times, most recently from 69cd3e4 to 8db8da1 Compare December 3, 2024 20:01

dbernstein force-pushed the PP-1358-add-new-events branch 2 times, most recently from df35c58 to 547869f Compare December 9, 2024 18:34

jonathangreen reviewed Dec 10, 2024

View reviewed changes

dbernstein added 15 commits December 10, 2024 14:27

[PP-1358] Add hold expired, loan expired, hold converted, and loan re…

1607e2f

…voked events.

Fix tests.

ad93d7f

Fix tests and mypy

1f37950

Add test for hold converted to loan and test for loan

658cb88

converted to hold.

Revert unintended change.

7ee4d16

Formatting.

ee5567b

Fix broken tests.

9d9001b

Make loan.patron_id and hold.patron_id fields non-nullable.

e8d9eaa

Improve test formatting.

3f6a31e

Create separate tasks for reaping holds by collection.

463ba28

Also use with_for_update to lock expired holds before deletion Also ensure that transaction is closed before collecting analytics events.

Change date on revision to ensure it reflects the order of application.

8e84469

Fix a couple of bugs.

3ae5a00

Fix mypy

c7b6883

Merge in mistakenly unmerged changes.

7b54daf

Fix migration target.

96c620b

dbernstein added 2 commits December 10, 2024 14:28

Remove unnecessary migration.

55b14f4

Ensure that event data is attached to a database session when passed …

5c942dd

…to analytics collector.

dbernstein force-pushed the PP-1358-add-new-events branch from 547869f to 5c942dd Compare December 11, 2024 00:06

Fix mypy

94d8de0

jonathangreen approved these changes Dec 12, 2024

View reviewed changes

dbernstein merged commit 48260da into main Dec 12, 2024
21 checks passed

dbernstein deleted the PP-1358-add-new-events branch December 12, 2024 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PP-1358] add new events #2174

[PP-1358] add new events #2174

dbernstein commented Nov 19, 2024 •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading

jonathangreen left a comment

jonathangreen commented Dec 9, 2024

dbernstein commented Dec 9, 2024

dbernstein commented Dec 9, 2024

jonathangreen left a comment

jonathangreen Dec 10, 2024

jonathangreen Dec 10, 2024

jonathangreen Dec 10, 2024

jonathangreen Dec 10, 2024

jonathangreen Dec 10, 2024

jonathangreen Dec 10, 2024

dbernstein commented Dec 11, 2024

jonathangreen left a comment

		@@ -0,0 +1,43 @@
		"""Make Loan.patron_id and Hold.patron_id non-nullable.

		Revision ID: 58b0ae7f5b67

[PP-1358] add new events #2174

[PP-1358] add new events #2174

Conversation

dbernstein commented Nov 19, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Nov 19, 2024 • edited Loading

Codecov Report

jonathangreen left a comment

Choose a reason for hiding this comment

jonathangreen commented Dec 9, 2024

dbernstein commented Dec 9, 2024

dbernstein commented Dec 9, 2024

jonathangreen left a comment

Choose a reason for hiding this comment

jonathangreen Dec 10, 2024

Choose a reason for hiding this comment

jonathangreen Dec 10, 2024

Choose a reason for hiding this comment

jonathangreen Dec 10, 2024

Choose a reason for hiding this comment

jonathangreen Dec 10, 2024

Choose a reason for hiding this comment

jonathangreen Dec 10, 2024

Choose a reason for hiding this comment

jonathangreen Dec 10, 2024

Choose a reason for hiding this comment

dbernstein commented Dec 11, 2024

jonathangreen left a comment

Choose a reason for hiding this comment

dbernstein commented Nov 19, 2024 •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading