Issue 1929 - Address Sonar suggestions (#1925)

* Refactor Optionals

* move return out of finally block

* refactor keyname lookup

* rename equals override

* empty collections

* refactor maps

* immutable sets

* orElseGet

* cleanup boolean logic

* joining

* isEmpty

* initialize map

* is empty

* boolean expressions

* isEmpty

elide-async/src/main/java/com/yahoo/elide/async/export/formatter/CSVExportFormatter.java

@@ -13,6 +13,8 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.github.opendevl.JFlat;
 
+import org.apache.commons.lang3.StringUtils;
+
 import lombok.extern.slf4j.Slf4j;
 
 import java.util.Arrays;
@@ -86,11 +88,11 @@ private String generateCSVHeader(EntityProjection projection) {
             return "";
         }
 
-        String header = projection.getAttributes().stream()
+        return projection.getAttributes().stream()
         .map(attr -> {
             StringBuilder column = new StringBuilder();
             String alias = attr.getAlias();
-            column.append(alias != null && !alias.isEmpty() ? alias : attr.getName());
+            column.append(StringUtils.isNotEmpty(alias) ? alias : attr.getName());
             return column;
         })
         .map(quotable -> {
@@ -101,21 +103,15 @@ private String generateCSVHeader(EntityProjection projection) {
             return quotable;
         })
         .collect(Collectors.joining(COMMA));
-
-        return header;
     }
 
     @Override
     public String preFormat(EntityProjection projection, TableExport query) {
-        if (projection == null) {
+        if (projection == null || skipCSVHeader) {
             return null;
         }
 
-        if (!skipCSVHeader) {
-            return generateCSVHeader(projection);
-        };
-
-        return null;
+        return generateCSVHeader(projection);
     }
 
     @Override

elide-async/src/main/java/com/yahoo/elide/async/export/formatter/JSONExportFormatter.java

@@ -15,6 +15,8 @@
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.ObjectMapper;
 
+import org.apache.commons.lang3.StringUtils;
+
 import lombok.extern.slf4j.Slf4j;
 
 import java.util.Collections;
@@ -73,7 +75,7 @@ private static Map<String, Object> getAttributes(PersistentResource resource) {
 
         for (Attribute field : attrFields) {
             String alias = field.getAlias();
-            String fieldName = alias != null && !alias.isEmpty() ? alias : field.getName();
+            String fieldName = StringUtils.isNotEmpty(alias) ? alias : field.getName();
             attributes.put(fieldName, resource.getAttribute(field));
         }
         return attributes;

elide-async/src/main/java/com/yahoo/elide/async/operation/GraphQLAsyncQueryOperation.java

@@ -16,6 +16,8 @@
 
 import com.jayway.jsonpath.JsonPath;
 
+import org.apache.commons.collections4.CollectionUtils;
+
 import lombok.extern.slf4j.Slf4j;
 
 import java.net.URISyntaxException;
@@ -53,7 +55,7 @@ public Integer calculateRecordCount(AsyncQuery queryObj, ElideResponse response)
         Integer count = null;
         if (response.getResponseCode() == 200) {
             List<Integer> countList = JsonPath.read(response.getBody(), "$..edges.length()");
-            count = countList.size() > 0 ? countList.get(0) : 0;
+            count = CollectionUtils.isNotEmpty(countList) ? countList.get(0) : 0;
         }
         return count;
     }

elide-async/src/main/java/com/yahoo/elide/async/operation/TableExportOperation.java

@@ -103,8 +103,8 @@ public AsyncAPIResult call() {
             // Follows same flow as GraphQL. The query may result in failure but request was successfully processed.
             exportResult.setHttpStatus(200);
             exportResult.setCompletedOn(new Date());
-            return exportResult;
         }
+        return exportResult;
     }
 
     private Observable<String> concatStringWithObservable(String toConcat, Observable<String> observable,

elide-core/src/main/java/com/yahoo/elide/core/Path.java

@@ -16,6 +16,9 @@
 import com.yahoo.elide.core.type.Type;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
+
+import org.apache.commons.collections4.CollectionUtils;
+
 import lombok.AllArgsConstructor;
 import lombok.EqualsAndHashCode;
 import lombok.Getter;
@@ -59,7 +62,7 @@ public PathElement(Type type, Type fieldType, String fieldName) {
             this.fieldType = fieldType;
             this.fieldName = fieldName;
             this.alias = fieldName;
-            this.arguments = Collections.EMPTY_SET;
+            this.arguments = Collections.emptySet();
         }
     }
 
@@ -114,7 +117,7 @@ private List<PathElement> resolvePathElements(Type<?> entityClass,
                 currentClass = joinClass;
             } else {
                 elements.add(resolvePathAttribute(currentClass, fieldName,
-                        fieldName, Collections.EMPTY_SET, dictionary));
+                        fieldName, Collections.emptySet(), dictionary));
             }
         }
 
@@ -177,7 +180,7 @@ public String getAlias() {
 
     @Override
     public String toString() {
-        return pathElements.size() == 0 ? "EMPTY"
+        return CollectionUtils.isEmpty(pathElements) ? "EMPTY"
                 : pathElements.stream()
                         .map(e -> '[' + getSimpleName(e.getType()) + ']' + PERIOD + e.getFieldName())
                 .collect(Collectors.joining("/"));

elide-core/src/main/java/com/yahoo/elide/core/PersistentResource.java

@@ -49,6 +49,7 @@
 import com.yahoo.elide.jsonapi.models.ResourceIdentifier;
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Predicates;
 import com.google.common.collect.Sets;
 import org.apache.commons.collections4.CollectionUtils;
 import org.apache.commons.collections4.IterableUtils;
@@ -1060,7 +1061,7 @@ private Observable<PersistentResource> getRelation(com.yahoo.elide.core.request.
 
         Optional<Pagination> pagination = Optional.ofNullable(relationship.getProjection().getPagination());
 
-        if (pagination.isPresent() && !pagination.get().isDefaultInstance()
+        if (pagination.filter(Predicates.not(Pagination::isDefaultInstance)).isPresent()
                 && !CanPaginateVisitor.canPaginate(relationClass, dictionary, requestScope)) {
             throw new BadRequestException(String.format("Cannot paginate %s",
                     dictionary.getJsonAliasFor(relationClass)));
@@ -1172,10 +1173,10 @@ private static boolean shouldSkipCollection(Type<?> resourceClass, Class<? exten
             RequestScope requestScope) {
         try {
             requestScope.getPermissionExecutor().checkUserPermissions(resourceClass, annotationClass);
+            return false;
         } catch (ForbiddenAccessException e) {
             return true;
         }
-        return false;
     }
 
     /**

elide-core/src/main/java/com/yahoo/elide/core/RequestScope.java

@@ -25,6 +25,7 @@
 import com.yahoo.elide.core.type.Type;
 import com.yahoo.elide.jsonapi.JsonApiMapper;
 import com.yahoo.elide.jsonapi.models.JsonApiDocument;
+import org.apache.commons.collections.MapUtils;
 import io.reactivex.Observable;
 import io.reactivex.subjects.PublishSubject;
 import io.reactivex.subjects.ReplaySubject;
@@ -138,11 +139,11 @@ public RequestScope(String baseUrlEndPoint,
                 ? new ActivePermissionExecutor(this)
                 : permissionExecutorGenerator.apply(this);
 
-        this.queryParams = (queryParams == null || queryParams.size() == 0)
+        this.queryParams = MapUtils.isEmpty(queryParams)
                 ? Optional.empty()
                 : Optional.of(queryParams);
 
-        this.requestHeaders = (requestHeaders == null || requestHeaders.size() == 0)
+        this.requestHeaders = MapUtils.isEmpty(requestHeaders)
                 ? Collections.emptyMap()
                 : requestHeaders;
         registerPreSecurityObservers();

elide-core/src/main/java/com/yahoo/elide/core/dictionary/EntityDictionary.java

@@ -59,6 +59,7 @@
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import org.antlr.v4.runtime.tree.ParseTree;
+import org.apache.commons.collections4.CollectionUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.tuple.Pair;
 import lombok.Getter;
@@ -136,7 +137,7 @@ public class EntityDictionary {
      *               to their implementing classes
      */
     public EntityDictionary(Map<String, Class<? extends Check>> checks) {
-        this(checks, Collections.EMPTY_SET);
+        this(checks, Collections.emptySet());
     }
 
     /**
@@ -182,7 +183,7 @@ public EntityDictionary(Map<String, Class<? extends Check>> checks, Set<Type<?>>
      *                 initialize Elide models.
      */
     public EntityDictionary(Map<String, Class<? extends Check>> checks, Injector injector) {
-        this(checks, injector, Collections.EMPTY_SET);
+        this(checks, injector, Collections.emptySet());
     }
 
     /**
@@ -204,7 +205,7 @@ public EntityDictionary(Map<String, Class<? extends Check>> checks, Injector inj
     public EntityDictionary(Map<String, Class<? extends Check>> checks,
                             Injector injector,
                             Function<Class, Serde> serdeLookup) {
-        this(checks, injector, serdeLookup, Collections.EMPTY_SET);
+        this(checks, injector, serdeLookup, Collections.emptySet());
     }
 
     public EntityDictionary(Map<String, Class<? extends Check>> checks,
@@ -1365,11 +1366,8 @@ public AccessibleObject getAccessibleObject(Object target, String fieldName) {
     public boolean isComputed(Type<?> entityClass, String fieldName) {
         AccessibleObject fieldOrMethod = getAccessibleObject(entityClass, fieldName);
 
-        if (fieldOrMethod == null) {
-            return false;
-        }
-
-        return (fieldOrMethod.isAnnotationPresent(ComputedAttribute.class)
+        return fieldOrMethod != null
+                && (fieldOrMethod.isAnnotationPresent(ComputedAttribute.class)
                 || fieldOrMethod.isAnnotationPresent(ComputedRelationship.class));
     }
 
@@ -1429,7 +1427,7 @@ public void scanForSecurityChecks() {
      */
     public void addSecurityChecks(Set<Class<?>> classes) {
 
-        if (classes == null || classes.size() == 0) {
+        if (CollectionUtils.isEmpty(classes)) {
             return;
         }
 

elide-core/src/main/java/com/yahoo/elide/core/exceptions/HttpStatusException.java

@@ -77,7 +77,7 @@ private Pair<Integer, JsonNode> buildResponse(String message) {
 
     public String getVerboseMessage() {
         return verboseMessageSupplier.map(Supplier::get)
-                .orElse(getMessage());
+                .orElseGet(this::getMessage);
     }
 
     public int getStatus() {

elide-core/src/main/java/com/yahoo/elide/core/filter/Operator.java

@@ -13,6 +13,9 @@
 import com.yahoo.elide.core.exceptions.InvalidOperatorNegationException;
 import com.yahoo.elide.core.type.Type;
 import com.yahoo.elide.core.utils.coerce.CoerceUtil;
+
+import org.apache.commons.collections4.CollectionUtils;
+
 import lombok.Getter;
 import lombok.RequiredArgsConstructor;
 
@@ -359,9 +362,6 @@ private static <T> Predicate<T> isEmpty(Path fieldPath, RequestScope requestScop
         return (T entity) -> {
 
             Object val = getFieldValue(entity, fieldPath, requestScope);
-            if (val == null) {
-                return false;
-            }
             if (val instanceof Collection<?>) {
                 return ((Collection<?>) val).isEmpty();
             }
@@ -383,9 +383,6 @@ private static <T> Predicate<T> hasMember(Path fieldPath, List<Object> values, R
                     .map(last -> CoerceUtil.coerce(values.get(0), last.getFieldType()))
                     .orElseGet(() -> CoerceUtil.coerce(values.get(0), String.class));
 
-            if (val == null) {
-                return false;
-            }
             if (val instanceof Collection<?>) {
                 return ((Collection<?>) val).contains(filterStr);
             }
@@ -437,7 +434,7 @@ private static <T> Object getFieldValue(T entity, Path fieldPath, RequestScope r
     private static <T> Predicate<T> getComparator(Path fieldPath, List<Object> values,
             RequestScope requestScope, Predicate<Integer> condition) {
         return (T entity) -> {
-            if (values.size() == 0) {
+            if (CollectionUtils.isEmpty(values)) {
                 throw new BadRequestException("No value to compare");
             }
             Object fieldVal = getFieldValue(entity, fieldPath, requestScope);

elide-core/src/main/java/com/yahoo/elide/core/filter/dialect/RSQLFilterDialect.java

@@ -36,6 +36,8 @@
 import com.yahoo.elide.jsonapi.parser.JsonApiParser;
 import com.google.common.collect.ImmutableMap;
 import org.apache.commons.collections4.CollectionUtils;
+import org.apache.commons.lang3.StringUtils;
+
 import cz.jirutka.rsql.parser.RSQLParser;
 import cz.jirutka.rsql.parser.RSQLParserException;
 import cz.jirutka.rsql.parser.ast.AndNode;
@@ -233,7 +235,7 @@ public FilterExpression parseFilterExpression(String expressionText,
                                                   boolean coerceValues,
                                                   boolean allowNestedToManyAssociations) throws ParseException {
         return parseFilterExpression(expressionText, entityType, coerceValues,
-                allowNestedToManyAssociations, Collections.EMPTY_SET);
+                allowNestedToManyAssociations, Collections.emptySet());
 
     }
 
@@ -288,7 +290,7 @@ public class RSQL2FilterExpressionVisitor implements RSQLVisitor<FilterExpressio
         private Set<Attribute> attributes;
 
         public RSQL2FilterExpressionVisitor(boolean allowNestedToManyAssociations) {
-            this(allowNestedToManyAssociations, true, Collections.EMPTY_SET);
+            this(allowNestedToManyAssociations, true, Collections.emptySet());
         }
 
         public RSQL2FilterExpressionVisitor(boolean allowNestedToManyAssociations,
@@ -357,7 +359,7 @@ private Path buildPath(Type rootEntityType, String selector) {
         }
 
         private void parseArguments(String argsString, Set<Argument> arguments) throws UnsupportedEncodingException {
-            if (argsString == null || argsString.isEmpty()) {
+            if (StringUtils.isEmpty(argsString)) {
                 return;
             }
 
@@ -540,7 +542,7 @@ private FilterExpression equalityExpression(String argument, Path path,
                 return new FilterPredicate(path, op, Collections.singletonList(value));
             }
 
-            Boolean isStringLike = path.lastElement()
+            boolean isStringLike = path.lastElement()
                     .filter(e -> e.getFieldType().isAssignableFrom(STRING_TYPE))
                     .isPresent();
             if (isStringLike) {

elide-core/src/main/java/com/yahoo/elide/core/pagination/PaginationImpl.java

@@ -11,6 +11,9 @@
 import com.yahoo.elide.core.request.Pagination;
 import com.yahoo.elide.core.type.ClassType;
 import com.yahoo.elide.core.type.Type;
+
+import com.google.common.collect.ImmutableMap;
+
 import lombok.EqualsAndHashCode;
 import lombok.Getter;
 import lombok.Setter;
@@ -48,14 +51,12 @@ public enum PaginationKey { offset, number, size, limit, totals }
     // For requesting total pages/records be included in the response page meta data
     public static final String PAGE_TOTALS_KEY = "page[totals]";
 
-    public static final Map<String, PaginationKey> PAGE_KEYS = new HashMap<>();
-    static {
-        PAGE_KEYS.put(PAGE_NUMBER_KEY, PaginationKey.number);
-        PAGE_KEYS.put(PAGE_SIZE_KEY, PaginationKey.size);
-        PAGE_KEYS.put(PAGE_OFFSET_KEY, PaginationKey.offset);
-        PAGE_KEYS.put(PAGE_LIMIT_KEY, PaginationKey.limit);
-        PAGE_KEYS.put(PAGE_TOTALS_KEY, PaginationKey.totals);
-    }
+    public static final Map<String, PaginationKey> PAGE_KEYS = ImmutableMap.of(
+            PAGE_NUMBER_KEY, PaginationKey.number,
+            PAGE_SIZE_KEY, PaginationKey.size,
+            PAGE_OFFSET_KEY, PaginationKey.offset,
+            PAGE_LIMIT_KEY, PaginationKey.limit,
+            PAGE_TOTALS_KEY, PaginationKey.totals);
 
     @Getter
     @Setter

elide-core/src/main/java/com/yahoo/elide/core/security/checks/prefab/Collections.java

@@ -69,6 +69,6 @@ private static boolean collectionIsSuperset(Collection<?> baseCollection, Collec
     }
 
     private static boolean changeSpecIsCollection(Optional<ChangeSpec> changeSpec) {
-        return changeSpec.isPresent() && changeSpec.get().getModified() instanceof Collection;
+        return changeSpec.filter(c -> c.getModified() instanceof Collection).isPresent();
     }
 }

elide-core/src/main/java/com/yahoo/elide/core/security/permissions/PermissionCondition.java

@@ -112,12 +112,11 @@ public static PermissionCondition create(
 
     @Override
     public String toString() {
-        Object entity = resource.isPresent() ? resource.get() : entityClass;
+        Object entity = ((Optional) resource).orElse(entityClass);
 
-        String withChanges = changes.isPresent() ? String.format("WITH CHANGES %s", changes.get()) : "";
-        String withField = field.isPresent() ? String.format("WITH FIELD %s", field.get()) : "";
-
-        String withClause = withChanges.isEmpty() ? withField : withChanges;
+        String withClause = changes.map(c -> String.format("WITH CHANGES %s", c))
+                .orElseGet(() -> field.map(f -> String.format("WITH FIELD %s", f))
+                .orElse(""));
 
         return String.format(
                 "%s PERMISSION WAS INVOKED ON %s %s",