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.

Sign up for GitHub

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

Jump to bottom

Add a way to specify more non-standard-compliant fields to Request #50

Merged
merged 3 commits into from
Aug 4, 2021
Merged

Add a way to specify more non-standard-compliant fields to Request #50

merged 3 commits into from
Aug 4, 2021

Conversation

lhchavez
Copy link
Contributor

This change introduces ExtraField, a CallOption that can add
arbitrary fields to the top-level JSON-RPC Request message.

Fixes: #49

@lhchavez
Add a way to specify more non-standard-compliant fields to Request
f987817 
This change introduces `ExtraField`, a `CallOption` that can add
arbitrary fields to the top-level JSON-RPC Request message.

Fixes: sourcegraph#49
lhchavez
lhchavez commented Jul 27, 2021
View reviewed changes
jsonrpc2.go Outdated
Comment on lines 92 to 93
// This is used to get the extra fields, which are not type-safe.
r3 := make(map[string]*json.RawMessage)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: #49 (comment)

things i tried:

  • make r2 a map[string]interface{}, and it's possible to extract the method, params, meta, and id, but then i needed to marshal all the extra fields into a *json.RawMessage.
  • make r2 a map[string]*json.RawMessage, and it's now possible to extract the extra fields, but the method, params, meta, and id now need an extra unmarshal.
  • make the RequestField.Value an interface{} so that the first option is easier (but didn't really try that because it felt inconsistent with the Meta and Params fields). it also defers the marshaling error to the MarshalJSON, but that might not be such a big deal since both would happen at the .Call() API level.

Copy link
Member

Choose a reason for hiding this comment

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

  • make r2 a map[string]interface{}, and it's possible to extract the method, params, meta, and id, but then i needed to marshal all the extra fields into a *json.RawMessage.

Can't we just make RequestField.Value an interface{}? This seems like the easiest option. It does mean if you want to avoid marshalling on extra fields you can't. Is this acceptable?

  • make r2 a map[string]*json.RawMessage, and it's now possible to extract the extra fields, but the method, params, meta, and id now need an extra unmarshal.

This seems like the best option for avoiding the extra work/allocations? It would also simplify our handling of detecting if params is JSON null? I'm a bit cognizant of "doubling" the work this does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't we just make RequestField.Value an interface{}? This seems like the easiest option. It does mean if you want to avoid marshalling on extra fields you can't. Is this acceptable?

totally acceptable (and in fact will make one particular test easier for us! haha), I just didn't want to start with an option that introduced inconsistency without checking with y'all.

This seems like the best option for avoiding the extra work/allocations? It would also simplify our handling of detecting if params is JSON null? I'm a bit cognizant of "doubling" the work this does.

it avoids the extra allocations, but introduces extra casts. the amount of work is roughly the same but it makes (our particular usage of) the interface a bit simpler due to the lack of the pre-marshaling for the extra fields.

Copy link
Contributor Author

@lhchavez lhchavez Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

oh wait, i lied: this unmarshaling option also adds extra marshaling: the params, id, and meta would now be fully deserialized by the map[string]interface{}, so now we would need to marshsal 'em back to a *json.RawMessage or ID.

keegancsmith
keegancsmith reviewed Jul 28, 2021
View reviewed changes
Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. A bit more discussion inline.

Hah! I see how we added Meta information. We just added in the Meta field even though it isn't part of the standard.

jsonrpc2.go Outdated
Comment on lines 92 to 93
// This is used to get the extra fields, which are not type-safe.
r3 := make(map[string]*json.RawMessage)
Copy link
Member

Choose a reason for hiding this comment

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

  • make r2 a map[string]interface{}, and it's possible to extract the method, params, meta, and id, but then i needed to marshal all the extra fields into a *json.RawMessage.

Can't we just make RequestField.Value an interface{}? This seems like the easiest option. It does mean if you want to avoid marshalling on extra fields you can't. Is this acceptable?

  • make r2 a map[string]*json.RawMessage, and it's now possible to extract the extra fields, but the method, params, meta, and id now need an extra unmarshal.

This seems like the best option for avoiding the extra work/allocations? It would also simplify our handling of detecting if params is JSON null? I'm a bit cognizant of "doubling" the work this does.

lhchavez added 2 commits July 28, 2021 13:16
@lhchavez
Make the RequestField.Value an interface{}
83d34e6 
This change trades an extra JSON unmarshal in `Request.UnmarshalJSON`
for several little casts, checks, and marshals.
@lhchavez
Ensure that SetExtraField cannot be used to overwrite another field
4e1e529
keegancsmith
keegancsmith approved these changes Aug 4, 2021
View reviewed changes
Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

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

Woah, thanks so much for implementing this! Looks a bit hairy but great. Sorry about the delay on approval, I've been mostly AFK for a few days.

jsonrpc2.go
Comment on lines +116 to +121
if ok {
b, err := json.Marshal(meta)
if err != nil {
return fmt.Errorf("failed to marshal Meta: %w", err)
}
r.Meta = (*json.RawMessage)(&b)
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the same technique for params and set it to (a copy of) emptyParams?

Copy link
Member

Choose a reason for hiding this comment

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

I'll save this as a follow-up PR if you agree. Will merge in as is :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good! #52

@keegancsmith keegancsmith merged commit d6ac66e into sourcegraph:master Aug 4, 2021
@lhchavez lhchavez deleted the extra-fields branch August 4, 2021 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keegancsmith keegancsmith keegancsmith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add a way to specify more non-standard-compliant fields to Request
2 participants
@lhchavez @keegancsmith