Add transaction methods extraction class [ECR-3923] #1274

MakarovS · 2019-12-07T04:50:18Z

Overview

Add transaction methods extraction class. It will be used by ServiceWrapper which would store a map of transaction methods and invoke them by an id.

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

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

MakarovS · 2019-12-07T16:21:24Z

...nding/core/src/test/java/com/exonum/binding/core/runtime/TransactionMethodExtractorTest.java

+    MethodHandle methodHandle = lookup.unreflect(transactionMethod);
+    assertThat(transactions).hasSize(1);
+    MethodHandle actualMethodHandle = transactions.get(ValidService.TRANSACTION_ID);
+    assertThat(actualMethodHandle.toString()).isEqualTo(methodHandle.toString());


I had assertThat(transactions).containsExactly(entry(ValidService.TRANSACTION_ID, methodHandle)); initially, but methodHandles are not equal to each other. Is there a better way to check this?

If invocation of the transaction method were implemented as the task specifies,
it could be tested by invoking the operation and verifying that it was indeed invoked:

Object service = spy(new Service()); TransactionInvoker invoker = new TransactionInvoker(service); invoker.invoke(id, context, bytes); verify(service).operation(context, bytes);

coveralls · 2019-12-07T17:13:35Z

Coverage increased (+0.1%) to 86.867% when pulling a5cbb3a on ECR-3923 into ce2174e on master.

dmitry-timofeev

There are no tests for extraction of annotated methods (class hierarchies, annotation applied to interface methods — e.g., our QaService).
There is no actual working invocation of the methods.
Tests for invocation must be extended to ensure the implementation is correct:

Verify the invocation target
Cover more than a single transaction method.

dmitry-timofeev · 2019-12-09T12:27:51Z

...m-java-binding/core/src/main/java/com/exonum/binding/core/transaction/TransactionMethod.java

+import java.lang.annotation.Target;
+
+/**
+ * Indicates that a method is a transaction method. The annotated method should:


The documentation is inadequate — see the Transaction and Transaction#execute
(since this annotation applies to methods, it must be documented appropriately).

dmitry-timofeev · 2019-12-09T12:28:03Z

...m-java-binding/core/src/main/java/com/exonum/binding/core/transaction/TransactionMethod.java

+  /**
+   * Returns the transaction type identifier which is unique within the service.
+   */
+  int id();


As there are no other properties than 'id', it is sensible to use 'value' instead.

dmitry-timofeev · 2019-12-09T12:29:54Z

...a-binding/core/src/main/java/com/exonum/binding/core/runtime/TransactionMethodExtractor.java

+   *
+   * @see TransactionMethod
+   */
+  static Map<Integer, MethodHandle> extractTransactionMethods(Class<?> serviceClass) {


What's the point of producing method handles of unknown signature? Is it
(byte[], TransactionContext) or (TransactionContext, byte[])? The caller won't be able
to determine that. It also won't work with arbitrary protobuf messages as arguments even if the order becomes fixed.

I think some object that can be invoked with context and byte[] and invoke the corresponding transaction method is needed.

dmitry-timofeev · 2019-12-09T12:30:26Z

...a-binding/core/src/main/java/com/exonum/binding/core/runtime/TransactionMethodExtractor.java

+  static Map<Integer, MethodHandle> extractTransactionMethods(Class<?> serviceClass) {
+    Map<Integer, MethodHandle> transactions = new HashMap<>();
+    MethodHandles.Lookup lookup = MethodHandles.lookup();
+    while (serviceClass != Object.class) {


This method does several things making testing more complicated:

Finds all methods annotated with a particular annotation

Validates these annotations

Converts to something callable

At least the first step, contributing to this outer loop, must be extracted and tested
separately.

dmitry-timofeev · 2019-12-09T12:31:23Z

...a-binding/core/src/main/java/com/exonum/binding/core/runtime/TransactionMethodExtractor.java

+  private static void validateTransactionMethod(Method transaction, Class<?> serviceClass,
+      Map<Integer, MethodHandle> transactions, int transactionId) {
+    checkArgument(!transactions.containsKey(transactionId),
+        "Service %s had more than one transaction with id: %s", serviceClass.getName(),


Service %s has more than one transaction with the same id (%s): first: %s; second: %s

dmitry-timofeev · 2019-12-09T12:36:18Z

...a-binding/core/src/main/java/com/exonum/binding/core/runtime/TransactionMethodExtractor.java

+        transaction.getName(), serviceClass.getName());
+    checkArgument(transaction.getParameterCount() == 2, errorMessage);
+    for (Class<?> parameterType: transaction.getParameterTypes()) {
+      checkArgument(isParameterTypeValid(parameterType), errorMessage);


Include the invalid parameterType. The present message might be enough now, but including the 'bad' parameter will become more important when we allow more than byte[]

dmitry-timofeev · 2019-12-09T12:36:37Z

...a-binding/core/src/main/java/com/exonum/binding/core/runtime/TransactionMethodExtractor.java

+        + " \"byte[]\" and \"com.exonum.binding.core.transaction.TransactionContext\"",
+        transaction.getName(), serviceClass.getName());
+    checkArgument(transaction.getParameterCount() == 2, errorMessage);
+    for (Class<?> parameterType: transaction.getParameterTypes()) {


This code accepts (byte[], byte[]) but shouldn't. Please test that properly.

dmitry-timofeev · 2019-12-09T12:37:53Z

...nding/core/src/test/java/com/exonum/binding/core/runtime/TransactionMethodExtractorTest.java

+  @Test
+  void validServiceMethodExtraction() throws Exception {
+    Map<Integer, MethodHandle> transactions =
+        TransactionMethodExtractor.extractTransactionMethods(ValidService.class);


I think local classes will be quite appropriate.

dmitry-timofeev · 2019-12-09T12:39:05Z

...nding/core/src/test/java/com/exonum/binding/core/runtime/TransactionMethodExtractorTest.java

+    MethodHandle methodHandle = lookup.unreflect(transactionMethod);
+    assertThat(transactions).hasSize(1);
+    MethodHandle actualMethodHandle = transactions.get(ValidService.TRANSACTION_ID);
+    assertThat(actualMethodHandle.toString()).isEqualTo(methodHandle.toString());


If invocation of the transaction method were implemented as the task specifies,
it could be tested by invoking the operation and verifying that it was indeed invoked:

Object service = spy(new Service()); TransactionInvoker invoker = new TransactionInvoker(service); invoker.invoke(id, context, bytes); verify(service).operation(context, bytes);

dmitry-timofeev · 2019-12-09T12:40:39Z

...nding/core/src/test/java/com/exonum/binding/core/runtime/TransactionMethodExtractorTest.java

+    Exception e = assertThrows(IllegalArgumentException.class,
+        () -> TransactionMethodExtractor
+            .extractTransactionMethods(MissingTransactionMethodArgumentsService.class));
+    Method transactionMethod =


(here and elsewhere) I don't think it is appropriate to use reflection to only specify the expected method name.

dmitry-timofeev · 2019-12-12T16:42:31Z

...m-java-binding/core/src/main/java/com/exonum/binding/core/transaction/TransactionMethod.java

+ * occurs. A correct transaction implementation must not throw such exceptions. The transaction
+ * will be committed as failed (status "panic").
+ *
+ * @see <a href="https://exonum.com/doc/version/0.12/architecture/transactions">Exonum Transactions</a>


0.13-rc.2, see https://wiki.bf.local/display/EJB/Java+Binding+0.9.0-rc1+Release+Checklist

dmitry-timofeev

Generally looks good! Also, we are almost with transactions-as-methods with no explicit TransactionConverter and de-serialization 🎉

dmitry-timofeev · 2019-12-12T17:35:02Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/TransactionInvoker.java

+  }
+
+  /**
+   * Execute the transaction, possibly modifying the blockchain state.


I feel that the copy-pasted user-facing documentation is not appropriate here.
This one may be more concise and bring a different message (what it does for which Service
implementations, not what it shall do as an interface implementation). The spec is different, and the target audience is completely different.

dmitry-timofeev · 2019-12-12T17:36:40Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/TransactionInvoker.java

+   */
+  void invokeTransaction(int transactionId, byte[] arguments, TransactionContext context)
+      throws TransactionExecutionException {
+    checkArgument(transactionMethods.containsKey(transactionId),


(Meta) We must extend the task on errors to cover routing errors — this is an instance of such error.

dmitry-timofeev · 2019-12-12T17:37:15Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/TransactionInvoker.java

+    checkArgument(transactionMethods.containsKey(transactionId),
+        "No method with transaction id (%s)", transactionId);
+    try {
+      MethodHandle methodHandle = transactionMethods.get(transactionId);


That's fine for this PR, but I'd remind that MHs won't work for any message (next task):

The caller won't be able
to determine that. It also won't work with arbitrary messages as arguments even if
the order is fixed.

dmitry-timofeev · 2019-12-12T17:37:48Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/TransactionInvoker.java

+      if (throwable instanceof TransactionExecutionException) {
+        throw (TransactionExecutionException) throwable;
+      } else {
+        throw new RuntimeException(throwable);


(Possibly for future improvement) The MH#invoke spec says

Throws:
Throwable – anything thrown by the underlying method propagates unchanged through the method handle call

So why don't we forbid checked exceptions in transaction methods (except TransactionExecutionException)
during discovery and throw RuntimeExceptions as is on invocation?

Please create an issue for that.

Created ECR-3988

dmitry-timofeev · 2019-12-12T17:39:10Z

...-java-binding/core/src/test/java/com/exonum/binding/core/runtime/TransactionInvokerTest.java

+        () -> invoker.invokeTransaction(ThrowingService.TRANSACTION_ID, ARGUMENTS, context));
+    assertThat(e.getErrorCode()).isEqualTo(ThrowingService.ERROR_CODE);
+  }
+


Please test handling of 'other' service originated exceptions, since it is important to handle them properly.

May add a second method to 'ThrowingService'.

dmitry-timofeev · 2019-12-12T17:39:31Z

...-java-binding/core/src/test/java/com/exonum/binding/core/runtime/TransactionInvokerTest.java

+    assertThat(e.getErrorCode()).isEqualTo(ThrowingService.ERROR_CODE);
+  }
+
+  class BasicService implements Service {


The classes below are unnecessarily nested classes of the test class — shall be made static.

dmitry-timofeev · 2019-12-12T17:40:50Z

...m-java-binding/core/src/main/java/com/exonum/binding/core/transaction/TransactionMethod.java

+ * transaction, possibly modifying the blockchain state. The method should:
+ * <ul>
+ *   <li>be public
+ *   <li>have exactly two parameters of types 'byte[]' and '{@link TransactionContext}' in this


Parameter documentation (most importantly, what is byte[], also what is context for)?
The com.exonum.binding.core.service.TransactionConverter#toTransaction(int, byte[]) and Transaction#execute have it correspondingly.

dmitry-timofeev · 2019-12-12T17:41:18Z

...a-binding/core/src/main/java/com/exonum/binding/core/runtime/TransactionMethodExtractor.java

+   */
+  static Map<Integer, MethodHandle> extractTransactionMethods(Class<?> serviceClass) {
+    Map<Integer, Method> transactionMethods = findTransactionMethods(serviceClass);
+    Lookup lookup = MethodHandles.lookup();


this code uses relative lookup (relative to this class) but does not need it (it
doesn't access any private or package-private members, for instance, which such call-site sensitive
lookup provides). Why don't we use
publicLookup (likely, 'relative' to the defining class to satisfy class-loading constraints
we bumped into earlier with protobuf reflective serializer)?

dmitry-timofeev · 2019-12-12T17:42:22Z

...a-binding/core/src/main/java/com/exonum/binding/core/runtime/TransactionMethodExtractor.java

+  private static void checkDuplicates(Map<Integer, Method> transactionMethods, int transactionId,
+      Class<?> serviceClass, Method method) {
+    if (transactionMethods.containsKey(transactionId)) {
+      String errorMessage = String.format("Service %s has more than one transaction with the same"


(Nit): this one is actually first (given the order of traversal). Also, variable.

Suggested change

String errorMessage = String.format("Service %s has more than one transaction with the same"

String firstMethodName = transactionMethods.get(transactionId).getName();

String errorMessage = String.format("Service %s has more than one transaction with the same"

+ " id (%s): first: %s; second: %s",

serviceClass.getName(), transactionId, firstMethodName, method.getName());

dmitry-timofeev · 2019-12-12T17:43:42Z

...nding/core/src/test/java/com/exonum/binding/core/runtime/TransactionMethodExtractorTest.java

+    static final int TRANSACTION_ID_2 = 2;
+
+    @TransactionMethod(TRANSACTION_ID)
+    public void transactionMethod(byte[] arguments, TransactionContext context) {}


I considered this case (the present duplicates the annotation which isn't something that shall happen, I think):

interface ServiceInterface { @TransactionMethod(1) // <-- Annotation is present on the interface void transactionMethod(byte[] arguments, TransactionContext context); } class ServiceInterfaceImpl implements ServiceInterface { @Override // <-- NO duplicate TransactionMethod annotation! public void transactionMethod(byte[] arguments, TransactionContext context) {} }

I think it is better to offload this to a sep. issue because there are more to that: e.g., what if an interface method and the implementation method have different @TransactionMethods?

Please document the considerations in a new issue.

Created ECR-3989

dmitry-timofeev · 2019-12-12T17:49:35Z

...nding/core/src/test/java/com/exonum/binding/core/runtime/TransactionMethodExtractorTest.java

+
+    @TransactionMethod(TRANSACTION_ID_2)
+    @SuppressWarnings("WeakerAccess") // Should be accessible
+    public void transactionMethod2(byte[] arguments, TransactionContext context) {}


A bike-shed target (@slowli , @bullet-tooth , @MakarovS, @vitvakatu )! Shall we require the arguments to be in order:

(MessageT arguments, TransactionContext context)

(TransactionContext context, MessageT arguments)

Allow any (mind extra impl. complexity!)

I think there is no need in arbitrary order. The either one works, but Rust runtime uses context, arguments one.

I agree with allowing any order, there'd be a couple more lines of code in implementation, but less unnecessary restrictions for users.
Created ECR-3990

I agree that it is not badly needed, and any could work: in some cases, the arguments is the first argument used (e.g., to check preconditions); in some cases, the context (e.g., to instantiate the schema and check the preconditions depending on the state, or to just update it).

No particular preference. From the unification perspective, context, arguments ordering makes sense, but AFAIK, this is not the Java way™. Since reflection is used to handle the processing, restricting the ordering of args is not necessary. (Further, it could make sense to allow to extract commonly used parts of the context into args, e.g., the service schema and the caller.)

ECR-3923 Add transaction methods extraction class

676f81e

MakarovS requested review from bullet-tooth and dmitry-timofeev December 7, 2019 04:50

MakarovS added 3 commits December 7, 2019 00:10

ECR-3923 Add Javadoc for TransactionMethod

a4ded9d

Merge remote-tracking branch 'remotes/origin/master' into ECR-3923

db470fb

ECR-3923 Fix some TODOs

8ca1f17

MakarovS commented Dec 7, 2019

View reviewed changes

ECR-3923 Checkstyle fix

9552f5b

dmitry-timofeev suggested changes Dec 9, 2019

View reviewed changes

MakarovS added 3 commits December 12, 2019 00:49

ECR-3923 Review fixes

33dd226

ECR-3923 Update tests

1868419

Merge remote-tracking branch 'remotes/origin/master' into ECR-3923

d47abff

dmitry-timofeev reviewed Dec 12, 2019

View reviewed changes

dmitry-timofeev suggested changes Dec 12, 2019

View reviewed changes

dmitry-timofeev reviewed Dec 12, 2019

View reviewed changes

MakarovS added 3 commits December 13, 2019 13:57

Merge remote-tracking branch 'remotes/origin/master' into ECR-3923

64b753d

ECR-3923 Review fixes

c14a62d

ECR-3923 Minor fix

b1977f8

dmitry-timofeev approved these changes Dec 16, 2019

View reviewed changes

MakarovS added 3 commits December 16, 2019 13:53

ECR-3923 Checkstyle fix

67c794f

Merge remote-tracking branch 'remotes/origin/master' into ECR-3923

76bf972

ECR-3923 Fix tests

a5cbb3a

dmitry-timofeev merged commit 1e322c6 into master Dec 17, 2019

dmitry-timofeev deleted the ECR-3923 branch December 17, 2019 11:25

-      String errorMessage = String.format("Service %s has more than one transaction with the same"
+      String firstMethodName = transactionMethods.get(transactionId).getName();
+      String errorMessage = String.format("Service %s has more than one transaction with the same"
+              + " id (%s): first: %s; second: %s",
+          serviceClass.getName(), transactionId, firstMethodName, method.getName());

Add transaction methods extraction class [ECR-3923] #1274

Add transaction methods extraction class [ECR-3923] #1274

Uh oh!

Conversation

MakarovS commented Dec 7, 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!

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

Uh oh!

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!

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!

MakarovS Dec 16, 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!

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!

MakarovS commented Dec 7, 2019 •

edited

Loading

coveralls commented Dec 7, 2019 •

edited

Loading

MakarovS Dec 16, 2019 •

edited

Loading