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

Increment id field on requests #210

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@IgorPerikov
Contributor

IgorPerikov commented Oct 29, 2017

#173

  1. Remove id from Request constructor
  2. Fixed tests - now tests check equality of request objects (except request id, since it's random uuid), and checks that request-id field is not null
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 29, 2017

Codecov Report

Merging #210 into master will increase coverage by <.01%.
The diff coverage is 16.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #210      +/-   ##
============================================
+ Coverage      77.1%   77.11%   +<.01%     
- Complexity     1486     1491       +5     
============================================
  Files           202      202              
  Lines          5472     5483      +11     
  Branches        884      890       +6     
============================================
+ Hits           4219     4228       +9     
+ Misses         1055     1050       -5     
- Partials        198      205       +7
Impacted Files Coverage Δ Complexity Δ
...java/org/web3j/protocol/admin/JsonRpc2_0Admin.java 100% <ø> (ø) 7 <0> (ø) ⬇️
.../java/org/web3j/protocol/core/JsonRpc2_0Web3j.java 94.87% <ø> (ø) 70 <0> (ø) ⬇️
...va/org/web3j/protocol/parity/JsonRpc2_0Parity.java 77.94% <ø> (ø) 25 <0> (ø) ⬇️
...n/java/org/web3j/protocol/geth/JsonRpc2_0Geth.java 100% <ø> (ø) 5 <0> (ø) ⬇️
...src/main/java/org/web3j/protocol/core/Request.java 69.44% <16.66%> (+5.44%) 12 <1> (+5) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3df473d...de21a4b. Read the comment docs.

codecov bot commented Oct 29, 2017

Codecov Report

Merging #210 into master will increase coverage by <.01%.
The diff coverage is 16.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #210      +/-   ##
============================================
+ Coverage      77.1%   77.11%   +<.01%     
- Complexity     1486     1491       +5     
============================================
  Files           202      202              
  Lines          5472     5483      +11     
  Branches        884      890       +6     
============================================
+ Hits           4219     4228       +9     
+ Misses         1055     1050       -5     
- Partials        198      205       +7
Impacted Files Coverage Δ Complexity Δ
...java/org/web3j/protocol/admin/JsonRpc2_0Admin.java 100% <ø> (ø) 7 <0> (ø) ⬇️
.../java/org/web3j/protocol/core/JsonRpc2_0Web3j.java 94.87% <ø> (ø) 70 <0> (ø) ⬇️
...va/org/web3j/protocol/parity/JsonRpc2_0Parity.java 77.94% <ø> (ø) 25 <0> (ø) ⬇️
...n/java/org/web3j/protocol/geth/JsonRpc2_0Geth.java 100% <ø> (ø) 5 <0> (ø) ⬇️
...src/main/java/org/web3j/protocol/core/Request.java 69.44% <16.66%> (+5.44%) 12 <1> (+5) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3df473d...de21a4b. Read the comment docs.

@IgorPerikov

This comment has been minimized.

Show comment
Hide comment
@IgorPerikov

IgorPerikov Oct 29, 2017

Contributor

Hi, @conor10. That's actually the first time I'm interacting with codecov and I don't understand what it's complaining about - increased complexity? Coverage is improved as I can see. What should I fix?

Contributor

IgorPerikov commented Oct 29, 2017

Hi, @conor10. That's actually the first time I'm interacting with codecov and I don't understand what it's complaining about - increased complexity? Coverage is improved as I can see. What should I fix?

@conor10

This comment has been minimized.

Show comment
Hide comment
@conor10

conor10 Oct 30, 2017

Collaborator

Unfortunately, it looks like geth and parity don't like UUID's in the JSON-RPC messages (even though strings are valid in the id field).

So instead, what I suggest is that you use an AtomicLong instead:

    private static AtomicLong nextId = new AtomicLong(0); 

...

    public Request(String method, List<S> params,
                   Web3jService web3jService, Class<T> type) {
        this.method = method;
        this.params = params;
        this.id = nextId.getAndIncrement();
        this.web3jService = web3jService;
        this.responseType = type;
    }

Then change the getters & setters to be for a long value instead of UUID.

There are a few other changes inline.

Collaborator

conor10 commented Oct 30, 2017

Unfortunately, it looks like geth and parity don't like UUID's in the JSON-RPC messages (even though strings are valid in the id field).

So instead, what I suggest is that you use an AtomicLong instead:

    private static AtomicLong nextId = new AtomicLong(0); 

...

    public Request(String method, List<S> params,
                   Web3jService web3jService, Class<T> type) {
        this.method = method;
        this.params = params;
        this.id = nextId.getAndIncrement();
        this.web3jService = web3jService;
        this.responseType = type;
    }

Then change the getters & setters to be for a long value instead of UUID.

There are a few other changes inline.

@@ -75,4 +77,25 @@ public T send() throws IOException {
public Observable<T> observable() {
return new RemoteCall<>(this::send).observable();
}

This comment has been minimized.

@conor10

conor10 Oct 30, 2017

Collaborator

Please can we use IntelliJ style equals & hashcode methods instead? This is to remain Android compatible.

@conor10

conor10 Oct 30, 2017

Collaborator

Please can we use IntelliJ style equals & hashcode methods instead? This is to remain Android compatible.

This comment has been minimized.

@IgorPerikov

IgorPerikov Oct 30, 2017

Contributor

Yeah, sure. Btw this one is generated by Intellij too, but requires java7(which is the problem for android, I guess)

@IgorPerikov

IgorPerikov Oct 30, 2017

Contributor

Yeah, sure. Btw this one is generated by Intellij too, but requires java7(which is the problem for android, I guess)

@@ -42,7 +46,12 @@ protected void verifyResult(String expected) throws Exception {
Buffer buffer = new Buffer();
requestBody.writeTo(buffer);
assertThat(buffer.readUtf8(), is(expected));
org.web3j.protocol.core.Request actualRequest =

This comment has been minimized.

@conor10

conor10 Oct 30, 2017

Collaborator

Unfortunately, I'm not keen on this approach.

We shouldn't be deserialising back into an object - the equality comparison should be using strings.

What I'd suggest is that you replace the id: <generatedValue>} via a regex that replaces this with id: <generatedValue>} then you check for string equality.

I think it's important that the JSON-RPC requests we're testing are similar to what's being sent down the wire.

@conor10

conor10 Oct 30, 2017

Collaborator

Unfortunately, I'm not keen on this approach.

We shouldn't be deserialising back into an object - the equality comparison should be using strings.

What I'd suggest is that you replace the id: <generatedValue>} via a regex that replaces this with id: <generatedValue>} then you check for string equality.

I think it's important that the JSON-RPC requests we're testing are similar to what's being sent down the wire.

This comment has been minimized.

@IgorPerikov

IgorPerikov Oct 30, 2017

Contributor

I'll fix this

@IgorPerikov

IgorPerikov Oct 30, 2017

Contributor

I'll fix this

This comment has been minimized.

@IgorPerikov

IgorPerikov Oct 30, 2017

Contributor

I realised, that I don't really understand what kind of fix are you talking about. I see 2 solutions, both with their cons and pros:

  1. use any long number in test method arguments like this verifyResult("...., \"id\":42"), replace this inside verifyResult method with some phrase(like <generatedValue>), replace actual request id with same phrase and compare strings. This one means you should pass some magic constant, which might be not very obvious for those who will read this.
  2. pass some code phrase in test methods instead of magic constant: verifyResult("...., \"id\":<generatedValue>"), replace actual request id with same code phrase and compare strings. No magic constant, but makes it a little harder to pass correct value, and it's still not very obvious. Writing new test will probably lead to some research - what should I copy-paste and why is it works.

What are your thoughts about it?

@IgorPerikov

IgorPerikov Oct 30, 2017

Contributor

I realised, that I don't really understand what kind of fix are you talking about. I see 2 solutions, both with their cons and pros:

  1. use any long number in test method arguments like this verifyResult("...., \"id\":42"), replace this inside verifyResult method with some phrase(like <generatedValue>), replace actual request id with same phrase and compare strings. This one means you should pass some magic constant, which might be not very obvious for those who will read this.
  2. pass some code phrase in test methods instead of magic constant: verifyResult("...., \"id\":<generatedValue>"), replace actual request id with same code phrase and compare strings. No magic constant, but makes it a little harder to pass correct value, and it's still not very obvious. Writing new test will probably lead to some research - what should I copy-paste and why is it works.

What are your thoughts about it?

This comment has been minimized.

@conor10

conor10 Nov 8, 2017

Collaborator

Hey @IgorPerikov, sorry for the delay in coming back to you - just got back from Devcon.

Option 1 is in line with my thinking.

Thanks.

@conor10

conor10 Nov 8, 2017

Collaborator

Hey @IgorPerikov, sorry for the delay in coming back to you - just got back from Devcon.

Option 1 is in line with my thinking.

Thanks.

@conor10

This comment has been minimized.

Show comment
Hide comment
@conor10

conor10 Oct 30, 2017

Collaborator

Cool.

How long do you need? I'm about to do the 3.0 release (next hour or so) - just need to know if you'll be able to get this in in time, or if it should go in the first release following 3.0.

Collaborator

conor10 commented Oct 30, 2017

Cool.

How long do you need? I'm about to do the 3.0 release (next hour or so) - just need to know if you'll be able to get this in in time, or if it should go in the first release following 3.0.

@IgorPerikov

This comment has been minimized.

Show comment
Hide comment
@IgorPerikov

IgorPerikov Oct 30, 2017

Contributor

Today's evening is all I need. Seems like it for the first release after 3.0

Contributor

IgorPerikov commented Oct 30, 2017

Today's evening is all I need. Seems like it for the first release after 3.0

@conor10

This comment has been minimized.

Show comment
Hide comment
@conor10

conor10 Oct 30, 2017

Collaborator
Collaborator

conor10 commented Oct 30, 2017

@IgorPerikov

This comment has been minimized.

Show comment
Hide comment
@IgorPerikov

IgorPerikov Nov 9, 2017

Contributor

Hi, @conor10. I am closing this pr in favour of #232

Contributor

IgorPerikov commented Nov 9, 2017

Hi, @conor10. I am closing this pr in favour of #232

@IgorPerikov IgorPerikov closed this Nov 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment