Add a transactional parameter to @Action

[CydeWeys]

This starts the process of removing inner transactions on some Registrar methods with some resultant refactoring, but then I gave up after awhile and turned them into reTransact() calls just to be able to get in the Action changes quickly.

---

This change is 

[CydeWeys]

I really wish there were a way to make these 3 repeated calls nicer, but I couldn't find any. You can't extract to a helper function that takes a lambda.

[jianglai]

It seems like most of the change are to various commands. Maybe you should consider changing the title of the PR to reflect that.

Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys)

---

core/src/main/java/google/registry/request/Action.java line 63 at r1 (raw file):

  /** Whether to create a wrapping DB transaction around the execution of the action. */
  boolean transactional() default true;

Are we sure we want to set the default to true? Did we survey the actions and conclude that the majority of them need to be wrapped in a transaction in their entirety?

---

core/src/main/java/google/registry/tools/ConfirmingCommand.java line 38 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I really wish there were a way to make these 3 repeated calls nicer, but I couldn't find any. You can't extract to a helper function that takes a lambda.

The root problem is that tm().transact() expects a Runnable, whose run() method doesn't declare a checked exception, so your lambda cannot either.

You can get around it by making your lambda a Callable<Void>, then feeding it to a wrapper method that converts it back to a Runnable but rethrows the checked exception as unchecked. The key here is that the Callable.call() method declares a checked exception, so you are allowed to supply a lambda that throws a checked exception.

So it would be like this:

private static Runnable wrapper(Callable<Void> callable) {  
  return new Runnable() {  
    @Override  
    public void run() {  
      try {  
        callable.call();  
      } catch (Exception e) {  
        rethrow(e);  
      }  
    }  
  };  
}

tm().transact(wrapper(  
    () -> {  
      init();  
      printLineIfNotEmpty(prompt());  
      return null;  
    }));

Whether it is preferable to just do the rethrow manually is up for debate.

Alternatively, I see no reason why we cannot just have tm().transact() take a Callable<T> instead of a Supplier<T> (which doesn't allow checked exceptions), so you don't need a wrapper method and can just do:

tm().transact(  
        () -> {  
          init();  
          printLineIfNotEmpty(prompt());  
          return null;  
        });

We can maybe go a step further and define our own ThrowableRunnable that allows throwing, and make tm().transact() take it instead of the vanilla Runnable, in which case you can just provide the lambda as is.

It seems to me that there really isn't a need to force the caller to handle checked exceptions when supplying a lambda to tm().transact(), if all they do is to just re-throw them as unchecked. tm().transact() wraps the work in its own try block so any exception thrown by the workload would be handled by the transaction manager anyway.

---

core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java line 221 at r1 (raw file):

  private Map<String, Object> read(String registrarId) {
    return tm().transact(

What's wrong with just wrapping loadRegistrarUnchecked(registrarId) inside the transaction? That way you don't need to change the return value here.

Also, the entire RegistrarResult class seems redundant at this point, since you are just calling the toJsonResponse() method on it to get the map anyway.

---

core/src/test/java/google/registry/whois/WhoisCommandFactoryTest.java line 100 at r1 (raw file):

            () -> {
              WhoisResponse response =
                  assertDoesNotThrow(

Why do we need this assertion? If the lambda throws, wouldn't the whole test method fail anyway?

[CydeWeys]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jianglai)

---

core/src/main/java/google/registry/request/Action.java line 63 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Are we sure we want to set the default to true? Did we survey the actions and conclude that the majority of them need to be wrapped in a transaction in their entirety?

Yes, I did survey the actions and determine that almost all of them are transactional.

[CydeWeys]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jianglai)

---

core/src/test/java/google/registry/whois/WhoisCommandFactoryTest.java line 100 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Why do we need this assertion? If the lambda throws, wouldn't the whole test method fail anyway?

If the lambda throws then it can't be passed to the lambda in transact(). The checked exception needs to be handled. assertDoesNotThrow() handles the checked exception and does not rethrow a checked exception.

[CydeWeys]

Reviewable status: 10 of 15 files reviewed, 4 unresolved discussions (waiting on @jianglai)

---

core/src/main/java/google/registry/tools/ConfirmingCommand.java line 38 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

The root problem is that tm().transact() expects a Runnable, whose run() method doesn't declare a checked exception, so your lambda cannot either.

You can get around it by making your lambda a Callable<Void>, then feeding it to a wrapper method that converts it back to a Runnable but rethrows the checked exception as unchecked. The key here is that the Callable.call() method declares a checked exception, so you are allowed to supply a lambda that throws a checked exception.

So it would be like this:

private static Runnable wrapper(Callable<Void> callable) {  
  return new Runnable() {  
    @Override  
    public void run() {  
      try {  
        callable.call();  
      } catch (Exception e) {  
        rethrow(e);  
      }  
    }  
  };  
}

tm().transact(wrapper(  
    () -> {  
      init();  
      printLineIfNotEmpty(prompt());  
      return null;  
    }));

Whether it is preferable to just do the rethrow manually is up for debate.

Alternatively, I see no reason why we cannot just have tm().transact() take a Callable<T> instead of a Supplier<T> (which doesn't allow checked exceptions), so you don't need a wrapper method and can just do:

tm().transact(  
        () -> {  
          init();  
          printLineIfNotEmpty(prompt());  
          return null;  
        });

We can maybe go a step further and define our own ThrowableRunnable that allows throwing, and make tm().transact() take it instead of the vanilla Runnable, in which case you can just provide the lambda as is.

It seems to me that there really isn't a need to force the caller to handle checked exceptions when supplying a lambda to tm().transact(), if all they do is to just re-throw them as unchecked. tm().transact() wraps the work in its own try block so any exception thrown by the workload would be handled by the transaction manager anyway.

Thanks for your ThrowingRunnable PR, this is nicer now.

[jianglai]

Reviewed 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys)

---

core/src/main/java/google/registry/request/Action.java line 63 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Yes, I did survey the actions and determine that almost all of them are transactional.

See my other comment below, EppTlsAction needs to be marked as non-transactional, and there needs to be another field to specify the transaction isolation level override, if any.

---

core/src/main/java/google/registry/request/RequestHandler.java line 161 at r3 (raw file):

      Runnable action = route.get().instantiator().apply(component);
      if (route.get().action().transactional()) {
        tm().transact(action::run);

Now that we have a good understanding on how to handle nested transactions, I have some concern on how this would work together with the transaction isolation level override in the long run.

The override can only be applied at the top level transaction, which would be here. We therefore need to allow setting the override (possibly via another field in the@Action annotation).

However, all EPP flows are executed by EppTlsAction, and we have envisioned using the @IsolationLevel annotation on a flow to signal to the FlowRunner.

In order to allow different EPP flows to use different isolation levels, we need to annotate the EppTlsAction as non-transactional, and define the transaction behavior at the flow level. This is doable, but needs to be clearly documented to avoid confusion.

---

core/src/main/java/google/registry/tools/ConfirmingCommand.java line 38 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Thanks for your ThrowingRunnable PR, this is nicer now.

It's it sooooo much nicer!

---

core/src/test/java/google/registry/whois/WhoisCommandFactoryTest.java line 100 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

If the lambda throws then it can't be passed to the lambda in transact(). The checked exception needs to be handled. assertDoesNotThrow() handles the checked exception and does not rethrow a checked exception.

Does it still apply now that we can throw checked exceptions in the lambda supplied to tm().transact()? It will just be wrapped in a RuntimeException if it throws, correct?