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

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 RE

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 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.

StackExchange Code Review Q#94222, answer score: 3