PP-129 OPDS 2 with acquisition links #1340

RishiDiwanTT · 2023-08-23T13:00:54Z

This PR should be reviewed in tandem with the Feed Refactor PR.

Description

Acquisition links for the OPDS 2 feeds as per the De Marque document.
This PR also adds the ability for certain feeds to be served as either XML or JSON based on the requests content-type.
This PR was also used to complete the OPDS2 serializer which was left out of #1308 .

Example

      {
          "href": "http://localhost:6500/VT0014/works/Overdrive%20ID/0e20dfec-501f-4b5d-926f-dd712aa39160/borrow",
          "rel": "http://opds-spec.org/acquisition/borrow",
          "type": "application/atom+xml;type=entry;profile=opds-catalog",
          "properties": {
            "availability": {
              "state": "available"
            },
            "indirectAcquisition": [
              {
                "type": "application/vnd.adobe.adept+xml",
                "child": [
                  {
                    "type": "application/pdf"
                  }
                ]
              },
              {
                "type": "application/vnd.overdrive.circulation.api+json;profile=ebook"
              },
              {
                "type": "application/atom+xml;type=entry;profile=opds-catalog",
                "child": [
                  {
                    "type": "text/html;profile=\"http://librarysimplified.org/terms/profiles/streaming-media\""
                  }
                ]
              }
            ]
          }
        }

Missing

The authenticate property has not been added, since we are currently providing the authentication document link as part of the feed links. Wasn't sure is we wanted to change that behaviour.

Motivation and Context

JIRA-129 and JIRA-78

How Has This Been Tested?

Manually checked

Checklist

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

codecov · 2023-08-24T05:49:46Z

Codecov Report

Patch coverage: 93.49% and project coverage change: +0.08% 🎉

Comparison is base (45a2f09) 89.96% compared to head (9772860) 90.04%.

Additional details and impacted files

@@                       Coverage Diff                       @@
##           feature/refactor-opds-feeds    #1340      +/-   ##
===============================================================
+ Coverage                        89.96%   90.04%   +0.08%     
===============================================================
  Files                              225      227       +2     
  Lines                            30473    30660     +187     
  Branches                          6979     7039      +60     
===============================================================
+ Hits                             27416    27609     +193     
+ Misses                            1965     1957       -8     
- Partials                          1092     1094       +2

Files Changed	Coverage Δ
core/opds_schema.py	`76.78% <0.00%> (-2.85%)`	⬇️
core/feed_protocol/serializer/opds.py	`89.75% <85.71%> (+0.25%)`	⬆️
core/feed_protocol/serializer/opds2.py	`93.66% <92.30%> (ø)`
core/feed_protocol/acquisition.py	`93.76% <100.00%> (+3.73%)`	⬆️
core/feed_protocol/annotator/base.py	`92.09% <100.00%> (+0.07%)`	⬆️
core/feed_protocol/annotator/circulation.py	`89.30% <100.00%> (ø)`
core/feed_protocol/navigation.py	`100.00% <100.00%> (ø)`
core/feed_protocol/opds.py	`100.00% <100.00%> (+10.71%)`	⬆️
core/feed_protocol/serializer/base.py	`100.00% <100.00%> (ø)`
core/feed_protocol/types.py	`91.41% <100.00%> (+0.16%)`	⬆️

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

jonathangreen · 2023-08-30T19:55:32Z

@RishiDiwanTT The first thing I was trying to do with this PR is to load the OPDS2 version of the feed in my test environment to see what the serialized OPDS2 looks like. Can you give me some pointers how I can get an OPDS2 response?

I tried a few incantations to coerce a OPDS2 response, but I came up empty. I would have expected that I could set an Accept header to be able to control the response, but that doesn't seem to be working for me. What were you doing to test this?

RishiDiwanTT · 2023-08-31T00:03:22Z

Currently only the /loans, /borrow and /feed APIs are activated for opds2. They can be seen by adding a content type : application/opds+json header to the GET request.

…

On Thu, Aug 31, 2023, 01:25 Jonathan Green ***@***.***> wrote: @RishiDiwanTT <https://github.com/RishiDiwanTT> The first thing I was trying to do with this PR is to load the OPDS2 version of the feed in my test environment to see what the serialized OPDS2 looks like. Can you give me some pointers how I can get an OPDS2 response? — Reply to this email directly, view it on GitHub <#1340 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVRR5S3WGRNDI7PBTLAFOEDXX6LEBANCNFSM6AAAAAA33ODQWQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

jonathangreen · 2023-08-31T01:17:50Z

Typically the content-type header indicates the type of the data included in the request. I'd expect to do content negotiation like this via an accept header included on the request, which would cause the content-type header to be included in the response.

RishiDiwanTT · 2023-08-31T01:26:33Z

That makes sense, and is a very small change.

…

On Thu, Aug 31, 2023, 06:48 Jonathan Green ***@***.***> wrote: Typically the content-type header <https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type> indicates the type of the data included in the request. I'd expect to do content negotiation like this via an accept header <https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept> included on the request, which would cause the content-type header to be included in the response. — Reply to this email directly, view it on GitHub <#1340 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVRR5SZ5JLBVRK2LM4JD3FTXX7Q4RANCNFSM6AAAAAA33ODQWQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

jonathangreen · 2023-08-31T12:41:49Z

Okay. So I pointed this branch at an existing DB with some books in it, and then I ran the resulting OPDS2 feed through our OPDS2 validator /bin/opds2_schema_validate and our feed is failing our own validation. So i think we need to get that sorted out to start with.

OPDS2 publication feed is now the same as the OPDS1 page feed, with a different serializer

The only real fix was to the audience category

RishiDiwanTT · 2023-09-01T09:45:14Z

Okay. So I pointed this branch at an existing DB with some books in it, and then I ran the resulting OPDS2 feed through our OPDS2 validator /bin/opds2_schema_validate and our feed is failing our own validation. So i think we need to get that sorted out to start with.

This ended up being trivial, changes required were 2

Use the "Accept" header instead of the "Content-Type" header
The "audience" scheme should be an URL always

Apart from that the actual validator code was broken, I guess somewhere along the way the OPDSImporter code changed a bit and the validator did not reflect those changes.

jonathangreen · 2023-09-05T13:42:55Z

From the comment on #1308:

jonathangreen Aug 30, 2023
Can we rename the package (directory) for this code as well? Right now its feed_protocol, I'd suggest feed.

RishiDiwanTT Sep 1, 2023
This will cause all kinds of unpleasantness while rebasing with #1340 . I will do this once that PR has been merged with this.

Moving this to a comment here so we don't forget the package needs to be renamed.

RishiDiwanTT · 2023-09-05T13:46:18Z

From the comment on #1308:

jonathangreen Aug 30, 2023
Can we rename the package (directory) for this code as well? Right now its feed_protocol, I'd suggest feed.

RishiDiwanTT Sep 1, 2023
This will cause all kinds of unpleasantness while rebasing with #1340 . I will do this once that PR has been merged with this.

Moving this to a comment here so we don't forget the package needs to be renamed.

Do we have anymore feedback for this PR or should I merge it into the refactor branch? Once merged I can rename the module.

jonathangreen · 2023-09-05T13:52:42Z

@RishiDiwanTT I'm taking another look over this PR now. I'll post a comment once I'm done.

jonathangreen

@RishiDiwanTT I made a few more comments here

core/feed_protocol/acquisition.py

tests/api/feed_protocol/test_opds_acquisition_feed.py

api/controller.py

tests/api/feed_protocol/test_library_annotator.py

jonathangreen · 2023-09-06T12:49:58Z

core/feed_protocol/acquisition.py

-        elif entry is None:
-            return None
+        elif isinstance(entry, OPDSMessage):
+            return ProblemDetail(


It looks like this new case isn't covered by a test, and it seems like the original code doesn't return a problem detail in this case, but converts the OPDSMessage into an error response?

Can you write up a test for this and verify that we aren't changing the behavior of the single item loans feed here?

Yes, I realised when I changed the order of the return statements, that the single_entry method does not return a ProblemDetail at all, it only returns an OPDSMessage with the problem.
This does change the behaviour from one of returning an XML document (from OPDSMessage.tag ) to that of returning a ProblemDetail when a problem occurs in the single_entry method. This change has been made since the response types are not bound to always being XML.

I think either:

we need to handle this at the serialization step, so that we get the same XML response we used to get in the case of an OPDS 1 feed

Or check with the mobile apps and see how returning a problem detail here instead of a feed will be handled

Since our mobile apps are the primary consumer of this feed still, we need to make sure they are in sync with any changes that get made to the responses.

We can return to the XML format easily, will that be an acceptable behaviour when the client is expecting JSON though?
Re: ProblemDetail: The same method also returns problem details in different circumstances so a problem detail should be already handled by the client side.

The behavior of the mobile apps in the case still worries me. It seems like the thing to do here is to modify OPDSMessage to be non-XML specific, so we are able to serialize it in both OPDS1 and OPDS2 instead of changing the feed behavior.

If that is too much work here, or if you don't want to do that as part of this PR. Can we modify the responses here so that the OPDS1 response (the one that is currently used by the mobile apps) remains the same, rendering the OPDSMessage in the feed as XML and put a follow up ticket in JIRA to handle this case in OPDS2.

@RishiDiwanTT what do you think of those ideas or do you have another idea how we could handle this. The main thing I am concerned about is how this will impact the mobile apps using the OPDS1 feed. The OPDS2 is more "beta" and we can continue to iterate there. I'd like to impact our existing users as little as possible while doing so.

Ok, I reverted to protocol specific data, OPDS1 will be the same XML and OPDS2 will be JSON.

core/feed_protocol/opds.py

jonathangreen

I'm still concerned here about the change in behavior of the OPDSMessage class as part of this PR. I don't want to hold up merging this, but I also don't want it to have a negative impact on the experience of our users consuming the OPDS 1 feed through the app.

core/feed_protocol/opds.py

jonathangreen · 2023-09-08T12:56:07Z

core/feed_protocol/acquisition.py

-        elif entry is None:
-            return None
+        elif isinstance(entry, OPDSMessage):
+            return ProblemDetail(


The behavior of the mobile apps in the case still worries me. It seems like the thing to do here is to modify OPDSMessage to be non-XML specific, so we are able to serialize it in both OPDS1 and OPDS2 instead of changing the feed behavior.

If that is too much work here, or if you don't want to do that as part of this PR. Can we modify the responses here so that the OPDS1 response (the one that is currently used by the mobile apps) remains the same, rendering the OPDSMessage in the feed as XML and put a follow up ticket in JIRA to handle this case in OPDS2.

@RishiDiwanTT what do you think of those ideas or do you have another idea how we could handle this. The main thing I am concerned about is how this will impact the mobile apps using the OPDS1 feed. The OPDS2 is more "beta" and we can continue to iterate there. I'd like to impact our existing users as little as possible while doing so.

Created a Serializer base for better typing and OOP

jonathangreen

🚀 I think this is looking good @RishiDiwanTT

* Added acquisition link serialization for OPDS2 * OPDS2 serialization with acuqisition links * Loan and hold specific statuses for acquisition links * Content types come from the serializer now OPDS2 publication feed is now the same as the OPDS1 page feed, with a different serializer * Ensure test_url_for is only used during testing * Mimetypes are iterated in priority order before serialization Created a Serializer base for better typing and OOP

RishiDiwanTT requested a review from a team August 23, 2023 13:01

RishiDiwanTT self-assigned this Aug 23, 2023

RishiDiwanTT changed the base branch from main to feature/refactor-opds-feeds August 23, 2023 13:06

RishiDiwanTT force-pushed the feature/opds2-with-acqusition-links branch from 16d8c6a to 5138840 Compare August 23, 2023 13:27

RishiDiwanTT changed the title ~~OPDS 2 with acquisition links~~ PP-129 OPDS 2 with acquisition links Aug 23, 2023

RishiDiwanTT force-pushed the feature/refactor-opds-feeds branch from b50811b to f70ec00 Compare August 24, 2023 06:10

RishiDiwanTT force-pushed the feature/opds2-with-acqusition-links branch from 39ec230 to 122365c Compare August 24, 2023 06:40

RishiDiwanTT force-pushed the feature/refactor-opds-feeds branch from c4b041b to f1f1b6b Compare August 29, 2023 07:41

RishiDiwanTT force-pushed the feature/opds2-with-acqusition-links branch from d1d5769 to 857ba47 Compare August 29, 2023 09:23

RishiDiwanTT mentioned this pull request Aug 30, 2023

PP-149 Refactor Acquisition feeds and Annotators #1308

Merged

2 tasks

RishiDiwanTT force-pushed the feature/opds2-with-acqusition-links branch from 857ba47 to aecc464 Compare August 30, 2023 12:05

RishiDiwanTT added 13 commits September 1, 2023 13:44

Added acquisition link serialization for OPDS2

cc45a75

OPDS2 serialization with acuqisition links

6c78807

OPDS2 serializer contributors

3abfc09

Availabilty state revert

4e617f0

Loan and hold specific statuses for acquisition links

8a7801a

Mypy fixes

6aef252

Content types come from the serializer now

ea99429

OPDS2 publication feed is now the same as the OPDS1 page feed, with a different serializer

Fix minor error

c8a7d7c

Test fixes

009a6ab

Additional test cases

bad77e6

Mypy fixes

cf6bf9d

Test Fix

49af1b0

Typing fixes

a9137d1

RishiDiwanTT added 3 commits September 1, 2023 13:45

rebase fixes

03715d3

Ensure test_url_for is only used during testing

90defb3

Rebase fixes

f77e2dd

RishiDiwanTT force-pushed the feature/opds2-with-acqusition-links branch from 127b5bb to f77e2dd Compare September 1, 2023 08:23

OPDS2 schema validation fixes

498c879

The only real fix was to the audience category

Comment update

8c4941b

jonathangreen reviewed Sep 5, 2023

View reviewed changes

core/feed_protocol/acquisition.py Outdated Show resolved Hide resolved

tests/api/feed_protocol/test_opds_acquisition_feed.py Outdated Show resolved Hide resolved

api/controller.py Outdated Show resolved Hide resolved

tests/api/feed_protocol/test_library_annotator.py Show resolved Hide resolved

Mimetypes are iterated in priority order before serialization

508b81a

jonathangreen reviewed Sep 6, 2023

View reviewed changes

Fixed up the get_serializer method and wrote tests

fbb7223

RishiDiwanTT requested a review from jonathangreen September 6, 2023 14:03

RishiDiwanTT added 5 commits September 6, 2023 19:54

Fixed the opds2 mimetype

8e0bbe7

Sort MIMEAccept type values in get_serializer

ec2620e

Fix for none types

fcad7b7

Mypy fix

79fcb0d

Only allow MIMEAccept information when getting the serializer

0cbc81b

jonathangreen reviewed Sep 8, 2023

View reviewed changes

Reverted OPDSMessage responses to type specific responses

9772860

Created a Serializer base for better typing and OOP

jonathangreen approved these changes Sep 12, 2023

View reviewed changes

jonathangreen added the feature New feature label Sep 13, 2023

RishiDiwanTT merged commit 38f4da5 into feature/refactor-opds-feeds Sep 14, 2023
28 checks passed

RishiDiwanTT deleted the feature/opds2-with-acqusition-links branch September 14, 2023 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PP-129 OPDS 2 with acquisition links #1340

PP-129 OPDS 2 with acquisition links #1340

RishiDiwanTT commented Aug 23, 2023 •

edited

Loading

codecov bot commented Aug 24, 2023 •

edited

Loading

jonathangreen commented Aug 30, 2023 •

edited

Loading

RishiDiwanTT commented Aug 31, 2023 via email

jonathangreen commented Aug 31, 2023

RishiDiwanTT commented Aug 31, 2023 via email

jonathangreen commented Aug 31, 2023

RishiDiwanTT commented Sep 1, 2023

jonathangreen commented Sep 5, 2023

RishiDiwanTT commented Sep 5, 2023 •

edited

Loading

jonathangreen commented Sep 5, 2023

jonathangreen left a comment

jonathangreen Sep 6, 2023

RishiDiwanTT Sep 6, 2023

jonathangreen Sep 6, 2023

RishiDiwanTT Sep 6, 2023

jonathangreen Sep 8, 2023

RishiDiwanTT Sep 11, 2023 •

edited

Loading

jonathangreen left a comment

jonathangreen Sep 8, 2023

jonathangreen left a comment

PP-129 OPDS 2 with acquisition links #1340

PP-129 OPDS 2 with acquisition links #1340

Conversation

RishiDiwanTT commented Aug 23, 2023 • edited Loading

Description

Missing

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Aug 24, 2023 • edited Loading

Codecov Report

jonathangreen commented Aug 30, 2023 • edited Loading

RishiDiwanTT commented Aug 31, 2023 via email

jonathangreen commented Aug 31, 2023

RishiDiwanTT commented Aug 31, 2023 via email

jonathangreen commented Aug 31, 2023

RishiDiwanTT commented Sep 1, 2023

jonathangreen commented Sep 5, 2023

RishiDiwanTT commented Sep 5, 2023 • edited Loading

jonathangreen commented Sep 5, 2023

jonathangreen left a comment

Choose a reason for hiding this comment

jonathangreen Sep 6, 2023

Choose a reason for hiding this comment

RishiDiwanTT Sep 6, 2023

Choose a reason for hiding this comment

jonathangreen Sep 6, 2023

Choose a reason for hiding this comment

RishiDiwanTT Sep 6, 2023

Choose a reason for hiding this comment

jonathangreen Sep 8, 2023

Choose a reason for hiding this comment

RishiDiwanTT Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

jonathangreen left a comment

Choose a reason for hiding this comment

jonathangreen Sep 8, 2023

Choose a reason for hiding this comment

jonathangreen left a comment

Choose a reason for hiding this comment

RishiDiwanTT commented Aug 23, 2023 •

edited

Loading

codecov bot commented Aug 24, 2023 •

edited

Loading

jonathangreen commented Aug 30, 2023 •

edited

Loading

RishiDiwanTT commented Sep 5, 2023 •

edited

Loading

RishiDiwanTT Sep 11, 2023 •

edited

Loading