Fix deserialization of Websocket received Notifications - #598
C0D3-M4513R wants to merge 3 commits into
Conversation
commit: |
There was a problem hiding this comment.
🟡 Changes recommended
The oneOf alternatives can reject valid payloads because multiple branches match the same object.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates Notification.details to support structured WebSocket payloads while preserving REST string handling.
Changes:
- Adds object-schema alternatives for WebSocket notifications.
- Retains string deserialization for REST responses.
File summaries
| File | Summary |
|---|---|
openapi/components/schemas/Notification.yaml |
Extends notification detail schemas; the oneOf branches are not mutually exclusive, and WebSocket coverage is missing. |
Review details
Suppressed comments (2)
openapi/components/schemas/Notification.yaml:15
NotificationDetailBoopis listed twice. WithoneOf, a Boop payload matches both identical branches and is rejected even if the other schemas are made disjoint. Remove the duplicate or replace it with the intended distinct detail schema.
- $ref: ./NotificationDetailBoop.yaml
openapi/components/schemas/Notification.yaml:16
- The repository's Arazzo suite currently exercises REST notification endpoints but contains no WebSocket workflow or capture. This union adds several WebSocket-only object shapes, including the empty-details case, so its types and branch behavior are not evidence-backed; add a workflow/capture before documenting these alternatives.
oneOf:
- type: string
- $ref: ./NotificationDetailBoop.yaml
- $ref: ./NotificationDetailBoop.yaml
- $ref: ./NotificationDetailInvite.yaml
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| oneOf: | ||
| - type: string | ||
| - $ref: ./NotificationDetailBoop.yaml |
There was a problem hiding this comment.
Ideally we would somehow have the detail be either a string or the deserialization of the concrete type be determined by the type field of the Notification?
| type: string | ||
| description: "**NOTICE:** This is not a JSON object when received from the REST API, but it is when received from the Websocket API. When received from the REST API, this is a json **encoded** object, meaning you have to json-de-encode to get the NotificationDetail object depending on the NotificationType." | ||
| default: "{}" | ||
| example: "OneOf: {}, NotificationDetailBoop, NotificationDetailInvite, NotificationDetailInviteResponse, NotificationDetailRequestInvite, NotificationDetailRequestInviteResponse, NotificationDetailVoteToKick" |
There was a problem hiding this comment.
Both default and example cannot be assigned to their types now.
There was a problem hiding this comment.
Afaict this property occurs on (at least) invite notifications.
The issue with the current code is, that the detail field in the notifications from the Rest API are apparently (didn't verify that) json-encoded, but the detail field from Notifications in the Websocket aren't.
Because a Json-Object isn't a string, deserialization fails for Notifications received over Websocket.
The details field seems to just hold more information, which is however notification-type specific, about a notification
There was a problem hiding this comment.
Both
defaultandexamplecannot be assigned to their types now.
Also why wouldn't they? string is still a valid type for details in both instances?
There was a problem hiding this comment.
I wonder if it's worth separating this into a WebhookNotification type.
There was a problem hiding this comment.
I wonder if it's worth separating this into a
WebhookNotificationtype.
You mean Websocket not Webhook, right?
It probably would, but didn't the generator dislike types, which are specified, but not used in any route?
Also if we were to do that, then why isn't there a type for Websocket messages as a whole?
(cause fwiw: I do have a source-available implementation of Websocket message deserialization (and also Websocket connection) for rust)
Notifications from the Websocket fail to deserialize properly with (at least) the Rust library, due to the
detailfield.Example (tho a bit redacted) received json over WS:
{ "id": "not_uuid", "type": "invite", "senderUserId": "id", "senderUsername": "username", "receiverUserId": "id", "message": "This is a generated invite to VRChat Home", "details": { "worldId": "wrld_4432ea9b-729c-46e3-8eaf-846aa0a37fdd:18701~group(grp_f300cc08-10e6-4e2a-970a-22d6c78b5353)~groupAccessType(public)~region(eu)", "worldName": "VRChat Home" }, "created_at": "2026-09-14T11:01:13.362Z" }To fix it, I added one-of to the
detailfield.