feature: content app json listing by TenSt · Pull Request #7996 · pulp/pulpcore

TenSt · 2026-08-20T14:15:10Z

📜 Checklist

Commits are cleanly separated with meaningful messages (simple features and bug fixes should be squashed to one commit)
A changelog entry or entries has been added for any significant changes
Follows the Pulp policy on AI Usage
User documentation and test coverage has been added

Description

This PR:

Serves paginated JSON directory listings when the content app Accept prefers JSON
Adds Distribution.content_handler_json() so plugins can customize that JSON
Keeps HTML/binary responses for browsers and other non-JSON clients

Closes #7887

dkliban

Re-review (v2)

Both findings from the initial review have been addressed:

Resolved: `list_directory_flat` now uses DB-level pagination

Pagination is pushed to the database with path_qs[offset:offset+limit] and .count() for the total. Only the page's items are loaded into memory. The _content_relationships() date lookup is now filtered with .filter(content_id__in=content_ids). A new test asserts that LIMIT appears in the captured SQL. This addresses the O(N) memory concern.

Resolved: Cache keys now use normalized pagination params

make_key now computes a normalized {limit}:{offset} string for JSON requests (via the shared json_listing_pagination() function) and omits the query component entirely for HTML requests. Junk query params are ignored for both formats. Verified with our proving test — all 3 cache pollution scenarios now pass.

Remaining: CI lint failure

The v2 push has a lint failure on import sorting (ruff check --fix && ruff format should fix it). Tests are gated on lint so haven't run yet in CI.

TenSt · 2026-08-21T13:09:46Z

@dkliban thanks for the review!

I've added DB pagination to the list_directory_flat function.
Removed query string as a whole from the cache (for json requests only) - only limit and offset will be used.
Skipped it as file content has no special JSON shape, so the generic listing is the right default. There is test that covers this: test_json_vs_html_listing_and_artifact.

Serve a paginated JSON directory listing when Accept prefers application/json. Plugins can override Distribution.content_handler_json. Listing pagination is applied in the database; cache keys use only normalized JSON limit/offset. ref pulp#7887 Assisted-By: Cursor

gerrod3

I'm not sure about this, don't approve before core team has talked about it.

gerrod3 · 2026-08-24T20:13:27Z


+    def content_handler_json(self, path):
+        """
+        Handler to serve a JSON representation of the content at ``path`` for this Distribution.


Use single tick marks.

gerrod3 · 2026-08-24T20:18:35Z

+        # A response for a given path/method can now differ based on the client's Accept
+        # header (JSON vs. HTML/binary), so CacheKeys.format must be part of the cache key.
+        # Without it, a JSON response could be cached and served back for an HTML request
+        # (or vice versa). See pulpcore.cache.accept_prefers_json.
+        # JSON listings are paginated via ?limit=&offset=. CacheKeys.query stores only those
+        # normalized values (and only for JSON), or page N would be served from page 0.


We don't need all these comments.

Actually, I'll disagree here - "why we're doing this" comments help a lot when, in six months, a new person is looking at this code and wondering wtf :)

gerrod3 · 2026-08-24T20:20:06Z

+        wants_json = accept_prefers_json(request.headers.get("Accept"))
+        if wants_json:
+            limit, offset = json_listing_pagination(getattr(request, "query", None))
+            query_key = f"{limit}:{offset}"


I'm not sure we want this to be specific for json.

gerrod3 · 2026-08-24T20:21:57Z

+            text=json.dumps(body, default=str),
+        )
+
+    async def list_directory_flat(self, repo_version, publication, path, limit, offset):


I'm not really liking the idea of making the content-app into an API with the limit and offset queries.

Need more approval from the project team

ggainey · 2026-08-24T14:47:11Z

+    dependency-free leaf module) rather than in ``pulpcore.content.handler`` so that both the
+    content app's response logic and its cache key (see ``AsyncContentCache.make_key``) can use
+    the exact same decision, avoiding any risk of a JSON response being cached/served for an
+    HTML request or vice versa.


Is there not any standard function to process the accept header? Starting from 9110 is def correct, if there's no help to be had from some more-standard place.

ggainey · 2026-08-24T15:03:37Z

+
+
+JSON_LIST_DEFAULT_LIMIT = 1000
+JSON_LIST_MAX_LIMIT = 10000


Having these code in "code" takes control away from the instance-admin. Is there a reason to not have these be controlled in settings instead?

ggainey · 2026-08-24T15:08:59Z


+    # Defaults/bounds for the ?limit=&offset= pagination of the generic JSON directory listing.
+    DEFAULT_JSON_LIST_LIMIT = JSON_LIST_DEFAULT_LIMIT
+    MAX_JSON_LIST_LIMIT = JSON_LIST_MAX_LIMIT


If JSON_LIST_*_LIMIT were in settings.py the tests could find it from the config, and we wouldn't need to duplicate them in Handler. A larger issue is that the content-handler hasn't had any sense of pagination (alas) - do we want to add that just for this special-case? It's already been a more-general problem - see #1951 , for example.

ggainey · 2026-08-24T15:10:31Z

+        This delegates to :func:`pulpcore.cache.accept_prefers_json`, which is also used by
+        :meth:`pulpcore.cache.AsyncContentCache.make_key` (via ``CacheKeys.format``) to key
+        cache entries by negotiated representation. Keeping a single implementation guarantees
+        the caching layer and this negotiation decision can never disagree.


ggainey · 2026-08-24T15:40:13Z

+        representation of ``path``; if that returns None, it falls back to a generic, recursive
+        JSON listing of every file at or below ``path`` (see :meth:`list_directory_flat`) when
+        ``path`` resolves to a directory. Concrete artifact paths are unaffected by ``Accept``
+        unless a plugin's ``content_handler_json`` explicitly handles them.


ggainey · 2026-08-24T21:38:07Z

+    redis_status,
+):
+    if not redis_status:
+        pytest.xfail("Could not connect to the Redis server")


I'd prefer to see a skip on "redis isn't enabled in settings", with "this test makes no sense if redis isn't enabled" - no-redis isn't in any way a failure of the function-under-test.

ggainey · 2026-08-24T21:53:25Z

+)
+def test_negotiate_json(accept, expected):
+    request = Mock(headers={} if accept is None else {"Accept": accept})
+    assert Handler.negotiate_json(request) is expected


negotiate_json() is a passthru for Cache.accept_prefers_json(). The only thing we really want to insure here, is that the call to negotiate_json(request) == accept_prefers_json(request) (since if they don't match, Something Went Worng) Between this and test_cache.test_accept_prefers_json() it feels like we're duplicating the test and not learning much, other than "if we ever have to change accept_prefers we're going to have to change two tests for no real gain".

github-actions Bot added multi-commit no-changelog no-issue labels Aug 20, 2026

dkliban requested changes Aug 21, 2026

View reviewed changes

This comment was marked as off-topic.

Sign in to view

TenSt force-pushed the stepan/7887-content-app-json-listing branch from fcef9d3 to 0d8cfb3 Compare August 21, 2026 12:46

TenSt force-pushed the stepan/7887-content-app-json-listing branch from 0d8cfb3 to c673d4f Compare August 21, 2026 19:39

TenSt force-pushed the stepan/7887-content-app-json-listing branch from c673d4f to d0def8c Compare August 21, 2026 19:43

github-actions Bot removed multi-commit no-changelog labels Aug 21, 2026

dkliban previously approved these changes Aug 21, 2026

View reviewed changes

TenSt mentioned this pull request Aug 24, 2026

feat: serve PyPI and Simple JSON from the content app pulp/pulp_python#1350

Draft

4 tasks

gerrod3 requested changes Aug 24, 2026

View reviewed changes

ggainey requested changes Aug 24, 2026

View reviewed changes

Uh oh!

Conversation

TenSt commented Aug 20, 2026

📜 Checklist

Description

Uh oh!

dkliban left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Re-review (v2)

Resolved: list_directory_flat now uses DB-level pagination

Resolved: Cache keys now use normalized pagination params

Remaining: CI lint failure

Uh oh!

This comment was marked as off-topic.

Uh oh!

TenSt commented Aug 21, 2026

Uh oh!

gerrod3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dkliban left a comment •

edited

Loading

Resolved: `list_directory_flat` now uses DB-level pagination