Fix 'NoneType' object has no attribute 'computed' error (PP-1961) #2184

jonathangreen · 2024-11-21T19:39:34Z

Description

There are two parts to this fix:

Trying a bit harder to get a work for the licensepools we are working with.
Tightening up some of our type hints and None handling, so that we log appropriate errors if we do get a None coming back.

I tried to make sure to add log statements for all of these so that we can figure out whats happening from the logs, if we see these happening in production.

Motivation and Context

This fixes an error we were seeing in production , where we couldn't generate a single item feed for a work. I was actually able to find an item this was happening to and reproduce it, which let me get a fix in place.

Stack trace from production:

Traceback (most recent call last):
  File "/var/www/circulation/env/lib/python3.10/site-packages/flask/app.py", line 880, in full_dispatch_request
    rv = self.dispatch_request()
  File "/var/www/circulation/env/lib/python3.10/site-packages/flask/app.py", line 865, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)  # type: ignore[no-any-return]
  File "/var/www/circulation/src/palace/manager/api/routes.py", line 120, in decorated
    return f(*args, **kwargs)
  File "/var/www/circulation/src/palace/manager/api/routes.py", line 70, in decorated
    return f(*args, **kwargs)
  File "/var/www/circulation/src/palace/manager/api/routes.py", line 93, in wrapped_function
    resp = make_response(f(*args, **kwargs))
  File "/var/www/circulation/src/palace/manager/core/app_server.py", line 87, in decorated
    v = f(*args, **kwargs)
  File "/var/www/circulation/src/palace/manager/core/app_server.py", line 163, in compressor
    return f(*args, **kwargs)
  File "/var/www/circulation/src/palace/manager/api/routes.py", line 510, in permalink
    return app.manager.work_controller.permalink(identifier_type, identifier)
  File "/var/www/circulation/src/palace/manager/api/controller/work.py", line 135, in permalink
    return OPDSAcquisitionFeed.entry_as_response(
  File "/var/www/circulation/src/palace/manager/feed/opds.py", line 88, in entry_as_response
    if not entry.computed:
AttributeError: 'NoneType' object has no attribute 'computed'

JIRA:

Parent: PP-1860
PP-1961

How Has This Been Tested?

Tested with item that was causing issue locally
Running tests in CI

Checklist

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

jonathangreen · 2024-11-21T19:43:25Z

src/palace/manager/api/controller/circulation_manager.py

-        work: Work = pools[0].work
+        # We know there is at least one LicensePool. Find the first one with
+        # a work set on it.
+        work: Work | None = first_or_default([lp.work for lp in pools if lp.work])


This is where the problem we were seeing in production stems from. pools[0].work was None, and that's where the trouble began. In that case there was another license pool that had a work set, so we try to find the first LP with a work set instead of just using the first one.

I also added some handling for the case where there is no work, since this case is still possible.

jonathangreen · 2024-11-21T19:48:33Z

src/palace/manager/api/controller/work.py

-    def permalink(self, identifier_type, identifier):
+    def permalink(
+        self, identifier_type: str, identifier: str
+    ) -> OPDSEntryResponse | ProblemDetail:


Added some type hinting here so mypy checks this function, which would have given us an indication this could have been a problem.

jonathangreen · 2024-11-21T19:51:08Z

src/palace/manager/feed/acquisition.py

@@ -558,15 +557,13 @@ def single_entry_loans_feed(
        if not annotator:
            annotator = LibraryLoanAndHoldAnnotator(circulation, None, library)

-        log = logging.getLogger(cls.__name__)
-


The logging changes in this file are only tangentially related. I just cleaned up some of the logging here to use LoggerMixin while I was at it, so its easier to see where these messages are coming from in the future.

jonathangreen · 2024-11-21T19:52:02Z

src/palace/manager/feed/acquisition.py

@@ -608,7 +605,7 @@ def single_entry_loans_feed(
    @classmethod
    def single_entry(
        cls,
-        work: Work | Edition | None,
+        work: Work | Edition,


I looked at everyone calling this function to make sure they are not (or updated them) passing None here.

codecov · 2024-11-21T19:59:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.03%. Comparing base (1d1b4eb) to head (2af3db1).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2184      +/-   ##
==========================================
+ Coverage   91.01%   91.03%   +0.01%     
==========================================
  Files         361      361              
  Lines       41205    41198       -7     
  Branches     8849     8846       -3     
==========================================
  Hits        37504    37504              
+ Misses       2425     2420       -5     
+ Partials     1276     1274       -2

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

tdilauro

Looks good! A couple small comments below.

tdilauro · 2024-11-22T19:55:08Z

src/palace/manager/api/controller/loan.py

            else:
-                content = etree.tostring(feed)
-            return Response(
-                response=content,
-                status=200,
-                content_type=OPDSFeed.ACQUISITION_FEED_TYPE,
-            )
+                raise PalaceValueError(
+                    "Unexpected return type from OPDSAcquisitionFeed.single_entry_loans_feed"
+                )


Minor: We can remove the else and pop the throw out one level, since it's guarded by other two cases.

Agreed. I just completely removed the if statement and raise since it should never get hit anyway.

tdilauro · 2024-11-22T20:03:37Z

src/palace/manager/feed/acquisition.py


        # There's no reason to present a book that has no active license pool.
        if not identifier:
-            logging.warning("%r HAS NO IDENTIFIER", work)
-            return None
+            raise PalaceValueError("Work has no identifier")


Do we not want the work info in this log message? Or are we getting that somehow?

Good call. I updated the exception to add the repr to the message:

PalaceValueError(f"Work has no associated identifier: {work!r}")

jonathangreen · 2024-11-22T20:11:57Z

src/palace/manager/feed/acquisition.py

            )
            return cls.error_message(
                identifier,
                403,
                "I know about this work but can offer no way of fulfilling it.",
            )
-        except Exception as e:
-            logging.error("Exception generating OPDS entry for %r", work, exc_info=e)
-            return None


Drop this overly broad except. If we are getting random exceptions, we should handle them as they come up. Looking back at our logs, I don't see this being triggered recently, so it should be fine to drop.

jonathangreen · 2024-11-22T20:13:15Z

tests/manager/feed/test_opds_acquisition_feed.py

+        with services_fixture.wired():
+            feed = OPDSAcquisitionFeed.active_loans_for(
+                None, patron, LibraryAnnotator(None, None, db.default_library())
+            )


This change is needed because without a wired service container we get an exception (and have been for awhile) that was being swallowed by the except clause I removed.

jonathangreen · 2024-11-22T20:13:36Z

tests/manager/api/admin/controller/test_custom_lists.py

+        with (
+            admin_librarian_fixture.request_context_with_library_and_admin("/"),
+            services_fixture.wired(),
+        ):


This change is needed because without a wired service container we get an exception (and have been for awhile) that was being swallowed by the except clause I removed.

Fix 'NoneType' object has no attribute 'computed' error

fa3bace

jonathangreen added the bug Something isn't working label Nov 21, 2024

Add additional log message.

e4ad277

jonathangreen commented Nov 21, 2024

View reviewed changes

jonathangreen requested a review from a team November 21, 2024 19:52

jonathangreen changed the title ~~Fix 'NoneType' object has no attribute 'computed' error (PP-1860)~~ Fix 'NoneType' object has no attribute 'computed' error (PP-1961) Nov 22, 2024

jonathangreen added 2 commits November 22, 2024 15:12

Sharpen up some of the types that we return

7f9a9a6

Fix tests

e62bf4b

tdilauro approved these changes Nov 22, 2024

View reviewed changes

Some updates

2af3db1

jonathangreen commented Nov 22, 2024

View reviewed changes

jonathangreen merged commit 7b3a977 into main Nov 22, 2024
21 checks passed

jonathangreen deleted the bugfix/nonetype-computed branch November 22, 2024 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 'NoneType' object has no attribute 'computed' error (PP-1961) #2184

Fix 'NoneType' object has no attribute 'computed' error (PP-1961) #2184

jonathangreen commented Nov 21, 2024 •

edited

Loading

jonathangreen Nov 21, 2024

jonathangreen Nov 21, 2024

jonathangreen Nov 21, 2024

jonathangreen Nov 21, 2024

codecov bot commented Nov 21, 2024 •

edited

Loading

tdilauro left a comment

tdilauro Nov 22, 2024

jonathangreen Nov 22, 2024

tdilauro Nov 22, 2024

jonathangreen Nov 22, 2024

jonathangreen Nov 22, 2024

jonathangreen Nov 22, 2024

jonathangreen Nov 22, 2024

Fix 'NoneType' object has no attribute 'computed' error (PP-1961) #2184

Fix 'NoneType' object has no attribute 'computed' error (PP-1961) #2184

Conversation

jonathangreen commented Nov 21, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

tdilauro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading