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

proposal: x/tools/internal/jsonrpc2_v2: export wireError #64882

Closed
kortschak opened this issue Dec 27, 2023 · 4 comments
Closed

proposal: x/tools/internal/jsonrpc2_v2: export wireError #64882

kortschak opened this issue Dec 27, 2023 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@kortschak
Copy link
Contributor

Proposal Details

This may not require a proposal since the package is internal. If a formal proposal is needed, I will flesh this out.

Problem

Currently the jsonrpc2.wireError does not provide access to the Data field within a handler. This makes it impossible for a handler to add data to a JSON RPC2.0 error in a way that will result in the caller receiving it due to the way that errors are handled by the response marshalling code path.

Proposal

I propose that the wireError type be exported. This will enable handlers to add values to the Data field and callers to be able to inspect the returned error data.

A less appealing alternative would be to add a new error creation function that adds the data field (additional API function surface) or alter the existing NewError (a relatively invasive and backward incompatible change, though note that the package is internal). Both of these changes would also require the addition an accessor for callers to be able to inspect the Data field.

Compatibility

The proposal is fully backward compatible because it exposes a previously unexported type. The package is currently internal.

Cost/Benefit

The package is currently internal and no internal user appears to have a need for this functionality. I make use of the package as a mechanically extracted non-internal package. If the proposal is rejected, I can work around this by making the change in the extracted package, but I'd like to see whether this is needed before I make that change.

@gopherbot gopherbot added this to the Proposal milestone Dec 27, 2023
@adonovan
Copy link
Member

adonovan commented Feb 7, 2024

This doesn't need a proposal as it doesn't change the public API. Feel free to send us a CL and we can discuss it there.

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Feb 7, 2024
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unreleased Feb 7, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/562515 mentions this issue: internal/jsonrpc2: export WireError type

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/562575 mentions this issue: internal/jsonrpc2: export WireError type

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/562575 mentions this issue: internal/jsonrpc2_v2: export WireError type

gopherbot pushed a commit to golang/tools that referenced this issue Feb 14, 2024
This makes it possible for users of the package to us the optional error
data field in error construction and error handling logic.

Updates golang/go#64882

Change-Id: Iaa554f42ff42a0b7355605ba0b49ac67f623da15
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562575
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants