[ECR-2914] LC: Implemented get transaction endpoint support #725

bullet-tooth · 2019-02-20T14:48:14Z

Overview

See: https://jira.bf.local/browse/ECR-2914

TransactionResult moved to the common module.
TransactionLocation moved to the common module.
Created JSON serializer for TransactionMessage.
Implemented get transaction endpoint support

Definition of Done

There are no TODOs left in the code
Change is covered by automated tests
The coding guidelines are followed
Public API has Javadoc
Method preconditions are checked and documented in the Javadoc of the method
Changelog is updated if needed (in case of notable or breaking changes)
The continuous integration build passes

bullet-tooth · 2019-02-20T14:57:17Z

...main/java/com/exonum/binding/common/serialization/json/TransactionMessageJsonSerializer.java

+import com.google.gson.JsonSerializer;
+import java.lang.reflect.Type;
+
+final class TransactionMessageJsonSerializer implements JsonSerializer<TransactionMessage>,


Are there any needs for TransactionMessage serialization to JSON in other ways?

Can't say 🤷‍♂️ @vitvakatu , @MakarovS ?

I'd leave it as it is for now.

bullet-tooth · 2019-02-20T14:58:24Z

...va-binding/common/src/main/java/com/exonum/binding/common/transaction/TransactionResult.java

@@ -32,8 +30,6 @@
 * if execution has failed.
 * Errors might be either service-defined or unexpected. Service-defined errors consist of an error
 * code and an optional description. Unexpected errors include only a description.
- *
- * @see TransactionExecutionException


the link becomes broken because TransactionExecutionException locates in the core module.

I see, would you add a back reference to TransactionExecutionException, if there isn't one already?

I added a reference at TransactionExecutionException to this class

coveralls · 2019-02-20T15:27:22Z

Coverage decreased (-0.003%) to 85.499% when pulling 5c2ed59 on ecr-2914-2 into 68871c8 on master.

MakarovS

👍

MakarovS · 2019-02-20T18:07:22Z

exonum-light-client/src/main/java/com/exonum/client/ExonumHttpClient.java

+      if (response.code() == HTTP_NOT_FOUND) {
+        return Optional.empty();
+      } else if (!response.isSuccessful()) {
+        throw new RuntimeException("Execution wasn't success: " + response.toString());


Either successful or a success

MakarovS · 2019-02-20T18:08:29Z

exonum-light-client/src/main/java/com/exonum/client/ExonumHttpClient.java

+  private String blockingExecutePlainText(Request request) {
+    return blockingExecute(request, response -> {
+      if (!response.isSuccessful()) {
+        throw new RuntimeException("Execution wasn't success: " + response.toString());


MakarovS · 2019-02-20T18:09:56Z

exonum-light-client/src/main/java/com/exonum/client/ExplorerApiHelper.java

+    } else if (executionStatus.getType() == GetTxResponseExecutionStatus.ERROR) {
+      return TransactionResult.error(executionStatus.getCode(), executionStatus.getDescription());
+    } else {
+      throw new IllegalArgumentException("Unexpected transaction execution status"


Some delimiter at the end?

dmitry-timofeev

Good, thanks 👍

I'd ponder on some questions/suggestions below.

dmitry-timofeev · 2019-02-21T10:04:33Z

...inding/common/src/main/java/com/exonum/binding/common/serialization/json/JsonSerializer.java

        .registerTypeAdapter(PublicKey.class, new PublicKeyJsonSerializer())
        .registerTypeAdapterFactory(StoredConfigurationAdapterFactory.create())
-        .setLongSerializationPolicy(LongSerializationPolicy.STRING);
+        .setLongSerializationPolicy(LongSerializationPolicy.STRING)


Nit: I'd possibly group related things together (type adapters, then long serialization policy, or vice-versa).

dmitry-timofeev · 2019-02-21T10:06:54Z

...va-binding/common/src/main/java/com/exonum/binding/common/transaction/TransactionResult.java

@@ -32,8 +30,6 @@
 * if execution has failed.
 * Errors might be either service-defined or unexpected. Service-defined errors consist of an error
 * code and an optional description. Unexpected errors include only a description.
- *
- * @see TransactionExecutionException


I see, would you add a back reference to TransactionExecutionException, if there isn't one already?

dmitry-timofeev · 2019-02-21T10:08:11Z

...ng/common/src/test/java/com/exonum/binding/common/serialization/json/JsonSerializerTest.java

  }

+  @Test
+  void transactionMessageSerializesAsValue() {


~~Shan't we make round-trip tests?~~

There is one below.

dmitry-timofeev · 2019-02-21T10:10:28Z

.../java/com/exonum/binding/common/serialization/json/TransactionMessageJsonSerializerTest.java

+  @ParameterizedTest
+  @MethodSource("source")
+  void roundTripTest(TransactionMessage msg) {
+    String json = json().toJson(msg);


This test relies on the fact that JsonSerializer includes a TransactionMessage adapter, correct? Shan't we use Gson here directly? Or move this test to JsonSerializerTest? I'd prefer to keep it here, separate.

dmitry-timofeev · 2019-02-21T10:15:20Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

  String getUserAgentInfo();

+  /**
+   * Returns transaction with current status. Or {@code Optional.empty()} if


Returns the information about the transaction; or {@code Optional.empty()} if ??

dmitry-timofeev · 2019-02-21T10:22:25Z

exonum-light-client/src/test/java/com/exonum/client/ExonumHttpClientIntegrationTest.java

-    byte[] actualMessageBytes = HEX_ENCODER.decode(encodedTxMessage);
-    TransactionMessage actualTxMessage = TransactionMessage.fromBytes(actualMessageBytes);
-
-    assertThat(actualTxMessage, is(txMessage));


Don't we longer check that we sent a properly encoded transaction?

we rely on Gson instance here. But I agree, let's have some checks in case of regression.

dmitry-timofeev · 2019-02-21T10:23:02Z

exonum-light-client/src/test/java/com/exonum/client/ExonumHttpClientIntegrationTest.java

  }
+
+  @Test
+  void getTransactionNotFound() throws InterruptedException {


Shan't we add a happy result?

dmitry-timofeev · 2019-02-21T10:24:05Z

exonum-light-client/src/test/java/com/exonum/client/ExplorerApiHelperTest.java

+    TransactionResponse transactionResponse = ExplorerApiHelper.parseGetTxResponse(json);
+
+    assertThat(transactionResponse.getStatus(), is(TransactionStatus.IN_POOL));
+    assertThat(transactionResponse.getMessage(), notNullValue());


Shan't we extract the message as a constant and assert that we get exactly that?

dmitry-timofeev · 2019-02-21T10:32:08Z

exonum-light-client/src/main/java/com/exonum/client/response/TransactionResponse.java

+  @NonNull
+  TransactionMessage message;
+  /**
+   * Transaction execution result; {@code null} - for in-pool transactions.


What if we define the getter that throws IllegalStateE instead? That will roughly align the behaviour of this class with Optional.

Alternatively we can return Optional<TransactionResult|TransactionLocation>, but in that case the clients who have checked the status would be penalized with an additional ifPresent/isPresent + get.

What do you think would be more convenient:

null

Optional

not-null with a required check of the status?

By going with the third approach you will also need to check the status before calling getter. Only one thing that can be considered as a proc (comparing with the first approach) is to have some error description instead of NPE

That's not one thing:

You get an error early, rather than in undefined time range. And yes, it has an error description.

There is a clear connection between the tx status and possible absence of a value — you don't have to check both values for nulls "just in case" if you have checked if the transaction is committed.

dmitry-timofeev · 2019-02-21T10:33:37Z

...main/java/com/exonum/binding/common/serialization/json/TransactionMessageJsonSerializer.java

+import com.google.gson.JsonSerializer;
+import java.lang.reflect.Type;
+
+final class TransactionMessageJsonSerializer implements JsonSerializer<TransactionMessage>,


Can't say 🤷‍♂️ @vitvakatu , @MakarovS ?

dmitry-timofeev · 2019-02-22T07:44:45Z

exonum-light-client/src/main/java/com/exonum/client/ExplorerApiHelper.java

      return TransactionResult.successful();
    } else if (executionStatus.getType() == GetTxResponseExecutionStatus.ERROR) {
      return TransactionResult.error(executionStatus.getCode(), executionStatus.getDescription());
+    } else if (executionStatus.getType() == GetTxResponseExecutionStatus.PANIC) {


Is there a test for that?

dmitry-timofeev

👍 , I'd also consider adding isCommitted

dmitry-timofeev · 2019-02-23T10:45:50Z

exonum-light-client/src/main/java/com/exonum/client/response/TransactionResponse.java

+   * @throws IllegalStateException if the transaction is not committed yet
+   */
+  public TransactionResult getExecutionResult() {
+    checkState(status == COMMITTED,


I think we must add such a convenience method if we require the clients to check for transaction status:

/** * Returns true if this transaction is {@linkplain TransactionStatus#COMMITTED committed} to the blockchain; * false — otherwise. */ boolean isCommitted();

dmitry-timofeev · 2019-02-23T10:46:37Z

exonum-light-client/src/main/java/com/exonum/client/response/TransactionResponse.java

+  TransactionLocation location;
+
+  /**
+   * Returns transaction execution result.


+ Not available unless the transaction is {@linkplain #isCommitted committed} to the blockchain?

dmitry-timofeev · 2019-02-23T10:48:39Z

exonum-light-client/src/main/java/com/exonum/client/response/TransactionStatus.java

+  @SerializedName("in-pool")
+  IN_POOL,
+  /**
+   * Shows that transaction is committed to the blockchain.


+

Please note that a committed transaction has not necessarily completed successfully — use the {@linplain TransactionResult execution result} to check that.

dmitry-timofeev

💯

Looks great (but check please if changelog needs to be updated)

bullet-tooth added 3 commits February 20, 2019 16:09

move tx classes

5f605bd

created tx message serializer

f2ab026

implemented get tx endpoint

5dfcab0

bullet-tooth requested review from MakarovS and dmitry-timofeev February 20, 2019 14:48

bullet-tooth mentioned this pull request Feb 20, 2019

[ECR-2914] Implemented get transaction endpoint support #724

Closed

7 tasks

bullet-tooth commented Feb 20, 2019

View reviewed changes

bullet-tooth requested a review from dip56 February 20, 2019 14:58

rm redundant dependency

4803bf0

MakarovS reviewed Feb 20, 2019

View reviewed changes

bullet-tooth added 2 commits February 21, 2019 08:57

PR comments fix

8d87e4d

Merge branch 'master' into ecr-2914-2

115cf81

dmitry-timofeev approved these changes Feb 21, 2019

View reviewed changes

bullet-tooth added 4 commits February 21, 2019 15:01

PR comments fix

f349175

PR comments fix

083c769

added panic status

4e8a354

mackage name refactoring

152f989

MakarovS approved these changes Feb 22, 2019

View reviewed changes

dmitry-timofeev reviewed Feb 22, 2019

View reviewed changes

dmitry-timofeev self-requested a review February 22, 2019 07:47

bullet-tooth added 3 commits February 22, 2019 11:50

optimize imports

a131385

Merge branch 'master' into ecr-2914-2

8ccc715

fix PR comments

06dd091

dmitry-timofeev reviewed Feb 23, 2019

View reviewed changes

dmitry-timofeev and others added 2 commits February 23, 2019 12:49

Merge branch 'master' into ecr-2914-2

a657e81

fix PR comments

d1ad7d1

dmitry-timofeev approved these changes Feb 25, 2019

View reviewed changes

changelog update

5c2ed59

dmitry-timofeev approved these changes Feb 25, 2019

View reviewed changes

dmitry-timofeev merged commit f8fbcdf into master Feb 25, 2019

dmitry-timofeev deleted the ecr-2914-2 branch February 25, 2019 09:26

[ECR-2914] LC: Implemented get transaction endpoint support #725

[ECR-2914] LC: Implemented get transaction endpoint support #725

Uh oh!

Conversation

bullet-tooth commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MakarovS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-timofeev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-timofeev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-timofeev Feb 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-timofeev left a comment

Choose a reason for hiding this comment

Uh oh!

bullet-tooth commented Feb 20, 2019 •

edited

Loading

coveralls commented Feb 20, 2019 •

edited

Loading

dmitry-timofeev Feb 23, 2019 •

edited

Loading