Merge pull request #270 from tsegismont/jira/HWKMETRICS-154

List metrics endpoint fails with 500 Internal Server Error when "type" param is missing

api/metrics-api-jaxrs/src/main/java/org/hawkular/metrics/api/jaxrs/handler/MetricHandler.java

@@ -19,6 +19,7 @@
 import static javax.ws.rs.core.MediaType.APPLICATION_JSON;
 
 import static org.hawkular.metrics.api.jaxrs.filter.TenantFilter.TENANT_HEADER_NAME;
+import static org.hawkular.metrics.api.jaxrs.util.ApiUtils.badRequest;
 import static org.hawkular.metrics.api.jaxrs.util.ApiUtils.executeAsync;
 
 import java.util.ArrayList;
@@ -70,33 +71,30 @@ public class MetricHandler {
     @GET
     @Path("/")
     @ApiOperation(value = "Find tenant's metric definitions.", notes = "Does not include any metric values. ",
-            response = List.class, responseContainer = "List")
-    @ApiResponses(value = { @ApiResponse(code = 200, message = "Successfully retrieved at least one metric "
-            + "definition."),
+                  response = List.class, responseContainer = "List")
+    @ApiResponses(value = {
+            @ApiResponse(code = 200, message = "Successfully retrieved at least one metric definition."),
             @ApiResponse(code = 204, message = "No metrics found."),
-            @ApiResponse(code = 400, message = "Given type is not a valid type.", response = ApiError.class),
+            @ApiResponse(code = 400, message = "Missing or invalid type parameter type.", response = ApiError.class),
             @ApiResponse(code = 500, message = "Failed to retrieve metrics due to unexpected error.",
                          response = ApiError.class)
     })
     public void findMetrics(
             @Suspended final AsyncResponse asyncResponse,
-            @ApiParam(
-                    value = "Queried metric type",
-                    required = true,
-                    allowableValues = "[gauge, availability, counter]")
-        @QueryParam("type") String type) {
-
-        try {
-            metricsService.findMetrics(tenantId, MetricType.fromTextCode(type))
-                    .map(MetricDefinition::new)
-                    .toList()
-                    .map(ApiUtils::collectionToResponse)
-                    .subscribe(asyncResponse::resume, t -> asyncResponse.resume(ApiUtils.serverError(t)));
-
-        } catch (IllegalArgumentException e) {
-            ApiError error = new ApiError("[" + type + "] is not a valid type. Accepted values are gauge|avail|log");
-            asyncResponse.resume(Response.status(Response.Status.BAD_REQUEST).entity(error).build());
+            @ApiParam(value = "Queried metric type",
+                      required = true,
+                      allowableValues = "[gauge, availability, counter]")
+            @QueryParam("type") MetricType metricType
+    ) {
+        if (metricType == null) {
+            asyncResponse.resume(badRequest(new ApiError("Missing type param")));
+            return;
         }
+        metricsService.findMetrics(tenantId, metricType)
+                      .map(MetricDefinition::new)
+                      .toList()
+                      .map(ApiUtils::collectionToResponse)
+                      .subscribe(asyncResponse::resume, t -> asyncResponse.resume(ApiUtils.serverError(t)));
     }
 
     @POST

api/metrics-api-jaxrs/src/main/java/org/hawkular/metrics/api/jaxrs/param/ConvertersProvider.java

@@ -23,6 +23,8 @@
 import javax.ws.rs.ext.ParamConverterProvider;
 import javax.ws.rs.ext.Provider;
 
+import org.hawkular.metrics.core.api.MetricType;
+
 import com.google.common.collect.ImmutableMap;
 
 /**
@@ -39,6 +41,7 @@ public ConvertersProvider() {
         paramConverters = paramConvertersBuilder
                 .put(Duration.class, new DurationConverter())
                 .put(Tags.class, new TagsConverter())
+                .put(MetricType.class, new MetricTypeConverter())
                 .build();
     }
 

api/metrics-api-jaxrs/src/main/java/org/hawkular/metrics/api/jaxrs/param/MetricTypeConverter.java

@@ -0,0 +1,39 @@
+/*
+ * Copyright 2014-2015 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.hawkular.metrics.api.jaxrs.param;
+
+import javax.ws.rs.ext.ParamConverter;
+
+import org.hawkular.metrics.core.api.MetricType;
+
+/**
+ * A JAX-RS {@link ParamConverter} for {@link MetricType} parameters.
+ *
+ * @author Thomas Segismont
+ */
+public class MetricTypeConverter implements ParamConverter<MetricType> {
+
+    @Override
+    public MetricType fromString(String s) {
+        return MetricType.fromTextCode(s);
+    }
+
+    @Override
+    public String toString(MetricType metricType) {
+        return metricType.getText();
+    }
+}

api/metrics-api-jaxrs/src/test/java/org/hawkular/metrics/api/jaxrs/param/MetricTypeConverterTest.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright 2014-2015 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.hawkular.metrics.api.jaxrs.param;
+
+import static java.util.stream.Collectors.joining;
+
+import java.util.Arrays;
+
+import org.hawkular.metrics.core.api.MetricType;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+/**
+ * @author Thomas Segismont
+ */
+public class MetricTypeConverterTest {
+
+    @Rule
+    public final ExpectedException expectedException = ExpectedException.none();
+
+    @Test
+    public void shouldThrowIllegalArgumentExceptionWithInvalidText() throws Exception {
+        expectedException.expect(IllegalArgumentException.class);
+
+        String invalidText = Arrays.stream(MetricType.values()).map(MetricType::getText).collect(joining("."));
+        new MetricTypeConverter().fromString(invalidText);
+    }
+}

core/metrics-core-api/src/main/java/org/hawkular/metrics/core/api/MetricType.java

@@ -16,29 +16,24 @@
  */
 package org.hawkular.metrics.core.api;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
+import java.util.Arrays;
+
+import com.google.common.collect.ImmutableMap;
+
 /**
- * An enumeration of the supported metric types which currently includes,
- *
- * <ul>
- *   <li>guage</li>
- *   <li>availability</li>
- *   <li>log events</li>
- * </ul>
+ * An enumeration of the supported metric types.
  *
  * @author John Sanda
  */
 public enum MetricType {
-
     GAUGE(0, "gauge"),
-
     AVAILABILITY(1, "availability"),
-
     COUNTER(2, "counter"),
-
     COUNTER_RATE(3, "counter_rate");
 
     private int code;
-
     private String text;
 
     MetricType(int code, String text) {
@@ -56,27 +51,42 @@ public String getText() {
 
     @Override
     public String toString() {
-        return text;
+        return getText();
+    }
+
+    private static final ImmutableMap<Integer, MetricType> codes;
+
+    static {
+        ImmutableMap.Builder<Integer, MetricType> builder = ImmutableMap.builder();
+        Arrays.stream(values()).forEach(type -> builder.put(type.code, type));
+        codes = builder.build();
+        checkArgument(codes.size() == values().length, "Some metric types have the same code");
+    }
+
+    private static final ImmutableMap<String, MetricType> texts;
+
+    static {
+        ImmutableMap.Builder<String, MetricType> builder = ImmutableMap.builder();
+        Arrays.stream(values()).forEach(type -> builder.put(type.text, type));
+        texts = builder.build();
+        checkArgument(texts.size() == values().length, "Some metric types have the same string value");
     }
 
     public static MetricType fromCode(int code) {
-        switch (code) {
-            case 0 : return GAUGE;
-            case 1 : return AVAILABILITY;
-            case 2 : return COUNTER;
-            case 3 : return COUNTER_RATE;
-            default: throw new IllegalArgumentException(code + " is not a recognized metric type");
+        MetricType type = codes.get(code);
+        if (type == null) {
+            throw new IllegalArgumentException(code + " is not a recognized metric type");
         }
+        return type;
     }
 
     public static MetricType fromTextCode(String textCode) {
-        switch (textCode) {
-            case "gauge": return GAUGE;
-            case "availability": return AVAILABILITY;
-            case "counter": return COUNTER;
-            case "counter_rate": return COUNTER_RATE;
-            default: throw new IllegalArgumentException(textCode + " is not a recognized metric type code");
+        checkArgument(textCode != null, "textCode is null");
+        MetricType type = texts.get(textCode);
+        if (type == null) {
+            throw new IllegalArgumentException(textCode + " is not a recognized metric type");
         }
+        return type;
     }
 
 }

rest-tests/src/test/groovy/org/hawkular/metrics/rest/CassandraBackendITest.groovy

@@ -21,6 +21,7 @@ import static org.joda.time.Seconds.seconds
 import static org.junit.Assert.assertEquals
 import static org.junit.Assert.assertTrue
 
+import org.hawkular.metrics.core.api.MetricType
 import org.hawkular.metrics.core.impl.DateTimeService
 import org.joda.time.DateTime
 import org.junit.Test
@@ -350,6 +351,31 @@ class CassandraBackendITest extends RESTTest {
     )
   }
 
+  @Test
+  void findMetricsShouldFailProperlyWhenTypeIsMissingOrInvalid() {
+    def tenantId = nextTenantId()
+
+    badGet(path: "metrics",
+        headers: [(tenantHeaderName): tenantId]) { exception ->
+      // Missing type
+      assertEquals(400, exception.response.status)
+      assertEquals("Missing type param", exception.response.data["errorMsg"])
+    }
+
+    def invalidType = MetricType.values().join('"')
+    badGet(path: "metrics", query: [type: invalidType],
+        headers: [(tenantHeaderName): tenantId]) { exception ->
+      // Invalid type
+      assertEquals(400, exception.response.status)
+      assertEquals(invalidType + " is not a recognized metric type", exception.response.data["errorMsg"])
+    }
+
+    def response = hawkularMetrics.get(path: "metrics", query: [type: MetricType.GAUGE.text],
+        headers: [(tenantHeaderName): tenantId])
+    // Works fine when type is correctly set (here 204 as not metric has been created)
+    assertEquals(204, response.status)
+  }
+
   @Test
   void findMetrics() {
     DateTime start = now().minusMinutes(20)