When sending read request via POST with a single Attribute and debug is true, request fails

[asafm]

How to create this bug?

1. Start your JVM process with -javaagent:/tmp/jolokia-jvm-1.3.5-agent.jar=debug=true. Emphasis on debug is on.
2. Send a POST request that wants to read a single attribute. Example:

{
  "type" : "read",
  "mbean" : "kafka.cluster:type=Partition,name=UnderReplicated,topic=rxinflux,partition=1",
  "attribute" : [ "Value" ]
}

Expected: The value of of the attribute returned in the response body.
Actual:

{"stacktrace":"java.lang.IllegalStateException: Request contains more than one attribute (attrs = [Value]). Use getAttributeNames() instead.\n\tat org.jolokia.request.JmxReadRequest.getAttributeName(JmxReadRequest.java:83)\n\tat org.jolokia.request.JmxReadRequest.appendReadParameters(JmxReadRequest.java:182)\n\tat org.jolokia.request.JmxReadRequest.toString(JmxReadRequest.java:134)\n\tat org.jolokia.http.HttpRequestHandler.handlePostRequest(HttpRequestHandler.java:128)\n\tat org.jolokia.jvmagent.handler.JolokiaHttpHandler.executePostRequest(JolokiaHttpHandler.java:279)\n\tat org.jolokia.jvmagent.handler.JolokiaHttpHandler.doHandle(JolokiaHttpHandler.java:233)\n\tat org.jolokia.jvmagent.handler.JolokiaHttpHandler.handle(JolokiaHttpHandler.java:178)\n\tat com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:79)\n\tat sun.net.httpserver.AuthFilter.doFilter(AuthFilter.java:83)\n\tat com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:82)\n\tat sun.net.httpserver.ServerImpl$Exchange$LinkHandler.handle(ServerImpl.java:675)\n\tat com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:79)\n\tat sun.net.httpserver.ServerImpl$Exchange.run(ServerImpl.java:647)\n\tat java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)\n\tat java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)\n\tat java.lang.Thread.run(Thread.java:745)\n","error_type":"java.lang.IllegalStateException","error":"java.lang.IllegalStateException : Request contains more than one attribute (attrs = [Value]). Use getAttributeNames() instead.","status":500}

I tracked the bug to JmxReadRequest:

    // initialize and detect multi attribute mode
    private void initAttribute(Object pAttrval) {
        if (pAttrval instanceof String) {
            attributeNames = EscapeUtil.split((String) pAttrval,EscapeUtil.CSV_ESCAPE,",");
            multiAttributeMode = attributeNames.size() > 1;
        } else if (pAttrval instanceof Collection) {
            Collection<String> attributes = (Collection<String>) pAttrval;
            if (attributes.size() == 1 && attributes.iterator().next() == null) {
                attributeNames = null;
                multiAttributeMode = false;
            } else {
                attributeNames = new ArrayList<String>(attributes);
                multiAttributeMode = true;
            }
        } else if (pAttrval == null) {
            attributeNames = null;
            multiAttributeMode = false;
        } else {
            throw new IllegalArgumentException("Attribute names must be either a String, Collection or null (and not " + pAttrval.getClass() + ")");
        }
    }

The fixed code should be:

            if (attributes.size() == 1 && attributes.iterator().next() == null) {
                attributeNames = null;
                multiAttributeMode = false;
            } else {
                attributeNames = new ArrayList<String>(attributes);
                multiAttributeMode = (attributeNames.size > 1);
            }

Also another bug here is that the status code returned is 200 although it's a bug. The reader has no way of telling whether to read a single JSON containing an error or a JSON Array of the result in case you request multiple read requests. The status code in this case should have been 500.

[rhuss]

Agree to your analysis w.r.t to the attribute names, will fix that asap.

w.r.t to the HTTP status code, we take a bit a different approach then REST:

• The HTTP status code reflects the status of the HTTP request processing itself. A 200 means that there is no error in processing the request. This does not mean that every JMX request suceeded.
• When the JMX request fails (because the MBean throws an error or the request invalid, then the status field for the JSON Response object is set to a value different to 200. In this case Jolokia thought that the request format was invalid, therefor it sets the status attribute to 500. But the HTTP return code is still 200.

The reason for this is that you can have bulk requests where some JMX requests can succeed and some can fail.

This is also explained in the reference manual:

For status codes it is important to distinguish status codes as they appear in Jolokia JSON response objects and the HTTP status code of the (outer) HTTP response. There can be many Jolokia status codes, one for each Jolokia request contained in the single HTTP request. The HTTP status code merely reflect the status of agent itself (i.e. whether it could perform the operation at all), whereas the Jolokia response status reflects the result of the operation (e.g. whether the performed operation throws an exception). So it is not uncommon to have an HTTP status code of 200, but the contained JSON response(s) indicate some errors.

And as said, Jolokia is not REST. See also this Blog Post for some reasoning why not.

[rhuss]

Sorry, was a bit too fast.

Actual the behaviour was correct, that when you use a list of attributes you should use a getAttributeNames() even when there is only one attribute. This simply distinguishes the different usage patterns: Setting a list as attributes --> getAttributeNames(), setting a scalar attribute --> getAttribute().

JmxReadRequest is mostly an internal object anyway.

The bug actually is in the JmxReadRequest.toString() method which should check for isMultiAttributeMode() instead of checking the length of the list. The bug was triggered because debug was switched on.

[asafm]

Thanks for the very fast fix!
Can I know what is the release schedule to know when I can deploy this fix?

Regarding the status code explanation. I'm perfectly fine with your approach. The only problem I have with it is that I have no way to know when to read a JSON object vs a JSON array.

When I issue a read, I do this:

            HttpResponse httpResponse = Post(jolokiaFullURL+"read?ignoreErrors=true&canonicalNaming=false")
                    .connectTimeout(connectTimeout)
                    .socketTimeout(socketTimeout)
                    .bodyString(requestBody, ContentType.APPLICATION_JSON)
                    .execute().returnResponse();

            if (httpResponse.getStatusLine().getStatusCode() != 200) {
                throw new RuntimeException("Failed reading beans from jolokia. Response = "+httpResponse.getStatusLine());
            }

            String responseBody =  IOUtils.toString(httpResponse.getEntity().getContent(), "UTF-8");

            if (logger.isTraceEnabled()) logger.trace("Jolokia getBeans response:\n{}", responseBody);

            ArrayList<Map<String, Object>> responses = objectMapper.readValue(responseBody, ArrayList.class);

I expect the response to be an ArrayList.

But when the same request fails I do not get an array list but a JSON object.

If the response were always a JSON object containing a statusCode field and then a body field which contains the actual result I can do an if.

This is the example of correct result:

[{"request":{"mbean":"java.util.logging:type=Logging","attribute":["LoggerNames","ObjectName"],"type":"read"},"value":{"LoggerNames":["org.glassfish.jersey.process.internal

This is an example of the result in case of failure:

{"stacktrace":"java.lang.IllegalStateException: Request contains more than one attribute (attrs = [Value]). Use getAttributeNames() instead.\n\tat org.jolokia.request.JmxReadRequest.getAttributeName(JmxReadRequest.java:83)\n\tat org.jolokia.request.JmxReadRequest.appendReadParameters(JmxReadRequest.java:182)\n\tat org.jolokia.request.JmxReadRequest.toString(JmxReadRequest.java:134)\n\tat org.jolokia.http.HttpRequestHandler.handlePostRequest(HttpRequestHandler.java:128)\n\tat org.jolokia.jvmagent.handler.JolokiaHttpHandler.executePostRequest(JolokiaHttpHandler.java:279)\n\tat org.jolokia.jvmagent.handler.JolokiaHttpHandler.doHandle(JolokiaHttpHandler.java:233)\n\tat org.jolokia.jvmagent.handler.JolokiaHttpHandler.handle(JolokiaHttpHandler.java:178)\n\tat com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:79)\n\tat sun.net.httpserver.AuthFilter.doFilter(AuthFilter.java:83)\n\tat com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:82)\n\tat sun.net.httpserver.ServerImpl$Exchange$LinkHandler.handle(ServerImpl.java:675)\n\tat com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:79)\n\tat sun.net.httpserver.ServerImpl$Exchange.run(ServerImpl.java:647)\n\tat java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)\n\tat java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)\n\tat java.lang.Thread.run(Thread.java:745)\n","error_type":"java.lang.IllegalStateException","error":"java.lang.IllegalStateException : Request contains more than one attribute (attrs = [Value]). Use getAttributeNames() instead.","status":500}

How do you recommend I will parse the body? How to know when to expect a JSON Object or an ARRAY?

[rhuss]

Completely agreed that the JSON structure should not be different for success or not. And normally this is not different, but in this case, where an internal error occurred (which was a bug in the toString() method) then there is a final catch around at the top-level handler method. So I expect to happen this only for bugs.

For this case it should be fixed, but I will have a closer look whether it can be made even more bullet proof.

w.r.t to a release date: A 1.3.6 release is already overdue, so I will release one soon. But unfortunately I'm quite stacked with work so no promises. But I think this should happen until the end of next week.

[rhuss]

Actually we should be quite save.

If you look at the loop over all requests you will see that everything within the executeRequest() method is guarded completely with catches, the only thing not catched in this loop over all requests it the debug statement. And actually this caused the trouble here. If this is safe (hope so now), then I don't expect any further issues w.r.t to the JSON response structure.