Skip to content

[2.x] Fix notifications endpoint 400 on invalid subject.discussion default include#4816

Open
karl-bullock wants to merge 1 commit into
flarum:2.xfrom
karl-bullock:fix/notification-subject-discussion-include
Open

[2.x] Fix notifications endpoint 400 on invalid subject.discussion default include#4816
karl-bullock wants to merge 1 commit into
flarum:2.xfrom
karl-bullock:fix/notification-subject-discussion-include

Conversation

@karl-bullock

Copy link
Copy Markdown
Contributor

Fixes #4815

Changes proposed in this pull request

NotificationResource added subject.discussion to the notifications endpoints' default include whenever more than one notification subject type was registered:

$this->initialized && count($this->subjectTypes()) > 1
    ? 'subject.discussion'
    : null,

That nested include is only valid when at least one subject type actually exposes an includable discussion relationship. The include validator validates default includes too, so when the subject collection is, for example, {discussions, users} (neither of which has a discussion relationship), it rejects the endpoint's own default include:

{"errors":[{"status":"400","code":"validation_error","title":"Bad Request","detail":"Invalid include [subject.discussion]","source":{"parameter":"include"}}]}

Because NotificationListState sends no explicit include and relies on the server default, this returns 400 for every user and breaks the notifications dropdown and page. User subjects are not exotic: flarum/mentions ships UserMentionedBlueprint; it only stays safe because it also ships PostMentionedBlueprint, and Post has a discussion relationship. Any forum with a User-subject (or Group-subject) notification type and no Post-subject type present hits this.

This PR gates the include on whether a subject type genuinely exposes an includable discussion relationship, instead of on the subject-type count. The include is preserved exactly where it matters (Post subjects) and dropped only when it would be invalid.

Reviewers should focus on

  • subjectsHaveDiscussionRelationship() resolves each subject type's resource and checks its fields for an includable discussion relationship. This uses resolveFields() (which applies field extenders), guarded by an instanceof AbstractResource check.

Verified

Reproduced and fixed on a fresh 2.0.0-rc.5 install (MariaDB):

Registered subject types GET /api/notifications before after
{discussions} (core only) 200 200
{discussions, users} 400 200
{discussions, users, posts} 200 200

Two regression tests are added to notifications/ListTest: one asserting the endpoint no longer 400s when a subject type has no discussion relationship, and one asserting the discussion is still eager-loaded when a discussion-bearing subject type is present. The existing notification tests still pass.

NotificationResource added `subject.discussion` to the default include
whenever more than one notification subject type was registered. That
include is only valid when a subject type actually has a `discussion`
relationship; with, for example, a User-subject notification type and no
Post-subject type, the include validator rejects the endpoint's own
default include with `400 Invalid include [subject.discussion]`, breaking
the notifications endpoint for every user on the forum.

Gate the include on whether any subject type genuinely exposes an
includable `discussion` relationship instead of on the subject-type count.

Adds regression tests covering both the no-discussion-subject case (no
longer 400s) and that the discussion is still eager-loaded when a
discussion-bearing subject type is present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[2.x] Notifications endpoint 400s on an invalid subject.discussion default include when a notification subject type has no discussion relationship

1 participant