[BAD-551]: Sqoop Import/Export: Namenode and Jobtracker values are ov…

…erwritten after opening job entry to edit and pressing 'Ok' button

The issue was introduced with big-data OSGIing changes (50758ca#diff-dc090573b3a321f1d9d9d6e0e1ed093eR249)
There are fixes and unit tests around changes in kettle-plugins/sqoop module.

Also fixed a couple of check style violations in

kettle-plugins/sqoop/src/main/java/org/pentaho/big/data/kettle/plugins/sqoop/SqoopConfig.java

@@ -22,6 +22,7 @@
 
 package org.pentaho.big.data.kettle.plugins.sqoop;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import org.pentaho.big.data.api.cluster.NamedCluster;
@@ -329,7 +330,7 @@ public SqoopConfig clone() {
 
   /**
    * Silently set the following properties: {@code database, connect, username, password}.
-   * 
+   *
    * @param database
    *          Database name
    * @param connect
@@ -652,7 +653,7 @@ public JobEntryMode getModeAsEnum() {
 
   /**
    * Sets the mode based on the enum value
-   * 
+   *
    * @param mode
    */
   public void setMode( JobEntryMode mode ) {
@@ -952,8 +953,7 @@ public void saveClusterConfig( Repository rep, ObjectId id_job, JobEntryInterfac
   }
 
   public boolean isAdvancedClusterConfigSet() {
-    Map<String, String> defaults = namedClusterProperties( createClusterTemplate() );
-    return Strings.isNullOrEmpty( getClusterName() ) && namedClusterProperties( getNamedCluster() ).equals( defaults );
+    return Strings.isNullOrEmpty( getClusterName() ) && ncPropertiesNotNullOrEmpty( getNamedCluster() );
   }
 
   private static Map<String, String> namedClusterProperties( NamedCluster namedCluster ) {
@@ -964,4 +964,11 @@ private static Map<String, String> namedClusterProperties( NamedCluster namedClu
       JOBTRACKER_PORT, Strings.nullToEmpty( namedCluster.getJobTrackerPort() )
     );
   }
+
+  @VisibleForTesting
+    boolean ncPropertiesNotNullOrEmpty( NamedCluster nc ) {
+    return !Strings.isNullOrEmpty( nc.getHdfsHost() ) || !Strings.isNullOrEmpty( nc.getHdfsPort() ) || !Strings.isNullOrEmpty( nc.getJobTrackerHost() ) || !Strings.isNullOrEmpty( nc
+        .getJobTrackerPort() );
+  }
+
 }

kettle-plugins/sqoop/src/main/java/org/pentaho/big/data/kettle/plugins/sqoop/SqoopUtils.java

@@ -74,7 +74,7 @@ public class SqoopUtils {
 
   /**
    * Parse a string into arguments as if it were provided on the command line.
-   * 
+   *
    * @param commandLineString
    *          A command line string, e.g. "sqoop import --table test --connect jdbc:mysql://bogus/bogus"
    * @param variableSpace
@@ -146,7 +146,7 @@ public static List<String> parseCommandLine( String commandLineString, VariableS
   /**
    * Configure a {@link SqoopConfig} object from a command line string. Variables will be replaced if
    * {@code variableSpace} is provided.
-   * 
+   *
    * @param config
    *          Configuration to update
    * @param commandLineString
@@ -218,7 +218,7 @@ public static void configureFromCommandLine(
 
   /**
    * Does the string reprsent an argument name as provided on the command line? Format: "--argname"
-   * 
+   *
    * @param s
    *          Possible argument name
    * @return {@code true} if the string represents an argument name (is prefixed with ARG_PREFIX)
@@ -242,7 +242,7 @@ private static int isArgName( String s ) {
   /**
    * Updates arguments of {@code config} based on the map of argument values. All other arguments will be cleared from
    * {@code config}.
-   * 
+   *
    * @param config
    *          Configuration object to update
    * @param args
@@ -320,7 +320,7 @@ private static String pickupArgumentValueFor( CommandLineArgument arg, Map<Strin
 
   /**
    * Generate a list of command line arguments and their values for arguments that require them.
-   * 
+   *
    * @param config
    *          Sqoop configuration to build a list of command line arguments from
    * @param variableSpace
@@ -347,7 +347,7 @@ public static List<String> getCommandLineArgs( SqoopConfig config, VariableSpace
   /**
    * Generate a command line string for the given configuration. Replace variables with the values from
    * {@code variableSpace} if provided.
-   * 
+   *
    * @param config
    *          Sqoop configuration
    * @param variableSpace
@@ -403,7 +403,7 @@ public static String generateCommandLineString( SqoopConfig config, VariableSpac
 
   /**
    * Escapes known Java escape sequences. See {@link #ESCAPE_SEQUENCES} for the list of escape sequences we escape here.
-   * 
+   *
    * @param s
    *          String to escape
    * @return Escaped string where all escape sequences are properly escaped
@@ -417,7 +417,7 @@ protected static String escapeEscapeSequences( String s ) {
 
   /**
    * If any whitespace is detected the string will be quoted. If any quotes exist in the string they will be escaped.
-   * 
+   *
    * @param s
    *          String to quote
    * @return A quoted version of {@code s} if whitespace exists in the string, otherwise unmodified {@code s}.
@@ -434,7 +434,7 @@ protected static String quote( String s ) {
 
   /**
    * Add all {@link ArgumentWrapper}s to a list of arguments
-   * 
+   *
    * @param args
    *          Arguments to append to
    * @param arguments
@@ -451,7 +451,7 @@ protected static void appendArguments( List<String> args, Set<? extends Argument
 
   /**
    * Append this argument to a list of arguments if it has a value or if it's a flag.
-   * 
+   *
    * @param args
    *          List of arguments to append to
    */
@@ -470,7 +470,7 @@ protected static void appendArgument( List<String> args, ArgumentWrapper arg, Va
     }
   }
 
-  private static void appendCustomArguments( List<String> args , SqoopConfig config, VariableSpace variableSpace ) {
+  private static void appendCustomArguments( List<String> args, SqoopConfig config, VariableSpace variableSpace ) {
     for ( PropertyEntry entry : config.getCustomArguments() ) {
       appendCustomArgument( args, entry, variableSpace, false );
     }
@@ -541,7 +541,7 @@ private static void handleCustomOption( List<String> args, String option, Stream
   /**
    * Find all fields annotated with {@link CommandLineArgument} in the class provided. All arguments must have valid
    * JavaBeans-style getter and setter methods in the object.
-   * 
+   *
    * @param o
    *          Object to look for arguments in
    * @return Ordered set of arguments representing all {@link CommandLineArgument}-annotated fields in {@code o}
@@ -584,7 +584,7 @@ public int compare( ArgumentWrapper o1, ArgumentWrapper o2 ) {
 
   /**
    * Determine the display name for the command line argument.
-   * 
+   *
    * @param anno
    *          Command line argument
    * @return {@link CommandLineArgument#displayName()} or, if not set,
@@ -597,7 +597,7 @@ public static String getDisplayName( CommandLineArgument anno ) {
   /**
    * Finds a method in the given class or any super class with the name {@code prefix + methodName} that accepts 0
    * parameters.
-   * 
+   *
    * @param aClass
    *          Class to search for method in
    * @param methodName

kettle-plugins/sqoop/src/main/java/org/pentaho/big/data/kettle/plugins/sqoop/ui/AbstractSqoopJobEntryController.java

@@ -710,7 +710,7 @@ protected void setUiMode( JobEntryMode mode ) {
         if ( selectedNamedCluster != null
           && !this.selectedNamedCluster.equals( USE_ADVANCED_OPTIONS_CLUSTER )
           && !suppressEventHandling ) {
-          config.setNamedCluster( null );
+          config.setNamedCluster( selectedNamedCluster );
         }
         toggleQuickMode( true );
         break;

kettle-plugins/sqoop/src/test/java/org/pentaho/big/data/kettle/plugins/sqoop/PropertyFiringObjectTest.java

@@ -60,7 +60,7 @@ public void testSqoopImportConfig() throws Exception {
   /**
    * Test that all private fields of the provided object have getter and setter methods, and they work as they should:
    * the getter returns the value and the setter generates a {@link PropertyChangeEvent} for that property.
-   * 
+   *
    * @param o
    *          Object to test
    * @throws Exception
@@ -85,7 +85,7 @@ private void testPropertyFiringForAllPrivateFieldsOf( XulEventSource o ) throws
   /**
    * Test that all private fields have getter and setter methods, and they work as they should: the getter returns the
    * value and the setter generates a {@link PropertyChangeEvent} for that property.
-   * 
+   *
    * @param o
    *          instance of event source to test
    * @param oClass
@@ -100,8 +100,7 @@ private void testPropertyFiringForAllPrivateFieldsOf( XulEventSource o, Class<?>
     }
 
     for ( Field f : oClass.getDeclaredFields() ) {
-      if ( !Modifier.isPrivate( f.getModifiers() ) || Modifier.isTransient( f.getModifiers() ) ||
-        !supportedTypes.contains( f.getType() ) ) {
+      if ( !Modifier.isPrivate( f.getModifiers() ) || Modifier.isTransient( f.getModifiers() ) || !supportedTypes.contains( f.getType() ) ) {
         // Skip non-private or transient fields
         continue;
       }
@@ -142,7 +141,7 @@ private void testPropertyFiringForAllPrivateFieldsOf( XulEventSource o, Class<?>
 
   /**
    * Get a value to test with that matches the type provided.
-   * 
+   *
    * @param type
    *          Type of test value
    * @param originalValue

kettle-plugins/sqoop/src/test/java/org/pentaho/big/data/kettle/plugins/sqoop/SqoopConfigTest.java

@@ -32,6 +32,7 @@
 import org.mockito.stubbing.Answer;
 import org.pentaho.big.data.api.cluster.NamedCluster;
 import org.pentaho.big.data.kettle.plugins.job.JobEntryMode;
+import org.pentaho.big.data.kettle.plugins.job.PropertyEntry;
 import org.pentaho.big.data.kettle.plugins.sqoop.util.MockitoAutoBean;
 import org.pentaho.di.core.variables.Variables;
 import org.pentaho.di.core.xml.XMLHandler;
@@ -50,8 +51,11 @@
 import java.util.UUID;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Matchers.eq;
@@ -67,10 +71,20 @@
  */
 @RunWith( MockitoJUnitRunner.class )
 public class SqoopConfigTest {
+  /**
+   *
+   */
+  private static final String ENTRY_VALUE = "entryValue";
+  /**
+   *
+   */
+  private static final String ENTRY_KEY = "entryKey";
+  private static final String EMPTY = "";
   public static final String HDFS_HOST = "hdfsHost";
   public static final String HDFS_PORT = "8020";
   public static final String JOB_TRACKER_HOST = "jobTracker";
   public static final String JOB_TRACKER_PORT = "2222";
+  private NamedCluster namedClusterMock = mock( NamedCluster.class );
 
   private SqoopConfig config;
   private NamedCluster template;
@@ -336,4 +350,113 @@ public void testSetNamedCluster() throws Exception {
     verify( template ).replaceMeta( namedCluster );
     assertEquals( "named cluster", config.getClusterName() );
   }
+
+  @Test
+  public void testIsAdvancedClusterConfigSet_ClusterNameNull() throws Exception {
+    when( namedClusterMock.getName() ).thenReturn( null );
+    config.setNamedCluster( namedClusterMock );
+    assertTrue( config.isAdvancedClusterConfigSet() );
+  }
+
+  @Test
+  public void testIsAdvancedClusterConfigSet_ClusterNameEmpty() throws Exception {
+    when( namedClusterMock.getName() ).thenReturn( EMPTY );
+    config.setNamedCluster( namedClusterMock );
+    assertTrue( config.isAdvancedClusterConfigSet() );
+  }
+
+  @Test
+  public void testIsAdvancedClusterConfigSet_ClusterNameEmptyOrNullAndAllNcPropertiesNull() throws Exception {
+    when( namedClusterMock.getName() ).thenReturn( null );
+    config.setNamedCluster( namedClusterMock );
+    when( config.getNamedCluster().getHdfsHost() ).thenReturn( null );
+    when( config.getNamedCluster().getHdfsPort() ).thenReturn( null );
+    when( config.getNamedCluster().getJobTrackerHost() ).thenReturn( null );
+    when( config.getNamedCluster().getJobTrackerPort() ).thenReturn( null );
+    assertFalse( config.isAdvancedClusterConfigSet() );
+  }
+
+  @Test
+  public void testIsAdvancedClusterConfigSet_ClusterNameNotNull() throws Exception {
+    when( namedClusterMock.getName() ).thenReturn( "Cluster Name For Testing" );
+    config.setNamedCluster( namedClusterMock );
+    assertFalse( config.isAdvancedClusterConfigSet() );
+  }
+
+  @Test
+  public void testNcPropertiesNotNullOrEmpty_AllNotNullNotEmpty() throws Exception {
+    boolean ncPropertiesNotNullOrEmpty = config.ncPropertiesNotNullOrEmpty( config.getNamedCluster() );
+    assertTrue( ncPropertiesNotNullOrEmpty );
+  }
+
+  @Test
+  public void testNcPropertiesNotNullOrEmpty_AllNull() throws Exception {
+    when( config.getNamedCluster().getHdfsHost() ).thenReturn( null );
+    when( config.getNamedCluster().getHdfsPort() ).thenReturn( null );
+    when( config.getNamedCluster().getJobTrackerHost() ).thenReturn( null );
+    when( config.getNamedCluster().getJobTrackerPort() ).thenReturn( null );
+    boolean ncPropertiesNotNullOrEmpty = config.ncPropertiesNotNullOrEmpty( config.getNamedCluster() );
+    assertFalse( ncPropertiesNotNullOrEmpty );
+  }
+
+  @Test
+  public void testNcPropertiesNotNullOrEmpty_AllEmpty() throws Exception {
+    when( config.getNamedCluster().getHdfsHost() ).thenReturn( EMPTY );
+    when( config.getNamedCluster().getHdfsPort() ).thenReturn( EMPTY );
+    when( config.getNamedCluster().getJobTrackerHost() ).thenReturn( EMPTY );
+    when( config.getNamedCluster().getJobTrackerPort() ).thenReturn( EMPTY );
+    boolean ncPropertiesNotNullOrEmpty = config.ncPropertiesNotNullOrEmpty( config.getNamedCluster() );
+    assertFalse( ncPropertiesNotNullOrEmpty );
+  }
+
+  @Test
+  public void testNcPropertiesNotNullOrEmpty_HdfsHostOnlyNotNull() throws Exception {
+    when( config.getNamedCluster().getHdfsPort() ).thenReturn( null );
+    when( config.getNamedCluster().getJobTrackerHost() ).thenReturn( null );
+    when( config.getNamedCluster().getJobTrackerPort() ).thenReturn( null );
+    boolean ncPropertiesNotNullOrEmpty = config.ncPropertiesNotNullOrEmpty( config.getNamedCluster() );
+    assertTrue( "It should be true - HDFS host: " + config.getNamedCluster().getHdfsHost(), ncPropertiesNotNullOrEmpty );
+  }
+
+  @Test
+  public void testNcPropertiesNotNullOrEmpty_HdfsPortOnlyNotNull() throws Exception {
+    when( config.getNamedCluster().getHdfsHost() ).thenReturn( null );
+    when( config.getNamedCluster().getJobTrackerHost() ).thenReturn( null );
+    when( config.getNamedCluster().getJobTrackerPort() ).thenReturn( null );
+    boolean ncPropertiesNotNullOrEmpty = config.ncPropertiesNotNullOrEmpty( config.getNamedCluster() );
+    assertTrue( "It should be true - HDFS port: " + config.getNamedCluster().getHdfsPort(), ncPropertiesNotNullOrEmpty );
+  }
+
+  @Test
+  public void testNcPropertiesNotNullOrEmpty_JobTrackerHostOnlyNotNull() throws Exception {
+    when( config.getNamedCluster().getHdfsHost() ).thenReturn( null );
+    when( config.getNamedCluster().getHdfsPort() ).thenReturn( null );
+    when( config.getNamedCluster().getJobTrackerPort() ).thenReturn( null );
+    boolean ncPropertiesNotNullOrEmpty = config.ncPropertiesNotNullOrEmpty( config.getNamedCluster() );
+    assertTrue( "It should be true - Job tracker host: " + config.getNamedCluster().getJobTrackerHost(), ncPropertiesNotNullOrEmpty );
+  }
+
+  @Test
+  public void testNcPropertiesNotNullOrEmpty_JobTrackerPortOnlyNotNull() throws Exception {
+    when( config.getNamedCluster().getHdfsHost() ).thenReturn( null );
+    when( config.getNamedCluster().getHdfsPort() ).thenReturn( null );
+    when( config.getNamedCluster().getJobTrackerHost() ).thenReturn( null );
+    boolean ncPropertiesNotNullOrEmpty = config.ncPropertiesNotNullOrEmpty( config.getNamedCluster() );
+    assertTrue( "It should be true - Job tracker host: " + config.getNamedCluster().getJobTrackerPort(), ncPropertiesNotNullOrEmpty );
+  }
+
+  @Test
+  public void testSetCustomArguments_GetCustomArguments() throws Exception {
+    PropertyEntry pEntryMock = new PropertyEntry( ENTRY_KEY, ENTRY_VALUE );
+    AbstractModelList<PropertyEntry> customArguments = new AbstractModelList<>();
+    customArguments.add( pEntryMock );
+    assertNotNull( config.getCustomArguments() );
+    assertEquals( 0, config.getCustomArguments().size() );
+    config.setCustomArguments( customArguments );
+    assertSame( customArguments, config.getCustomArguments() );
+    assertEquals( 1, config.getCustomArguments().size() );
+    assertEquals( ENTRY_KEY, config.getCustomArguments().get( 0 ).getKey() );
+    assertEquals( ENTRY_VALUE, config.getCustomArguments().get( 0 ).getValue() );
+  }
+
 }