Add JSON-LD namespace and content negotiation for AP objects - #1385
Add JSON-LD namespace and content negotiation for AP objects#1385alphatownsman wants to merge 1 commit into
Conversation
- Define neodb: namespace (https://neodb.social/ns#) for all custom ActivityPub types and properties used by NeoDB - Add @context to catalog item AP responses (Movie, Edition, etc.) - Add @context to journal piece standalone AP responses - Add content negotiation to review and collection views so they serve AP JSON when Accept header requests it - Add /p/<uuid> endpoint for serving Rating, Comment, ShelfMember, Note, and CollectionMember as dereferenceable AP objects - Update neodb-takahe submodule to include neodb namespace in outgoing federated Note context
There was a problem hiding this comment.
Code Review
This pull request introduces ActivityPub and JSON-LD context support for catalog items and journal pieces, allowing them to be served as standalone ActivityPub objects. It adds a new piece_retrieve view and updates the collection_retrieve and review_retrieve views to support content negotiation via the Accept header. The review feedback recommends making the content negotiation logic more robust by checking for specific ActivityPub mime types and suggests moving a local import to the top level for consistency.
| collection = get_object_or_404(Collection, uid=get_uuid_or_404(collection_uuid)) | ||
| if not collection.is_visible_to(request.user): | ||
| raise PermissionDenied(_("Insufficient permission")) | ||
| if request.headers.get("Accept", "").endswith("json"): |
There was a problem hiding this comment.
The content negotiation logic is a bit too broad. Checking if the Accept header endswith("json") can lead to incorrectly serving JSON for mime types like text/json. It would be more robust to check for specific ActivityPub-related mime types like application/activity+json and application/ld+json.
| if request.headers.get("Accept", "").endswith("json"): | |
| if any(mime in request.headers.get("Accept", "") for mime in ("application/activity+json", "application/ld+json")): |
| raise PermissionDenied(_("Insufficient permission")) | ||
| if request.method == "HEAD": | ||
| return HttpResponse() | ||
| if request.headers.get("Accept", "").endswith("json"): |
There was a problem hiding this comment.
The content negotiation logic is a bit too broad. Checking if the Accept header endswith("json") can lead to incorrectly serving JSON for mime types like text/json. It would be more robust to check for specific ActivityPub-related mime types like application/activity+json and application/ld+json.
| if request.headers.get("Accept", "").endswith("json"): | |
| if any(mime in request.headers.get("Accept", "") for mime in ("application/activity+json", "application/ld+json")): |
| if request.headers.get("Accept", "").endswith("json"): | ||
| from django.http import JsonResponse | ||
|
|
||
| return JsonResponse( | ||
| piece.ap_object_response(), content_type="application/activity+json" | ||
| ) |
There was a problem hiding this comment.
This block can be improved in two ways:
- The content negotiation logic is too broad. Checking if the
Acceptheaderendswith("json")can incorrectly match mime types liketext/json. It's better to check for specific ActivityPub mime types likeapplication/activity+jsonandapplication/ld+json. - The
from django.http import JsonResponseis a local import. For consistency and best practice, it should be moved to the top of the file with other imports.
Here's a suggested replacement for this block. Please also remember to add the import to the top of the file.
if any(mime in request.headers.get("Accept", "") for mime in ("application/activity+json", "application/ld+json")):
return JsonResponse(
piece.ap_object_response(), content_type="application/activity+json"
)| return JsonResponse( | ||
| piece.ap_object_response(), content_type="application/activity+json" | ||
| ) |
There was a problem hiding this comment.
Bug: Requests for FeaturedCollection or Like objects via the /p/<uuid> endpoint with an ActivityPub Accept header will crash because they are missing the required ap_object implementation.
Severity: HIGH
Suggested Fix
Implement the ap_object property on the FeaturedCollection and Like models to return a valid ActivityPub object representation. Alternatively, if these models are not meant to be served via this endpoint, add logic to piece_retrieve to explicitly exclude them or handle them gracefully instead of causing a server error.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: journal/views/post.py#L61-L63
Potential issue: The `piece_retrieve` view at `/p/<uuid>` can fetch any subclass of
`Piece`. However, the `FeaturedCollection` and `Like` models, which are `Piece`
subclasses, do not implement the required `ap_object` property as mandated by the base
class. When a request is made to the UUID of a `FeaturedCollection` or `Like` object
with an ActivityPub-related `Accept` header (e.g., one ending in `json`), the code
attempts to call the unimplemented `ap_object` property. This triggers an unhandled
`NotImplementedError`, resulting in an HTTP 500 server error and crashing the request.
Did we get this right? 馃憤 / 馃憥 to inform future reviews.
| collection = get_object_or_404(Collection, uid=get_uuid_or_404(collection_uuid)) | ||
| if not collection.is_visible_to(request.user): | ||
| raise PermissionDenied(_("Insufficient permission")) | ||
| if request.headers.get("Accept", "").endswith("json"): |
There was a problem hiding this comment.
Bug: The content negotiation logic uses a fragile .endswith("json") check on the Accept header, which fails for valid headers with parameters or multiple values, causing HTML to be served instead of JSON.
Severity: HIGH
Suggested Fix
Replace the .endswith("json") check with a more robust content negotiation method. For example, check if a specific media type like "application/activity+json" or "application/ld+json" is present in the Accept header string. A better approach would be to use a library or Django's built-in utilities that can properly parse Accept headers and respect quality factors.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: journal/views/collection.py#L96
Potential issue: The content negotiation logic in multiple views checks if the `Accept`
header string ends with `"json"` to determine whether to serve ActivityPub JSON. This
check is not robust. It will incorrectly fail for valid, spec-compliant `Accept` headers
that contain parameters (e.g., `application/ld+json; profile="..."`) or multiple media
types (e.g., `application/activity+json, text/html`). In these realistic scenarios, the
server will fall back to serving HTML instead of the requested JSON, which breaks object
dereferencing for ActivityPub federation.
Did we get this right? 馃憤 / 馃憥 to inform future reviews.
Summary
neodb:namespace (https://neodb.social/ns#) for all custom ActivityPub types and properties@contextwith proper JSON-LD namespace to catalog item and journal piece AP responses/p/<uuid>endpoint so Rating, Comment, ShelfMember, Note, CollectionMember are dereferenceable AP objectsDetails
NeoDB publishes custom types (
Review,Rating,Comment,Status,CollectionItem) and properties (withRegardTo,relatedWith,best,worst,value, etc.) in ActivityPub objects. Previously these were bare JSON keys without a JSON-LD namespace, making them invisible to JSON-LD-aware consumers. Catalog items were served asapplication/activity+jsonwithout any@context.This PR:
@contextwithneodb:namespace to all AP object responses/review/<uuid>only returned HTML, and/p/<uuid>was a 404)canonicalise()default context so federated Notes carry proper namespace definitionsNoteandCollectionjournal types are intentionally omitted from the neodb context to avoid conflicting with AS2 types of the same name.Backward compatible: old servers use our context from the incoming document for JSON-LD processing, so all terms round-trip correctly.
Test plan
@contextwith neodb namespace/review/<uuid>withAccept: application/activity+jsonreturns AP JSON/collection/<uuid>withAccept: application/activity+jsonreturns AP JSON/p/<uuid>serves AP JSON for Rating/Comment/ShelfMember/Note pieces@context