Merge pull request #346 from tekkamanendless/master

[JENKINS-39498] Stop panicking about "eventCreatedOn" and losing my position
jenkinsci · Feb 6, 2018 · e48c9b2 · e48c9b2
2 parents f6cdb2f + faba911
commit e48c9b2
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 19 deletions.
diff --git a/...onyericsson/hudson/plugins/gerrit/trigger/playback/GerritMissedEventsPlaybackManager.java b/...onyericsson/hudson/plugins/gerrit/trigger/playback/GerritMissedEventsPlaybackManager.java
@@ -152,8 +152,13 @@ public String getServerName() {
     public void checkIfEventsLogPluginSupported() {
         GerritServer server = PluginImpl.getServer_(serverName);
         if (server != null && server.getConfig() != null) {
-            isSupported = GerritPluginChecker.isPluginEnabled(
-                    server.getConfig(), EVENTS_LOG_PLUGIN_NAME, true);
+            Boolean newValue = GerritPluginChecker.isPluginEnabled(server.getConfig(), EVENTS_LOG_PLUGIN_NAME, true);
+            if (newValue == null) {
+                logger.warn("Could not determine plugin support for " + EVENTS_LOG_PLUGIN_NAME
+                    + "; leaving status as " + String.valueOf(isSupported));
+            } else {
+                isSupported = newValue;
+            }
         }
     }
 
@@ -446,19 +451,16 @@ protected String buildEventsLogURL(IGerritHudsonTriggerConfig config, Date date1
      * @return true if was able to persist event.
      */
     synchronized boolean persist(GerritTriggeredEvent evt) {
-
+        // If there is not timestamp, then ignore this event.
         if (evt == null || evt.getEventCreatedOn() == null) {
-            logger.warn("Event CreatedOn is null...Gerrit Server might not support attribute eventCreatedOn. "
-                    + "Will NOT persist this event and Missed Events will be disabled!");
-            isSupported = false;
+            logger.debug("'eventCreatedOn' is null; skipping event.");
             return false;
         }
 
         long ts = evt.getEventCreatedOn().getTime();
+        // If the timestamp is invalid, then ignore this event.
         if (ts == 0) {
-            logger.warn("Event CreatedOn is 0...Gerrit Server does not support attribute eventCreatedOn. "
-                    + "Will NOT persist this event and Missed Events will be disabled!");
-            isSupported = false;
+            logger.debug("'eventCreatedOn' is 0; skipping event.");
             return false;
         }
 

diff --git a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/utils/GerritPluginChecker.java b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/utils/GerritPluginChecker.java
@@ -32,7 +32,6 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.io.IOException;
 import java.net.HttpURLConnection;
 
 /**
@@ -133,21 +132,22 @@ private static boolean decodeStatus(int statusCode, String pluginName, boolean q
      * Query Gerrit to determine if plugin is enabled.
      * @param config Gerrit Server Config
      * @param pluginName The Gerrit Plugin name.
-     * @return true if enabled.
+     * @return true if enabled, false if not, and null if we couldn't tell.
      */
-    public static boolean isPluginEnabled(IGerritHudsonTriggerConfig config, String pluginName) {
+    public static Boolean isPluginEnabled(IGerritHudsonTriggerConfig config, String pluginName) {
         return isPluginEnabled(config, pluginName, false);
     }
 
     /**
      * Query Gerrit to determine if plugin is enabled.
+     *
+     * In the event that this cannot determine the status, it will throw an Exception.
      * @param config Gerrit Server Config
      * @param pluginName The Gerrit Plugin name.
      * @param quiet Whether we want to log a message.
-     * @return true if enabled.
+     * @return true if enabled, false if not, and null if we couldn't tell.
      */
-    public static boolean isPluginEnabled(IGerritHudsonTriggerConfig config, String pluginName, boolean quiet) {
-
+    public static Boolean isPluginEnabled(IGerritHudsonTriggerConfig config, String pluginName, boolean quiet) {
         String restUrl = buildURL(config);
         if (restUrl == null) {
             if (quiet) {
@@ -165,18 +165,17 @@ public static boolean isPluginEnabled(IGerritHudsonTriggerConfig config, String
             int statusCode = execute.getStatusLine().getStatusCode();
             logger.debug("status code: {}", statusCode);
             return decodeStatus(statusCode, pluginName, quiet);
-        } catch (IOException e) {
+        } catch (Exception e) {
             logger.warn(Messages.PluginHttpConnectionGeneralError(pluginName,
                     e.getMessage()), e);
-            return false;
-        } finally {
             if (execute != null) {
                 try {
                     execute.close();
                 } catch (Exception exp) {
                     logger.trace("Error happened when close http client.", exp);
                 }
             }
+            return null;
         }
     }
 }
diff --git a/...ricsson/hudson/plugins/gerrit/trigger/playback/GerritMissedEventsPlaybackManagerTest.java b/...ricsson/hudson/plugins/gerrit/trigger/playback/GerritMissedEventsPlaybackManagerTest.java
@@ -155,7 +155,6 @@ public void setUp() throws IOException {
         PowerMockito.mockStatic(GerritPluginChecker.class);
         PowerMockito.when(GerritPluginChecker.isPluginEnabled((IGerritHudsonTriggerConfig)anyObject()
                 , anyString(), anyBoolean())).thenReturn(true);
-
     }
 
     /**
@@ -335,4 +334,59 @@ public void testHandleGarbageResponse() {
 
     }
 
+    /**
+     * This tests that the initial `isSupported` state is false.
+     */
+    @Test
+    public void testInitialSupportedState() {
+       // Option 1a: not supported
+       PowerMockito.when(GerritPluginChecker.isPluginEnabled((IGerritHudsonTriggerConfig)anyObject()
+               , anyString(), anyBoolean())).thenReturn(false);
+
+        GerritMissedEventsPlaybackManager missingEventsPlaybackManager
+                = new GerritMissedEventsPlaybackManager("defaultServer");
+        Assert.assertFalse("isSupported should be false", missingEventsPlaybackManager.isSupported());
+
+       // Option 1b: not supported
+       PowerMockito.when(GerritPluginChecker.isPluginEnabled((IGerritHudsonTriggerConfig)anyObject()
+               , anyString(), anyBoolean())).thenReturn(null);
+
+        missingEventsPlaybackManager
+                = new GerritMissedEventsPlaybackManager("defaultServer");
+        Assert.assertFalse("isSupported should be false", missingEventsPlaybackManager.isSupported());
+
+       // Option 2: supported
+       PowerMockito.when(GerritPluginChecker.isPluginEnabled((IGerritHudsonTriggerConfig)anyObject()
+               , anyString(), anyBoolean())).thenReturn(true);
+
+        missingEventsPlaybackManager
+                = new GerritMissedEventsPlaybackManager("defaultServer");
+        Assert.assertTrue("isSupported should be true", missingEventsPlaybackManager.isSupported());
+    }
+
+    /**
+     * This tests that the supported state does not change if GerritPluginChecker
+     * cannot determine the state successfully.
+     */
+    @Test
+    public void testStateOnlyChangesWhenValid() {
+       PowerMockito.when(GerritPluginChecker.isPluginEnabled((IGerritHudsonTriggerConfig)anyObject()
+               , anyString(), anyBoolean())).thenReturn(false);
+
+        GerritMissedEventsPlaybackManager missingEventsPlaybackManager
+                = new GerritMissedEventsPlaybackManager("defaultServer");
+        Assert.assertFalse("isSupported should be false", missingEventsPlaybackManager.isSupported());
+
+        PowerMockito.when(GerritPluginChecker.isPluginEnabled((IGerritHudsonTriggerConfig)anyObject()
+                , anyString(), anyBoolean())).thenReturn(true);
+
+        missingEventsPlaybackManager.checkIfEventsLogPluginSupported();
+        Assert.assertTrue("isSupported should be true", missingEventsPlaybackManager.isSupported());
+
+        PowerMockito.when(GerritPluginChecker.isPluginEnabled((IGerritHudsonTriggerConfig)anyObject()
+                , anyString(), anyBoolean())).thenReturn(null);
+
+        missingEventsPlaybackManager.checkIfEventsLogPluginSupported();
+        Assert.assertTrue("isSupported should be true", missingEventsPlaybackManager.isSupported());
+    }
 }