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

json rpc server returns trace if enabled #60

Merged
merged 6 commits into from
Nov 25, 2017
Merged

json rpc server returns trace if enabled #60

merged 6 commits into from
Nov 25, 2017

Conversation

prolic
Copy link
Owner

@prolic prolic commented Nov 25, 2017

see also: #56

This is a small BC as the Error Interface now has an additional method. Usually it would not be expected to have an implementation of this Error Interface in userland code, so maybe we can introduce it without new major version.

Thoughts? @oqq @thomasvargiu @bweston92

@coveralls
Copy link

coveralls commented Nov 25, 2017

Coverage Status

Coverage increased (+0.1%) to 92.801% when pulling beb8574 on json_rpc_trace into 73a777b on master.

@oqq
Copy link
Contributor

oqq commented Nov 25, 2017

This change does not match the JSON-RPC 2.0 Spec for the error object.
How I read the rfc, this response should only contain code, message and data.

Maybe the trace should be in data.

@prolic
Copy link
Owner Author

prolic commented Nov 25, 2017

The trace is only meant for debugging in development mode, and should not be turned on in production, that's why I think it's okay to add it as additional field to the error object. The purpose of data is to show example data, how a good request looks like, also changing the data field would lead to BC break on consumer side, that's why we should not add it to data.

@oqq
Copy link
Contributor

oqq commented Nov 25, 2017

http://www.jsonrpc.org/specification#error_object

data
A Primitive or Structured value that contains additional information about the error.
This may be omitted.
The value of this member is defined by the Server (e.g. detailed error information, nested errors etc.).

The data field is defined exactly for this behaivor.. adding some more info about the error.

@coveralls
Copy link

coveralls commented Nov 25, 2017

Coverage Status

Coverage decreased (-1.4%) to 91.333% when pulling 78c42ea on json_rpc_trace into 73a777b on master.

@coveralls
Copy link

coveralls commented Nov 25, 2017

Coverage Status

Coverage decreased (-1.3%) to 91.365% when pulling 909eb0e on json_rpc_trace into 73a777b on master.

@prolic prolic merged commit be3b148 into master Nov 25, 2017
@prolic prolic deleted the json_rpc_trace branch November 25, 2017 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants