From fd78d281d57d7b1ffa49d27cd03de39b6a961592 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Sat, 12 Nov 2016 15:14:55 +0100 Subject: [PATCH] Fix Quality flaws - missing tests for Protobuf - remove incorrect imports of org.elasticsearch.common.Strings --- .../authentication/BasicAuthenticator.java | 6 +-- .../server/authentication/JwtHttpHandler.java | 4 +- .../authentication/RealmAuthenticator.java | 14 +++--- .../server/authentication/ws/LoginAction.java | 4 +- .../setting/ws/ListDefinitionsAction.java | 31 ++++--------- .../sonar/server/setting/ws/ValuesAction.java | 4 +- .../org/sonar/core/util/ProtobufTest.java | 43 ++++++++++++++++--- 7 files changed, 60 insertions(+), 46 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/BasicAuthenticator.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/BasicAuthenticator.java index 3c000d9b467..9808d57b8f3 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/BasicAuthenticator.java +++ b/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(); diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/JwtHttpHandler.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/JwtHttpHandler.java index bcb9f5ba349..54d571abe98 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/JwtHttpHandler.java +++ b/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); diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/RealmAuthenticator.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/RealmAuthenticator.java index d2ee0426bc4..76ccd170e91 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/RealmAuthenticator.java +++ b/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) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/ws/LoginAction.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/ws/LoginAction.java index eb69f2b569d..3a959c4fd12 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/ws/LoginAction.java +++ b/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); diff --git a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/ListDefinitionsAction.java b/server/sonar-server/src/main/java/org/sonar/server/setting/ws/ListDefinitionsAction.java index 6c4f0a639f8..1ada6c3c0e7 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/ListDefinitionsAction.java +++ b/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 options = definition.options(); if (!options.isEmpty()) { builder.addAllOptions(options); diff --git a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/ValuesAction.java b/server/sonar-server/src/main/java/org/sonar/server/setting/ws/ValuesAction.java index e5e6567e6d6..4388c04c97e 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/setting/ws/ValuesAction.java +++ b/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 loadDefaultSettings(Set 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()); } diff --git a/sonar-core/src/test/java/org/sonar/core/util/ProtobufTest.java b/sonar-core/src/test/java/org/sonar/core/util/ProtobufTest.java index 95e2f6ee648..dcbef1412cb 100644 --- a/sonar-core/src/test/java/org/sonar/core/util/ProtobufTest.java +++ b/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 it = Protobuf.readStream(file, Fake.PARSER); + CloseableIterator 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 it = Protobuf.readStream(file, Fake.PARSER); + CloseableIterator 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(); + } } -- 2.39.5