Adding proper validations on all the parameters coming from the URL in Rest Service

Question

I am working on a RestService in which my URL will take lot of parameters. Below are the parameters my URL will take:

• UserId: This should always be a long and a number in general.
• DeviceId: This should always be a String.
• PetaId: This should always be a String.
• FlowId: This should be a long as well and a number in general.
• Timeout: This should be a long as well and a number in general.
• DataFlag: This should be a boolean, true or false.
• ProcessFlag: This should be a boolean, true or false.

Now I need to do validations on these above properties and if they are not passing correct stuff, then I need to throw proper JSON response with BAD REQUEST error message.

• If anyone is passing UserId in the URL, then it should not be empty or null and it should be a proper number.
• If anyone is passing DeviceId in the URL, then it should not be empty or null.
• If anyone is passing PetaId in the URL, then it should not be empty or null.
• If anyone is passing FlowId in the URL, then it should not be empty or null and it should be a proper number.
• If anyone is passing Timeout in the URL, then it should not be negative or zero.
• If anyone is passing DataFlag in the URL, then it should be a boolean value.
• If anyone is passing ProcessFlag in the URL, then it should be a boolean value.

Point is if they are setting any of the above fields, they should set it with proper values otherwise they should not. If any of the above cases fails, then I need to return back a proper JSON error response along with BAD REQUEST 400 message so that they can understand what went wrong.

For example as shown below or if there is anything better, I am open to that as well:

{"error": "user id cannot be null or empty"}

Now there are two more important things which are mandatory always:

• FlowId should always be set. If FlowId is not set, like if it is empty or null or zero then I need to return BAD REQUEST and with proper JSON error message like FlowId cannot be null or empty or anything better.
• Also at least one of the keys from UserId, DeviceId and PetaId should be valid and set. If all of them are missing/invalid then I need to return BAD REQUEST with proper JSON error message like "all three keys cannot be null or empty" or anything better.

After extracting all the stuff from the URL, I need to pass these values to another library (DataClient) which takes these parameter.

• If UserId is not empty and not null and a valid number, then pass this value to the library.
• If DeviceId is not empty and not null, then pass this value to the library.
• If PetaId is not empty and not null, then pass this value to the library.
• If Timeout is present in the URL with a valid value, then pass this value, otherwise use the default value of 200.
• If DataFlag is present in the URL with a valid boolean value, then pass this value, otherwise don't set it if DataFlag is not present.
• If ProcessFlag is present in the URL with valid a boolean value, then pass this value, otherwise don't set it if ProcessFlag is not present.

As an example if UserId is empty or null and then if we try to pass this value to the library, then library will throw exception that "UserId cannot be null or empty", so that's why I needed to have all these checks.

Below is my code where I am extracting all the stuff out of it from the URL:

@GET
@Path("/json/lime")
@Produces(MediaType.APPLICATION_JSON)
public Response getData(@Context UriInfo uriInfo) {
    String client = getClientId();
    if (DataUtils.isEmpty(client)) {
        return Response.status(HttpServletResponse.SC_BAD_REQUEST)
                .entity("{\"error\": \"client id is null or empty\"}").build();
    }

    MultivaluedMap<String, String> m = uriInfo.getQueryParameters();
    String flowid = null;
    if (m.get("flowid") != null) {
        flowid = m.get(flowid).get(0);
    }
    if (DataUtils.isEmpty(flowid)) {
        throw new WebApplicationException(Response.status(HttpServletResponse.SC_BAD_REQUEST)
                .type(MediaType.APPLICATION_JSON).entity("{\"error\": \"flow id cannot be null or empty\"}")
                .build());
    }
    String userId = null;
    if (m.get("userid") != null) {
        userId = m.get("userid").get(0);
        if (DataUtils.isEmpty(userId)) {
            throw new WebApplicationException(Response.status(HttpServletResponse.SC_BAD_REQUEST)
                    .type(MediaType.APPLICATION_JSON).entity("{\"error\": \"user id cannot be null or empty\"}")
                    .build());
        }
    }
    String deviceid = null;
    if (m.get("deviceid") != null) {
        deviceid = m.get("deviceid").get(0);
        if (DataUtils.isEmpty(deviceid)) {
            throw new WebApplicationException(Response.status(HttpServletResponse.SC_BAD_REQUEST)
                    .type(MediaType.APPLICATION_JSON).entity("{\"error\": \"device id cannot be null or empty\"}")
                    .build());
        }
    }
    String petaid = null;
    if (m.get("petaid") != null) {
        petaid = m.get("petaid").get(0);
        if (DataUtils.isEmpty(petaid)) {
            throw new WebApplicationException(Response.status(HttpServletResponse.SC_BAD_REQUEST)
                    .type(MediaType.APPLICATION_JSON).entity("{\"error\": \"peta id cannot be null or empty\"}")
                    .build());
        }
    }

    if (DataUtils.isEmpty(userId) && DataUtils.isEmpty(petaid) && DataUtils.isEmpty(deviceid)) {
        throw new WebApplicationException(Response.status(HttpServletResponse.SC_BAD_REQUEST)
                .type(MediaType.APPLICATION_JSON).entity("{\"error\": \"all three keys cannot be null or empty\"}")
                .build());
    }

    long timeout = 200;
    if (m.get("timeout") != null) {
        try {
            timeout = Long.parseLong(m.get("timeout").get(0));
            if (timeout <= 0) {
                throw new WebApplicationException(Response.status(HttpServletResponse.SC_BAD_REQUEST)
                        .type(MediaType.APPLICATION_JSON).entity("{\"error\": \"invalid timeout is passed\"}")
                        .build());
            }
        } catch (NumberFormatException ex) {
            throw new WebApplicationException(Response.status(HttpServletResponse.SC_BAD_REQUEST)
                    .type(MediaType.APPLICATION_JSON).entity("{\"error\": \"invalid timeout is passed\"}").build());
        }
    }

    boolean dataFlag = false;
    if (m.get("dataflag") != null) {
        if (Boolean.parseBoolean(m.get("dataflag").get(0))) {
            dataFlag = true;
        }
    }

    boolean processFlag = true;
    if (m.get("processflag") != null) {
        if (!Boolean.parseBoolean(m.get("processflag").get(0))) {
            processFlag = false;
        }
    }

    //now build headers
    org.springframework.http.HttpHeaders reqHeader = new org.springframework.http.HttpHeaders();
    reqHeader.add("Context", client);
    HttpEntity<Object> entity = new HttpEntity<Object>(reqHeader);

    //Now send the request to DataClient
    DataKey key = null;
    Builder builder = null;
    try {
        builder = new DataKey.Builder(Integer.parseInt(flowid));
        if (!DataUtils.isEmpty(userId)) {
            builder.setUserId(Long.parseLong(userId));
        }
        if (!DataUtils.isEmpty(deviceid)) {
            builder.setCguid(deviceid);
        }
        if (!DataUtils.isEmpty(petaid)) {
            builder.setUdid(petaid);
        }
        if (dataFlag) {
            builder.dataFlag(dataFlag);
        }
        if (!processFlag) {
            builder.processFlag(processFlag);
        }
        key = builder.addTimeout(timeout).attributeMap(convertToMap(m))
                .addHeaders(entity).build();
    } catch (Exception e) {
        throw new WebApplicationException(Response.status(HttpServletResponse.SC_BAD_REQUEST)
                .type(MediaType.APPLICATION_JSON).entity("{\"error\": \""+e.getMessage()"\"}")
                .build());
    }

    DataResponse resp = client.getDataHistory(key);

    // if request is unsuccessful
    if (resp.getStatus().ordinal() != 0) {
        int errorCode = resp.getError().getCode();
        throw new WebApplicationException(Response.status(resp.getError().getCode())
                .type(MediaType.APPLICATION_JSON).entity("resp.getResponse()")
                .build());
    }

    // if request was successful
    return Response
            .status(resp.getError().getCode())
            .entity(resp.getResponse()).build();
}

Here is the DataResponse class which contains actual response and error code and then whether it's a successful response or not.

public class DataResponse {
    private final String response;
    private final ErrorCode error;
    private final StatusCode status;

    // constructors/getters     
}

The StatusCode class contains two properties:

public enum StatusCode {
    SUCCESS, ERROR;
}

The ErrorCode class contains all the status code with proper error messages:

public enum ErrorCode {

    OK(200, "NONE", ),
    NO_CONTENT(204, "No Content"),
    BAD_REQUEST(400, "Bad Request")

    // other customized error messages
}

I am trying to see whether there is any improvement I can do in my rest service code as shown above.

Answer 1

It doesn't look bad, but it's very repetitive. I see

    if (DataUtils.isEmpty(XXX)) {
        throw new WebApplicationException(Response.status(HttpServletResponse.SC_BAD_REQUEST)
                .type(MediaType.APPLICATION_JSON).entity("{\"error\": \"XXX cannot be null or empty\"}")
                .build());
    }

a few million times. Except the very first time, where you just

    return Response.status(HttpServletResponse.SC_BAD_REQUEST)
            .entity("{\"error\": \"client id is null or empty\"}").build();

instead of throwing. To me it looks like a bug, though I may have missed something in your description.

---

You should have a method like

void checkCondition(boolean condition, String errorMessage) {
    if (!condition) {
       throw new WebApplicationException(
           Response.status(HttpServletResponse.SC_BAD_REQUEST)
              .type(MediaType.APPLICATION_JSON)
              .entity("{\"error\": \"" + errorMessage + "\"}")
              .build());
}

and use it like

String client = getClientId();
checkCondition(!DataUtils.isEmpty(client), "client id is null or empty");

You may want to define more specific constraints like checkNonEmptyAndNonNull, but this is possibly past the point of diminishing returns.

---

String flowid = null;
if (m.get("flowid") != null) {
    flowid = m.get(flowid).get(0);
}

This pattern occurs a lot and I really don't like it. You could write

String flowid = m.get("flowid") == null ? null : m.get(flowid).get(0);

or better write a helper method like

String getFromStupidMap(MultivaluedMap<String, String> m, String key) {
    WhateverStupidType value = m.get(key);
    return value == null ? null : value.get(0);
}

and use it like

String flowid = getFromStupidMap(m, "flowid");

---

What would you do if m contained a key multiple times?

---

        timeout = Long.parseLong(m.get("timeout").get(0));

Consider using Guava's tryParse.

    } catch (NumberFormatException ex) {

What if m.get("timeout").get(0) returns null?

As an example if UserId is empty or null and then if we try to pass this value to the library, then library will throw exception that "UserId cannot be null or empty", so that's why I needed to have all these checks.

I'm not sure about that. For me, passing

whateverurl?userid=&foo=bar

should be equivalent to

whateverurl?foo=bar

When the library doesn't eat it, then you obviously must prepare the data for it, but imposing such a constraint on anyone else IMHO doesn't make much sense.

---

You're also dealing with null and empty strings a lot, and I'd suggest to normalize everything by converting nulls to empty strings ASAP.