-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
PP-129 OPDS 2 with acquisition links #1340
Merged
RishiDiwanTT
merged 26 commits into
feature/refactor-opds-feeds
from
feature/opds2-with-acqusition-links
Sep 14, 2023
Merged
Changes from 25 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
cc45a75
Added acquisition link serialization for OPDS2
RishiDiwanTT 6c78807
OPDS2 serialization with acuqisition links
RishiDiwanTT 3abfc09
OPDS2 serializer contributors
RishiDiwanTT 4e617f0
Availabilty state revert
RishiDiwanTT 8a7801a
Loan and hold specific statuses for acquisition links
RishiDiwanTT 6aef252
Mypy fixes
RishiDiwanTT ea99429
Content types come from the serializer now
RishiDiwanTT c8a7d7c
Fix minor error
RishiDiwanTT 009a6ab
Test fixes
RishiDiwanTT bad77e6
Additional test cases
RishiDiwanTT cf6bf9d
Mypy fixes
RishiDiwanTT 49af1b0
Test Fix
RishiDiwanTT a9137d1
Typing fixes
RishiDiwanTT 03715d3
rebase fixes
RishiDiwanTT 90defb3
Ensure test_url_for is only used during testing
RishiDiwanTT f77e2dd
Rebase fixes
RishiDiwanTT 498c879
OPDS2 schema validation fixes
RishiDiwanTT 8c4941b
Comment update
RishiDiwanTT 508b81a
Mimetypes are iterated in priority order before serialization
RishiDiwanTT fbb7223
Fixed up the get_serializer method and wrote tests
RishiDiwanTT 8e0bbe7
Fixed the opds2 mimetype
RishiDiwanTT ec2620e
Sort MIMEAccept type values in get_serializer
RishiDiwanTT fcad7b7
Fix for none types
RishiDiwanTT 79fcb0d
Mypy fix
RishiDiwanTT 0cbc81b
Only allow MIMEAccept information when getting the serializer
RishiDiwanTT 9772860
Reverted OPDSMessage responses to type specific responses
RishiDiwanTT File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,48 +1,87 @@ | ||
from __future__ import annotations | ||
|
||
import logging | ||
from typing import Any, List, Optional | ||
from typing import Any, Dict, List, Optional, Type | ||
|
||
from werkzeug.datastructures import MIMEAccept | ||
|
||
from core.feed_protocol.base import FeedInterface | ||
from core.feed_protocol.serializer.opds import OPDS1Serializer | ||
from core.feed_protocol.serializer.opds2 import OPDS2Serializer | ||
from core.feed_protocol.types import FeedData, WorkEntry | ||
from core.util.flask_util import OPDSEntryResponse, OPDSFeedResponse | ||
from core.util.opds_writer import OPDSMessage | ||
|
||
|
||
def get_serializer( | ||
jonathangreen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
mime_types: Optional[MIMEAccept], | ||
) -> OPDS1Serializer | OPDS2Serializer: | ||
# Loop through and return whichever mimetype is encountered first | ||
# Sort values by q-value first | ||
serializers: Dict[str, Type[Any]] = { | ||
"application/opds+json": OPDS2Serializer, | ||
"application/atom+xml": OPDS1Serializer, | ||
} | ||
if mime_types: | ||
match = mime_types.best_match( | ||
serializers.keys(), default="application/atom+xml" | ||
) | ||
return serializers[match]() # type: ignore[no-any-return] | ||
jonathangreen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# Default | ||
return OPDS1Serializer() | ||
|
||
|
||
class BaseOPDSFeed(FeedInterface): | ||
def __init__( | ||
self, title: str, url: str, precomposed_entries: Optional[List[Any]] = None | ||
self, | ||
title: str, | ||
url: str, | ||
precomposed_entries: Optional[List[OPDSMessage]] = None, | ||
) -> None: | ||
self.url = url | ||
self.title = title | ||
self._precomposed_entries = precomposed_entries or [] | ||
self._feed = FeedData() | ||
self._serializer = OPDS1Serializer() | ||
self.log = logging.getLogger(self.__class__.__name__) | ||
|
||
def serialize(self) -> bytes: | ||
return self._serializer.serialize_feed(self._feed) | ||
def serialize(self, mime_types: Optional[MIMEAccept] = None) -> bytes: | ||
serializer = get_serializer(mime_types) | ||
return serializer.serialize_feed(self._feed) | ||
|
||
def add_link(self, href: str, rel: Optional[str] = None, **kwargs: Any) -> None: | ||
self._feed.add_link(href, rel=rel, **kwargs) | ||
|
||
def as_response(self, **kwargs: Any) -> OPDSFeedResponse: | ||
def as_response( | ||
self, | ||
mime_types: Optional[MIMEAccept] = None, | ||
**kwargs: Any, | ||
) -> OPDSFeedResponse: | ||
"""Serialize the feed using the serializer protocol""" | ||
serializer = get_serializer(mime_types) | ||
return OPDSFeedResponse( | ||
self._serializer.serialize_feed( | ||
serializer.serialize_feed( | ||
self._feed, precomposed_entries=self._precomposed_entries | ||
), | ||
content_type=serializer.content_type(), | ||
**kwargs, | ||
) | ||
|
||
@classmethod | ||
def entry_as_response( | ||
cls, entry: WorkEntry, **response_kwargs: Any | ||
cls, | ||
entry: WorkEntry, | ||
mime_types: Optional[MIMEAccept] = None, | ||
**response_kwargs: Any, | ||
) -> OPDSEntryResponse: | ||
if not entry.computed: | ||
logging.getLogger().error(f"Entry data has not been generated for {entry}") | ||
raise ValueError(f"Entry data has not been generated") | ||
serializer = OPDS1Serializer() | ||
return OPDSEntryResponse( | ||
response=serializer.serialize_work_entry(entry.computed), **response_kwargs | ||
serializer = get_serializer(mime_types) | ||
response = OPDSEntryResponse( | ||
response=serializer.serialize_work_entry(entry.computed), | ||
**response_kwargs, | ||
) | ||
if isinstance(serializer, OPDS2Serializer): | ||
# Only OPDS2 has the same content type for feed and entry | ||
response.content_type = serializer.content_type() | ||
return response |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thesingle_entry
method. This change has been made since the response types are not bound to always being XML.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think either:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I reverted to protocol specific data, OPDS1 will be the same XML and OPDS2 will be JSON.