Skip to content
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

fix: not omitting result when empty #103

Conversation

distractedm1nd
Copy link

Closes #102

A MarshalJSON method is required because the omitempty is conditional - if result could be set if error exists, the diff would only be changing the tag on the struct.

@Wondertan
Copy link

Wondertan commented Sep 12, 2023

Seems like omitempty tag removal would have the same effect or do I miss anything?

@distractedm1nd
Copy link
Author

distractedm1nd commented Sep 20, 2023

@Wondertan It does not have the same effect unfortunately - that was my first solution but it still violates the spec:

This member is REQUIRED on success.
This member MUST NOT exist if there was an error invoking the method.
The value of this member is determined by the method invoked on the Server.

It must exist on success (if error is not empty), even if the value is null. And on error, it is not allowed to exist when the value is null (this is the case that breaks your proposal). This is what I mean in the PR description

Copy link

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Explanation makes sense. Thanks

Comment on lines +114 to +115
// Result field MUST NOT exist if there was an error invoking the method.
// Result field is REQUIRED on success.

Choose a reason for hiding this comment

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

Doc nit: add a line that this is from specs

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Hey, sorry for taking so long to get to this.

Generally this direction looks good, but there's either a serious perf regression here or something is subtly wrong somewhere - this PR makes my tests hang, go test -count=1 -v ./... has now been running for a few mitutes with no signs of finishing, and they do pass in a few seconds on master.

@magik6k
Copy link
Contributor

magik6k commented May 8, 2024

Fixed version of this was now merged in #107

@magik6k magik6k closed this May 8, 2024
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.

Methods that return nothing must contain result field
3 participants