Skip to content

fix: make ResponseMessage.Result omitzero #1533

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 8, 2025

Conversation

skewb1k
Copy link
Contributor

@skewb1k skewb1k commented Aug 7, 2025

According to the JSON-RPC specification, a response object must include either a result field or an error field, but not both.

Without omitzero, if Result is nil, it is marshalled as 'null', causing both result and error fields to appear in the JSON object when Error is also present, which violates the specification.

According to the JSON-RPC specification, a response object
must include either a `result` field or an `error` field, but not both.

Without omitzero, if `Result` is nil, it is marshalled as 'null', causing
both `result` and `error` fields to appear in the JSON object when `Error`
is also present, which violates the specification.
@jakebailey
Copy link
Member

What client are you using that is so strict?

@skewb1k
Copy link
Contributor Author

skewb1k commented Aug 7, 2025

What client are you using that is so strict?

Neovim on the master branch. This is actually caused by my pull request that added the capability to distinguish between null and undefined fields, so now it can detect responses like {result: null, error: {}}. I agree it might be too strict and should be ignored, relying only on the presence of the error field. These two flaws actually help catch each other :).
Still, I think it’s good practice for servers not to violate the JSON-RPC spec.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

LGTM but since I pushed to it last, I can no longer approve and merge it 😅

@jakebailey jakebailey enabled auto-merge August 8, 2025 16:01
@jakebailey
Copy link
Member

Technically you could merge main and then I could merge the PR 😉

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Thanks!

@skewb1k
Copy link
Contributor Author

skewb1k commented Aug 8, 2025

Technically you could merge main and then I could merge the PR 😉

Wow, it feels like this shouldn’t work that way.

@jakebailey
Copy link
Member

Sorta kinda, I could have also sent code via GitHub code suggestions and you could have merged them in via the UI and it would still commit "as me" and do the same thing, so, oh well.

@jakebailey jakebailey added this pull request to the merge queue Aug 8, 2025
Merged via the queue into microsoft:main with commit 4d28085 Aug 8, 2025
22 checks passed
@skewb1k skewb1k deleted the fix/result-omitzero branch August 8, 2025 17:37
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.

3 participants