Skip to content

Add first-class FacetData type for OPDS feed facet groups (PP-3675)#3109

Merged
jonathangreen merged 5 commits intomainfrom
feature/first-class-facets
Mar 11, 2026
Merged

Add first-class FacetData type for OPDS feed facet groups (PP-3675)#3109
jonathangreen merged 5 commits intomainfrom
feature/first-class-facets

Conversation

@jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Mar 4, 2026

Description

Introduce a FacetData dataclass that groups facet links by their facet group, moving group-level metadata (group name, type) out of individual Link objects and into a dedicated container. This eliminates redundant per-link fields (facet_group, facet_group_type, active_sort) and removes the need to reconstruct groups at serialization time (e.g. via defaultdict in the OPDS2 serializer).

Key changes:

  • New FacetData type in feed/types.py with group, type, and links fields
  • FeedData.facet_links: list[Link] replaced by FeedData.facets: list[FacetData]
  • Facet grouping now happens in acquisition.py at construction time, not at serialization time
  • OPDS facet rel (http://opds-spec.org/facet) moved from data layer into OPDS1 serializer where it belongs
  • active-sort attribute extracted into an ACTIVE_SORT_ATTR constant
  • Removed dead code: is_sort_facet(), palace_active_sort property, active_sort link field
  • OPDS2 sort facets now render in the facets array (with @type) instead of as top-level feed links

OPDS2 Breaking Changes

Sort facets moved from top-level links[] to facets[]

Previously, sort options were serialized as individual top-level links with rel: "http://palaceproject.io/terms/rel/sort" and a palace_active_sort property. They now appear as a facet group inside the facets[] array, identified by "@type": "http://palaceproject.io/terms/rel/sort" in the facet metadata. The active sort option is indicated by "rel": "self" on its link (consistent with how all other active facets are marked).

// Before
{
  "links": [
    { "href": "...", "rel": "http://palaceproject.io/terms/rel/sort", "title": "Title",
      "properties": { "http://palaceproject.io/terms/properties/active-sort": true } }
  ]
}

// After
{
  "facets": [
    {
      "metadata": { "title": "Sort by", "@type": "http://palaceproject.io/terms/rel/sort" },
      "links": [
        { "href": "...", "rel": "self", "title": "Title",
          "properties": { "http://palaceproject.io/terms/properties/default": true } }
      ]
    }
  ]
}

Removed palace_active_sort link property

The http://palaceproject.io/terms/properties/active-sort property has been removed from OPDS2 link properties. Active sort/facet state is now uniformly communicated via "rel": "self" on the active link within a facet group.

Impact

These changes only affect the OPDS2 feed format. The only current OPDS2 consumer is Aspen, which does not use sort or entrypoint facet links, so these changes should have no practical impact. OPDS1 v1 and v2 feeds are unaffected by the structural changes (sort links and facet attributes are serialized as before).

Motivation and Context

The previous design stored group-level metadata (group name, group type) redundantly on every individual facet Link. This meant serializers had to reconstruct the grouping by inspecting link attributes, and format-specific concerns (like the OPDS1 facet rel) leaked into the shared data layer. The new FacetData type models the one-to-many relationship between a facet group and its links directly, making the code cleaner and less error-prone.

How Has This Been Tested?

  • All existing tests updated to use the new FacetData structure
  • New test assertions verify facet rel is set by the OPDS1 serializer
  • Tests cover OPDS1 v1, OPDS1 v2, and OPDS2 serialization paths

Checklist

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

@jonathangreen jonathangreen added the feature New feature label Mar 4, 2026
@codecov
Copy link

codecov bot commented Mar 4, 2026

Codecov Report

❌ Patch coverage is 96.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.24%. Comparing base (94fba06) to head (3393a8c).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/palace/manager/feed/acquisition.py 94.44% 0 Missing and 1 partial ⚠️
src/palace/manager/feed/serializer/opds.py 95.65% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3109   +/-   ##
=======================================
  Coverage   93.23%   93.24%           
=======================================
  Files         492      492           
  Lines       45429    45404   -25     
  Branches     6250     6245    -5     
=======================================
- Hits        42357    42335   -22     
+ Misses       1984     1983    -1     
+ Partials     1088     1086    -2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jonathangreen jonathangreen force-pushed the feature/first-class-facets branch from 1f55f84 to c5155bb Compare March 5, 2026 14:30
Introduce a FacetData dataclass that groups facet links by their facet
group, moving group-level metadata (group name, type) out of individual
Link objects. This eliminates redundant per-link fields (facet_group,
facet_group_type, active_sort) and removes the need to reconstruct
groups at serialization time. The OPDS facet rel and active-sort
attribute are now applied by the OPDS1 serializer where they belong.
@jonathangreen jonathangreen force-pushed the feature/first-class-facets branch from 3c0c247 to 67ea9a6 Compare March 6, 2026 15:14
@jonathangreen jonathangreen added the incompatible changes Changes that require a new major version label Mar 6, 2026
- Use dataclasses.replace() in _serialize_facet_link to preserve all
  Link fields instead of manually copying a subset
- Remove unused group_name parameter from _entrypoint_link
- Move active_sort into V2_ATTRIBUTE_MAPPING for consistency
- Fold _serialize_sort_links into _serialize_facet_links, removing the
  separate abstract method since sort is just another facet group
Detect the sort facet group by its constant key rather than by
display title string comparison.  Remove unused rel fields from
facet link test data since serializers inject the correct rel.
Replace the href fallback for missing facet link titles with an
error log identifying it as a programming error, since all facet
links should always have a title set. Move the minimum-links
check after link filtering so title-less links are excluded from
the count.
V1 historically serialized sort facets as regular facets without a
facetGroupType attribute. The FacetData refactor inadvertently added
this attribute to V1 sort links. Clear the type for sort groups in
V1's _serialize_facet_links to retain the old behavior.
feed.add_link(start_url, rel="start", title=top_level_title)

# Add facet links for visibility filtering
facet_group_data = FacetData(group=SuppressedFacets.VISIBILITY_FACET_GROUP_NAME)
Copy link
Member Author

@jonathangreen jonathangreen Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but there is some duplication here, that I want to resolve. I'm going to handle this by doing a bit of a refactor in a follow up PR. So I kept the changes here minimal for now.

Edit: PR here: #3119

@jonathangreen jonathangreen marked this pull request as ready for review March 6, 2026 16:40
@jonathangreen jonathangreen requested a review from a team March 6, 2026 16:40
Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! 🎉

)
element = self._serialize_link(sort_link)
if link.active_facet:
element.set(self._attr_name("active_sort"), "true")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor - The PR description mentioned that active_sort was extracted into an ACTIVE_SORT_ATTR constant. Although, looking through at least the changes on this PR, it looks like this is the only occurrence left.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah, I changed this up after I opened the PR and forgot to update the description. I think whats here is more consistent with the existing code.

@jonathangreen jonathangreen merged commit 2acad53 into main Mar 11, 2026
20 checks passed
@jonathangreen jonathangreen deleted the feature/first-class-facets branch March 11, 2026 15:41
jonathangreen added a commit that referenced this pull request Mar 11, 2026
) (#3119)

## Description

Refactors `SuppressedFacets` to extend `BaseFacets` and use the standard
`FacetGroup` NamedTuple,
replacing the custom `FacetGroup` dataclass that was specific to the
suppressed feed. This brings
the suppressed feed's faceting into alignment with the rest of the
system's facet infrastructure.

Key changes:
- `SuppressedFacets` now extends `BaseFacets` and implements its
`facet_groups` / `items` protocol
- Removed the custom `FacetGroup` dataclass in favor of the standard
`FacetGroup` from `feed.facets.base`
- Added `AdminSuppressedFeed.facet_links` override to handle the
suppressed feed's custom title scheme
(via `VisibilityFilter.display_title`) while using the standard facet
link machinery
- Added `AdminSuppressedAnnotator.facet_url` to bridge the standard
`facet_url(facets)` interface
  with the existing `suppressed_url(**kwargs)` method
- Used `Self` return types and `self.__class__()` for subclass-friendly
construction

## Motivation and Context

The suppressed feed previously had its own parallel `FacetGroup` type
with different field names
(`group_name` / `filter_value` vs the standard `group` / `value`),
making the codebase harder
to reason about. This PR eliminates that duplication so there is one
`FacetGroup` type across
the system.

Stacked on #3109.

## How Has This Been Tested?

- All existing tests pass
- Added focused unit tests for the new `facet_links` and `facet_url`
methods, including edge case
  coverage for empty URLs
- mypy passes cleanly

## Checklist

- [x] I have updated the documentation accordingly.
- [x] All new and existing tests passed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature incompatible changes Changes that require a new major version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants