Painful exception handling

[Hans Ridder]

Sunk a few hours tracking down this problem....

At https://github.com/google/nomulus/blob/master/java/google/registry/flows/domain/BaseDomainCreateFlow.java#L127 , you'll find code like the following:
 

  protected String createFlowRepoId() {
    // The domain name hasn't been validated yet, so if it's invalid, instead of throwing an error,
    // simply leave the repoId blank (it won't be needed anyway as the flow will fail when
    // validation fails later).
    try {
      Optional<InternetDomainName> tldParsed =
          findTldForName(InternetDomainName.from(command.getFullyQualifiedDomainName()));
      return tldParsed.isPresent()
          ? createDomainRoid(ObjectifyService.allocateId(), tldParsed.get().toString())
          : null;
    } catch (IllegalArgumentException e) {
      return null;
    }
  }

We managed to get duplicate ROID suffixes (null) in two TLDs, which causes createDomainRoid() to die with IllegalArgumentException, like so:

java.lang.IllegalArgumentException: value already present: null
        at com.google.common.collect.HashBiMap.put(HashBiMap.java:282)
        at com.google.common.collect.HashBiMap.put(HashBiMap.java:258)
        at google.registry.model.RoidSuffixes$1$1.run(RoidSuffixes.java:40)
        at google.registry.model.RoidSuffixes$1$1.run(RoidSuffixes.java:34)
        at google.registry.model.ofy.Ofy.doTransactionless(Ofy.java:289)
        at google.registry.model.RoidSuffixes$1.get(RoidSuffixes.java:34)
        at google.registry.model.RoidSuffixes$1.get(RoidSuffixes.java:31)
        at com.google.common.base.Suppliers$ExpiringMemoizingSupplier.get(Suppliers.java:199)
        at google.registry.model.RoidSuffixes.getRoidSuffixForTld(RoidSuffixes.java:55)
        at google.registry.model.EppResourceUtils.createDomainRoid(EppResourceUtils.java:58)
        at google.registry.flows.domain.BaseDomainCreateFlow.createFlowRepoId(BaseDomainCreateFlow.java:126)
        at google.registry.flows.ResourceCreateFlow.initRepoId(ResourceCreateFlow.java:49)

However, you wouldn't get to see that stack trace because of the catch in createFlowRepoId(). Instead, you'll get something like below because of the time-bomb created by the catch:

google.registry.flows.FlowRunner run: java.lang.IllegalStateException: RepoId hasn't been initialized yet; getResourceKey() called too early (FlowRunner.java:138)
        at com.google.common.base.Preconditions.checkState(Preconditions.java:174)
        at google.registry.flows.ResourceCreateOrMutateFlow.getResourceKey(ResourceCreateOrMutateFlow.java:96)
        at google.registry.flows.ResourceCreateFlow.createOrMutateResource(ResourceCreateFlow.java:70)
        at google.registry.flows.ResourceCreateOrMutateFlow.runResourceFlow(ResourceCreateOrMutateFlow.java:104)

I don't know what to suggest, except to say that it seems exceptions are being used there to create "application logic", and thus subverting much (all?) of the value of the Preconditions checks.... Perhaps the validation of the tld/name should be done explicitly before getting to this point? In any case, the assumption stated in the comment, that "it won't be needed anyway as the flow will fail when validation fails later," is wrong in this case. We can argue that "other" assumptions (duplicate ROIDS) were violated and so all bets are off, but the swallowing of the exception makes it really hard to track that mistake down....

-h

[Nick Felt]

Sorry this was so painful - I can see how it would have been hard to debug.  That catch statement is definitely too broad - it should be scoped to just InternetDomainName.from().  Better yet, it should be using InternetDomainName.isValid() to avoid having the exception at all:
https://google.github.io/guava/releases/snapshot/api/docs/com/google/common/net/InternetDomainName.html#isValid(java.lang.String)

I've sent out a change internally to fix this issue, thanks for flagging it.

For what it's worth, Corey has also been working on migrating all of the flows to a greatly flattened hierarchy, which will hopefully eliminate a lot of these gotcha bits of code where we detect an error but have to sidestep it because the "right" code to detect the error is later on.  The domain create flows are probably the hardest to migrate and haven't been done yet, but once they're migrated this whole area should look way more straightforward.

--
NOTE: This is a public discussion list for the Nomulus domain registry project.
---
You received this message because you are subscribed to the Google Groups "nomulus-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to nomulus-discu...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/nomulus-discuss/e3560891-5013-48d9-ab7c-91c14d86ca10%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Corey Goldfeder]

I just, literally today, deleted that file, and got rid of that exception-as-control-flow.

--
NOTE: This is a public discussion list for the Nomulus domain registry project.
---
You received this message because you are subscribed to the Google Groups "nomulus-discuss" group.

To unsubscribe from this group and stop receiving emails from it, send an email to nomulus-discuss+unsubscribe@googlegroups.com.