Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[WIP] 3116 valid number rpc params #1901

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

pscott
Copy link

@pscott pscott commented Sep 3, 2019

PR description

WIP:

  • Add assertLength method to JsonRpcRequest
  • Use assertLength in every RPC method to check whether the number of parameters passed in is correct.
  • Add tests for every method to ensure that behaviour is as expected when too many parameters are passed in.

Fixed Issue(s)

Fixes #3116

@NicolasMassart NicolasMassart self-assigned this Sep 9, 2019
@NicolasMassart NicolasMassart added the api Related to public APIs label Sep 9, 2019
@@ -69,6 +70,15 @@ public String getVersion() {
return params;
}

@JsonIgnore
public void assertMaxLength(final int expectedLength) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest reworking the structure of this code a bit by:

  • Adding a method int getMaxAllowedParams() to the JsonRpcMethod interface
  • Adding a default method assertValidRequest(JsonRpcRequest request) to JsonRpcMethod that throws an InvalidJsonRcpParameters exception as you have here if there are too many params
  • Validating the request by calling jsonRpcMethod.assertValidRequest(request) before executing jsonRpcMethod.response(request) in JsonRpcHttpService

This will make it straightforward to consistently enforce this validation without expecting every method to run these checks manually. And it allows methods to override the default validation if necessary.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much! Definitely a better code structure, I will do my best to implement it.
Quick question : should I still manually add a tooManyParams test for every method, or should I just add one test for one method and "assume" that it will be correct for other methods ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems kind of painful to handcraft tests for every method. But if there was some way to "automatically" test each method, that would be cool. One idea:

  • Write a parameterized test (example) that runs for every method and constructs a request with 1 too many parameters, then verifies that the expected exception is thrown

Otherwise, I think we can probably get away with just adding a single test to JsonRpcHttpServiceTest.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api Related to public APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants