From 223e8c23168f17ccb8e52919ed9ab28779c36dda Mon Sep 17 00:00:00 2001
From: cgoldfeder <cgoldfeder@google.com>
Date: Thu, 2 Feb 2017 07:52:37 -0800
Subject: [PATCH] Fix a null check bug in HostUpdateFlow

This bug is about a bad use of Optional.
We were checking == null instead of .isPresent(), so the check
always passed, and we always set a lastSubordinateTime when
updating hosts, even if the host was external and should have
had a null value in that field.

There is almost certainly bad data in prod in the sense
that any external host that was ever updated will have a value
for this field instead of null. However, this is not
consequential as the field is entirely meaningless for
external hosts, and will be properly reset if the host is
ever moved to be internal.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=146363178
---
 java/google/registry/flows/host/HostUpdateFlow.java          | 2 +-
 javatests/google/registry/flows/host/HostUpdateFlowTest.java | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/java/google/registry/flows/host/HostUpdateFlow.java b/java/google/registry/flows/host/HostUpdateFlow.java
index 857bd09f5..df838c956 100644
--- a/java/google/registry/flows/host/HostUpdateFlow.java
+++ b/java/google/registry/flows/host/HostUpdateFlow.java
@@ -155,7 +155,7 @@ public final class HostUpdateFlow implements TransactionalFlow {
         // the lookupSuperordinateDomain(...) call above, so that it will never be stale.
         .setSuperordinateDomain(
             superordinateDomain.isPresent() ? Key.create(superordinateDomain.get()) : null)
-        .setLastSuperordinateChange(superordinateDomain == null ? null : now)
+        .setLastSuperordinateChange(superordinateDomain.isPresent() ? now : null)
         .build()
         // Rely on the host's cloneProjectedAtTime() method to handle setting of transfer data.
         .cloneProjectedAtTime(now);
diff --git a/javatests/google/registry/flows/host/HostUpdateFlowTest.java b/javatests/google/registry/flows/host/HostUpdateFlowTest.java
index 0e6c412db..0264328ca 100644
--- a/javatests/google/registry/flows/host/HostUpdateFlowTest.java
+++ b/javatests/google/registry/flows/host/HostUpdateFlowTest.java
@@ -562,6 +562,8 @@ public class HostUpdateFlowTest extends ResourceFlowTestCase<HostUpdateFlow, Hos
     // The last transfer time should be what was on the superordinate domain at the time of the host
     // update, not what it is later changed to.
     assertThat(renamedHost.getLastTransferTime()).isEqualTo(lastTransferTime);
+    // External hosts should always have null lastSuperordinateChange.
+    assertThat(renamedHost.getLastSuperordinateChange()).isNull();
   }
 
   @Test