Fix Quality flaws

- missing tests for Protobuf
- remove incorrect imports of org.elasticsearch.common.Strings

server/sonar-server/src/main/java/org/sonar/server/authentication/BasicAuthenticator.java

@@ -20,9 +20,6 @@
 
 package org.sonar.server.authentication;
 
-import static java.util.Locale.ENGLISH;
-import static org.elasticsearch.common.Strings.isEmpty;
-
 import com.google.common.base.Charsets;
 import java.util.Base64;
 import java.util.Optional;
@@ -33,6 +30,9 @@ import org.sonar.db.user.UserDto;
 import org.sonar.server.exceptions.UnauthorizedException;
 import org.sonar.server.usertoken.UserTokenAuthenticator;
 
+import static java.util.Locale.ENGLISH;
+import static org.apache.commons.lang.StringUtils.isEmpty;
+
 public class BasicAuthenticator {
 
   private static final Base64.Decoder BASE64_DECODER = Base64.getDecoder();

server/sonar-server/src/main/java/org/sonar/server/authentication/JwtHttpHandler.java

@@ -38,8 +38,8 @@ import org.sonar.db.DbSession;
 import org.sonar.db.user.UserDto;
 
 import static java.util.Objects.requireNonNull;
+import static org.apache.commons.lang.StringUtils.isEmpty;
 import static org.apache.commons.lang.time.DateUtils.addSeconds;
-import static org.elasticsearch.common.Strings.isNullOrEmpty;
 import static org.sonar.server.authentication.CookieUtils.findCookie;
 
 @ServerSide
@@ -117,7 +117,7 @@ public class JwtHttpHandler {
     }
     Cookie cookie = jwtCookie.get();
     String token = cookie.getValue();
-    if (isNullOrEmpty(token)) {
+    if (isEmpty(token)) {
       return Optional.empty();
     }
     return Optional.of(token);

server/sonar-server/src/main/java/org/sonar/server/authentication/RealmAuthenticator.java

@@ -20,12 +20,6 @@
 
 package org.sonar.server.authentication;
 
-import static java.util.Objects.requireNonNull;
-import static org.apache.commons.lang.StringUtils.trimToNull;
-import static org.elasticsearch.common.Strings.isNullOrEmpty;
-import static org.sonar.api.CoreProperties.CORE_AUTHENTICATOR_CREATE_USERS;
-import static org.sonar.server.user.UserUpdater.SQ_AUTHORITY;
-
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.Locale;
@@ -47,6 +41,12 @@ import org.sonar.db.user.UserDto;
 import org.sonar.server.exceptions.UnauthorizedException;
 import org.sonar.server.user.SecurityRealmFactory;
 
+import static java.util.Objects.requireNonNull;
+import static org.apache.commons.lang.StringUtils.isEmpty;
+import static org.apache.commons.lang.StringUtils.trimToNull;
+import static org.sonar.api.CoreProperties.CORE_AUTHENTICATOR_CREATE_USERS;
+import static org.sonar.server.user.UserUpdater.SQ_AUTHORITY;
+
 public class RealmAuthenticator implements Startable {
 
   private static final Logger LOG = Loggers.get(RealmAuthenticator.class);
@@ -107,7 +107,7 @@ public class RealmAuthenticator implements Startable {
     String name = details.getName();
     UserIdentity.Builder userIdentityBuilder = UserIdentity.builder()
       .setLogin(userLogin)
-      .setName(isNullOrEmpty(name) ? userLogin : name)
+      .setName(isEmpty(name) ? userLogin : name)
       .setEmail(trimToNull(details.getEmail()))
       .setProviderLogin(userLogin);
     if (externalGroupsProvider != null) {

server/sonar-server/src/main/java/org/sonar/server/authentication/ws/LoginAction.java

@@ -38,7 +38,7 @@ import org.sonar.server.user.ServerUserSession;
 import org.sonar.server.user.ThreadLocalUserSession;
 
 import static java.net.HttpURLConnection.HTTP_BAD_REQUEST;
-import static org.elasticsearch.common.Strings.isNullOrEmpty;
+import static org.apache.commons.lang.StringUtils.isEmpty;
 
 public class LoginAction extends ServletFilter {
 
@@ -85,7 +85,7 @@ public class LoginAction extends ServletFilter {
   private UserDto authenticate(HttpServletRequest request) {
     String login = request.getParameter("login");
     String password = request.getParameter("password");
-    if (isNullOrEmpty(login) || isNullOrEmpty(password)) {
+    if (isEmpty(login) || isEmpty(password)) {
       throw new UnauthorizedException();
     }
     return credentialsAuthenticator.authenticate(login, password, request);

server/sonar-server/src/main/java/org/sonar/server/setting/ws/ListDefinitionsAction.java

@@ -38,7 +38,8 @@ import org.sonarqube.ws.Settings;
 import org.sonarqube.ws.Settings.ListDefinitionsWsResponse;
 import org.sonarqube.ws.client.setting.ListDefinitionsRequest;
 
-import static org.elasticsearch.common.Strings.isNullOrEmpty;
+import static com.google.common.base.Strings.emptyToNull;
+import static org.sonar.core.util.Protobuf.setNullable;
 import static org.sonar.server.component.ComponentFinder.ParamNames.ID_AND_KEY;
 import static org.sonar.server.setting.ws.SettingsWsComponentParameters.addComponentParameters;
 import static org.sonar.server.ws.WsUtils.writeProtobuf;
@@ -134,30 +135,14 @@ public class ListDefinitionsAction implements SettingsWsAction {
       .setKey(key)
       .setType(Settings.Type.valueOf(definition.type().name()))
       .setMultiValues(definition.multiValues());
-    String deprecatedKey = definition.deprecatedKey();
-    if (!isNullOrEmpty(deprecatedKey)) {
-      builder.setDeprecatedKey(deprecatedKey);
-    }
-    String name = definition.name();
-    if (!isNullOrEmpty(name)) {
-      builder.setName(name);
-    }
-    String description = definition.description();
-    if (!isNullOrEmpty(description)) {
-      builder.setDescription(description);
-    }
+    setNullable(emptyToNull(definition.deprecatedKey()), builder::setDeprecatedKey);
+    setNullable(emptyToNull(definition.name()), builder::setName);
+    setNullable(emptyToNull(definition.description()), builder::setDescription);
     String category = propertyDefinitions.getCategory(key);
-    if (!isNullOrEmpty(category)) {
-      builder.setCategory(category);
-    }
+    setNullable(emptyToNull(category), builder::setCategory);
     String subCategory = propertyDefinitions.getSubCategory(key);
-    if (!isNullOrEmpty(subCategory)) {
-      builder.setSubCategory(subCategory);
-    }
-    String defaultValue = definition.defaultValue();
-    if (!isNullOrEmpty(defaultValue)) {
-      builder.setDefaultValue(defaultValue);
-    }
+    setNullable(emptyToNull(subCategory), builder::setSubCategory);
+    setNullable(emptyToNull(definition.defaultValue()), builder::setDefaultValue);
     List<String> options = definition.options();
     if (!options.isEmpty()) {
       builder.addAllOptions(options);

server/sonar-server/src/main/java/org/sonar/server/setting/ws/ValuesAction.java

@@ -47,7 +47,7 @@ import org.sonarqube.ws.Settings.ValuesWsResponse;
 import org.sonarqube.ws.client.setting.ValuesRequest;
 
 import static java.lang.String.format;
-import static org.elasticsearch.common.Strings.isNullOrEmpty;
+import static org.apache.commons.lang.StringUtils.isEmpty;
 import static org.sonar.api.PropertyType.PROPERTY_SET;
 import static org.sonar.server.component.ComponentFinder.ParamNames.ID_AND_KEY;
 import static org.sonar.server.setting.ws.SettingsWsComponentParameters.addComponentParameters;
@@ -156,7 +156,7 @@ public class ValuesAction implements SettingsWsAction {
   private List<Setting> loadDefaultSettings(Set<String> keys) {
     return propertyDefinitions.getAll().stream()
       .filter(definition -> keys.contains(definition.key()))
-      .filter(defaultProperty -> !isNullOrEmpty(defaultProperty.defaultValue()))
+      .filter(defaultProperty -> !isEmpty(defaultProperty.defaultValue()))
       .map(Setting::createForDefinition)
       .collect(Collectors.toList());
   }

sonar-core/src/test/java/org/sonar/core/util/ProtobufTest.java

@@ -21,6 +21,7 @@ package org.sonar.core.util;
 
 import java.io.File;
 import org.apache.commons.io.FileUtils;
+import org.apache.commons.lang.StringUtils;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -30,6 +31,7 @@ import org.sonar.test.TestUtils;
 import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.sonar.core.test.Test.Fake;
+import static org.sonar.core.util.Protobuf.setNullable;
 
 public class ProtobufTest {
 
@@ -51,13 +53,13 @@ public class ProtobufTest {
 
     File file = temp.newFile();
     FileUtils.forceDelete(file);
-    Protobuf.read(file, Fake.PARSER);
+    Protobuf.read(file, Fake.parser());
   }
 
   @Test
   public void read_file_returns_empty_message_if_file_is_empty() throws Exception {
     File file = temp.newFile();
-    Fake msg = Protobuf.read(file, Fake.PARSER);
+    Fake msg = Protobuf.read(file, Fake.parser());
     assertThat(msg).isNotNull();
     assertThat(msg.isInitialized()).isTrue();
   }
@@ -66,7 +68,7 @@ public class ProtobufTest {
   public void read_file_returns_message() throws Exception {
     File file = temp.newFile();
     Protobuf.write(Fake.getDefaultInstance(), file);
-    Fake message = Protobuf.read(file, Fake.PARSER);
+    Fake message = Protobuf.read(file, Fake.parser());
     assertThat(message).isNotNull();
     assertThat(message.isInitialized()).isTrue();
   }
@@ -88,7 +90,7 @@ public class ProtobufTest {
     Fake item2 = Fake.newBuilder().setLabel("two").build();
     Protobuf.writeStream(asList(item1, item2), file, false);
 
-    CloseableIterator<Fake> it = Protobuf.readStream(file, Fake.PARSER);
+    CloseableIterator<Fake> it = Protobuf.readStream(file, Fake.parser());
     Fake read = it.next();
     assertThat(read.getLabel()).isEqualTo("one");
     assertThat(read.getLine()).isEqualTo(1);
@@ -104,16 +106,43 @@ public class ProtobufTest {
     thrown.expectMessage("Unable to read messages");
 
     File dir = temp.newFolder();
-    Protobuf.readStream(dir, Fake.PARSER);
+    Protobuf.readStream(dir, Fake.parser());
   }
 
   @Test
   public void read_empty_stream() throws Exception {
     File file = temp.newFile();
-    CloseableIterator<Fake> it = Protobuf.readStream(file, Fake.PARSER);
+    CloseableIterator<Fake> it = Protobuf.readStream(file, Fake.parser());
     assertThat(it).isNotNull();
     assertThat(it.hasNext()).isFalse();
   }
 
-  // TODO test in-moemry file
+  @Test
+  public void setNullable_sets_field_if_value_is_not_null() {
+    Fake.Builder builder = Fake.newBuilder();
+
+    setNullable("foo", builder::setLabel);
+    assertThat(builder.getLabel()).isEqualTo("foo");
+
+    builder.clear();
+    setNullable(null, builder::setLabel);
+    assertThat(builder.hasLabel()).isFalse();
+  }
+
+  @Test
+  public void setNullable_converts_value_and_sets_field_if_value_is_not_null() {
+    Fake.Builder builder = Fake.newBuilder();
+
+    setNullable("foo", builder::setLabel, StringUtils::upperCase);
+    assertThat(builder.getLabel()).isEqualTo("FOO");
+
+    builder.clear();
+    setNullable((String)null, builder::setLabel, StringUtils::upperCase);
+    assertThat(builder.hasLabel()).isFalse();
+
+    // do not set field if value is present but result of conversion is null
+    builder.clear();
+    setNullable("    ", builder::setLabel, StringUtils::trimToNull);
+    assertThat(builder.hasLabel()).isFalse();
+  }
 }