Remove external integrations from collection integrations (PP-503) #1494

jonathangreen · 2023-11-01T16:55:50Z

Description

This PR changes the data model for our collections in a number of ways with the goal of removing all reliance on external integrations from our collections.

All the database columns and tables "removed" by this PR have just been renamed on the Python side, but left in place for now. Once this goes into a release #1498 can be merged to remove the now unused tables and columns.

The major changes:

The external_integration_id column has been removed.
Collections no longer have a name column.
- Previously both integrations and collections had names, this meant that they could get out of sync. Now collection names are driven by the name of their integration. A migration handles making sure that the existing integrations that have gotten out of sync are updated to have their collections name.
The collections_libraries table has been removed.
- Collections used to be able to be related to libraries in two ways: through the collections_libraries table, and via their integration_library_configuration. This situation was confusing, and meant that these could get out of sync.
The collection table no longer uses its external_account_id column. This was duplicated between the table and the settings before, it now lives only in the settings. As with the others the column is still present, but is renamed and unused and will be dropped once this is rolled out everywhere.
The settings column get a gin index, to make it easier to query the jsonb column.
Some of the properties on the collection class have been moved out to the specific license provider API that used them. These were on the collection model because previously some providers didn't have their own API classes.
- There are still some of these that could be moved into a more suitable location, but that can happen in a follow up PR.
The collection controller and collection self test controller have been updated to use the integration settings controller base classes. This lets us keep all the logic for updating these settings isolated there. The biggest consequence of this change is that we now pass around the integration ID rather then the collection ID when referencing collections and collection self tests.
- This is transparent to the admin UI, since it just sends back the IDs given to it when it queries for the collection settings.
- This makes the collection controllers act like the other controllers that modify integrations, rather then using different IDs in the collection controller.
The collection self test controller removes the special case that used to exist for OPDS importer collections, since they now have their own API class.
- This PR removes the self test functionality for the OPDS 1.x importer class, since we don't have any OPDS 1.x collections in our production CMs. Here is a ticket to add self tests for the collection classes missing them (OPDS 2, OPDS 2 + ODL), but this is the situation we've been in for awhile, so no impact from this PR. This PR will make it easier to add these self tests by just implementing the self test interface in the api classes.
- There is also a DB migration to apply these database changes and an associated migration test.
All the major files touched by this PR are now fully type hinted, and added to the strict mypy group in pyproject.toml.

Motivation and Context

Remove external integrations from collections PP-503.

How Has This Been Tested?

Light local testing with the CM admin UI:
- Setting up collections
- Running migrations
- Associating and disassociating libraries
CI tests

Checklist

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

codecov · 2023-11-01T20:09:16Z

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (8cbe939) 90.40% compared to head (94efafa) 90.39%.

Files	Patch %	Lines
core/model/collection.py	84.61%	3 Missing and 3 partials ⚠️
api/admin/controller/collection_settings.py	93.42%	1 Missing and 4 partials ⚠️
core/opds2_import.py	40.00%	2 Missing and 1 partial ⚠️
core/integration/base.py	95.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1494      +/-   ##
==========================================
- Coverage   90.40%   90.39%   -0.01%     
==========================================
  Files         233      233              
  Lines       29371    29157     -214     
  Branches     6860     6799      -61     
==========================================
- Hits        26552    26357     -195     
+ Misses       1796     1783      -13     
+ Partials     1023     1017       -6

Flag	Coverage Δ
Api	`73.83% <90.03%> (+0.04%)`	⬆️
Core	`61.06% <29.96%> (+0.09%)`	⬆️
migration	`29.42% <16.61%> (+0.12%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

api/admin/controller/patron_auth_service_self_tests.py

api/admin/problem_details.py

api/controller.py

api/integration/registry/license_providers.py

core/model/__init__.py

core/model/integration.py

core/model/listeners.py

tests/api/admin/controller/test_collection_settings.py

core/model/library.py

…gration config.

…roller.

RishiDiwanTT

This took a while 😅
I mostly had comments about the migration, most of the rest seemed A-OK 👌

RishiDiwanTT · 2023-11-06T06:14:40Z

alembic/versions/20231101_2d72d6876c52_remove_collection_external_integration.py

+
+log = logging.getLogger(f"palace.migration.{revision}")
+log.setLevel(logging.INFO)
+log.disabled = False


Maybe these 3 lines can be a util method like get_migration_logger(revision)

alembic/versions/20231101_2d72d6876c52_remove_collection_external_integration.py

RishiDiwanTT · 2023-11-06T07:02:09Z

alembic/versions/20231101_2d72d6876c52_remove_collection_external_integration.py

+    # value, then make it non-nullable.
+    op.add_column(
+        "integration_configurations",
+        sa.Column("context", postgresql.JSONB(astext_type=sa.Text()), nullable=True),


Why not set nullable=False with default='{}' here rather than later?

I don't think that you can use default with a new column in a table that has existing data.

When I try that the migration fails for me with:

E sqlalchemy.exc.IntegrityError: (psycopg2.errors.NotNullViolation) column "context" contains null values E E [SQL: ALTER TABLE integration_configurations ADD COLUMN context JSONB NOT NULL]

Ah ok, yes default is an application level default. Using server_default will work, I tested it out.

I think this changes the DB level schema though, right?

So migrated tables would have a different schema definition then tables that weren't migrated. We could mitigate this by changing the column definition to use server_default, but since none of our other tables with defaults use server_default, I'd rather leave it as default for consistency.

api/admin/controller/settings.py

RishiDiwanTT · 2023-11-06T10:11:33Z

api/odl.py

@@ -78,7 +78,7 @@ class ODLAPIConstants:


 class ODLSettings(OPDSImporterSettings):
-    external_account_id: Optional[HttpUrl] = FormField(
+    external_account_id: HttpUrl = FormField(


Now that we are using pydantic for this attribute (and not ignoring it anymore in favour of the collection.external_account_id) this is causing problems for me locally since some of my feeds are http://localhost and HttpUrl has require_tld=True. This can go away with me changing my local settings, but it's something all the developers will need to be aware of.

Hmm that's a good point... I maybe I'll change this one to AnyHttpUrl, so its a bit less strict.

Updated this one.

core/model/__init__.py

RishiDiwanTT · 2023-11-07T06:54:48Z

api/odl2.py

@@ -37,7 +37,7 @@
    from core.model.patron import Hold, Loan, Patron


-class ODL2Settings(OPDS2ImporterSettings, ODLSettings):


I assume the side effect of this making username and password mandatory was on purpose

It is intentional, I think these were changed to non-required by accident in #1189. If you checkout 299003e and take a look at the config, the configuration form in this PR matches what I see there.

jonathangreen · 2023-11-07T14:19:32Z

@RishiDiwanTT I think I've got all the code review feedback resolved here. Pinging you for another review.

RishiDiwanTT

Looks good 👍

Clean up leftover columns from #1494, now that it has gone into a release.

jonathangreen force-pushed the feature/remove-external-integration-collection branch from c8f6854 to cadf562 Compare November 1, 2023 23:18

jonathangreen changed the title ~~Remove external integrations from collection integrations~~ Remove external integrations from collection integrations (PP-503) Nov 2, 2023

jonathangreen added the DB migration This PR contains a DB migration label Nov 2, 2023

jonathangreen marked this pull request as ready for review November 2, 2023 23:09

jonathangreen requested a review from a team November 2, 2023 23:10

jonathangreen commented Nov 2, 2023

View reviewed changes

jonathangreen mentioned this pull request Nov 3, 2023

Remove old columns from collection table #1498

Merged

2 tasks

jonathangreen added 18 commits November 3, 2023 14:20

Remove external integration.

0d9d4ff

Collections don't store their own name, that is delegated to the inte…

400d0b1

…gration config.

Collections are related to libraries via their settings.

635f313

Remove collection external_account_id setting.

33a83d9

Remove a couple more unused properties

72df032

Remove OPDS importer specific primary_identifier_source from collection.

58497be

Turn protocol into a property rather then a hybrid-property

c89b097

Move child settings into child settings object.

bbe033d

Add index.

c8d9039

Replace collection settings controller with integration settings cont…

1d324f3

…roller.

Update self test controller

1ac0512

Remove unused OPDS self test code

6927204

Add initial migration script

adf7aa7

Add additional test for collection self tests

4a444c2

Work through the migration. Don't remove old DB columns.

cfc1f59

Mypy fix

613e134

Clean up code coverage a bit.

593901c

Add tests for migration.

b1c284f

jonathangreen force-pushed the feature/remove-external-integration-collection branch from c9817fe to 2a439b9 Compare November 3, 2023 17:20

Small updates for issues found in manual testing.

eafb469

jonathangreen force-pushed the feature/remove-external-integration-collection branch from 2a439b9 to eafb469 Compare November 3, 2023 17:35

RishiDiwanTT reviewed Nov 6, 2023

View reviewed changes

jonathangreen added 2 commits November 6, 2023 16:17

Address code review feedback.

7de714b

Make sure AnyHttpUrl is used everywhere.

d902766

jonathangreen requested a review from RishiDiwanTT November 6, 2023 21:08

RishiDiwanTT reviewed Nov 7, 2023

View reviewed changes

jonathangreen requested a review from RishiDiwanTT November 7, 2023 14:19

RishiDiwanTT approved these changes Nov 7, 2023

View reviewed changes

jonathangreen added 3 commits November 7, 2023 11:12

Merge branch 'main' into feature/remove-external-integration-collection

cc2f196

Fix opds validation tests.

75ba9ef

Merge branch 'main' into feature/remove-external-integration-collection

94efafa

jonathangreen merged commit f3b9c9b into main Nov 17, 2023
30 checks passed

jonathangreen deleted the feature/remove-external-integration-collection branch November 17, 2023 13:58

jonathangreen added a commit that referenced this pull request Nov 29, 2023

Remove old columns from collection table (#1498)

a99a74f

Clean up leftover columns from #1494, now that it has gone into a release.

jonathangreen mentioned this pull request Jan 4, 2024

Fix overdrive advantage collection parent reporting (PP-806) #1590

Merged

2 tasks

jonathangreen restored the feature/remove-external-integration-collection branch February 6, 2024 16:18

jonathangreen deleted the feature/remove-external-integration-collection branch February 6, 2024 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove external integrations from collection integrations (PP-503) #1494

Remove external integrations from collection integrations (PP-503) #1494

jonathangreen commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 1, 2023 •

edited

Loading

RishiDiwanTT left a comment

RishiDiwanTT Nov 6, 2023

RishiDiwanTT Nov 6, 2023

jonathangreen Nov 6, 2023

RishiDiwanTT Nov 7, 2023

jonathangreen Nov 7, 2023

RishiDiwanTT Nov 6, 2023

jonathangreen Nov 6, 2023

jonathangreen Nov 6, 2023

RishiDiwanTT Nov 7, 2023

jonathangreen Nov 7, 2023

jonathangreen commented Nov 7, 2023

RishiDiwanTT left a comment

		@@ -37,7 +37,7 @@
		from core.model.patron import Hold, Loan, Patron


		class ODL2Settings(OPDS2ImporterSettings, ODLSettings):

Remove external integrations from collection integrations (PP-503) #1494

Remove external integrations from collection integrations (PP-503) #1494

Conversation

jonathangreen commented Nov 1, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Nov 1, 2023 • edited Loading

Codecov Report

RishiDiwanTT 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen commented Nov 7, 2023

RishiDiwanTT left a comment

Choose a reason for hiding this comment

jonathangreen commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 1, 2023 •

edited

Loading