Skullabs/kikaha

DefaultResponse not allows NULL

roneigebert opened this issue · 12 comments

I have the following code:

@GET
@Path("/api/case-1")
public Object returnCase1(){
    return null;
}

@GET
@Path("/api/case-2")
public Response returnCase2(){
    return DefaultResponse.ok().entity( null );
}

@GET
@Path("/api/case-3")
public Response returnCase3(){
    return DefaultResponse.ok();
}

@GET
@Path("/api/case-4")
public void returnCase4(AsyncResponse asyncResponse) throws Exception {
    val response =  DefaultResponse.ok();
    val entityField = response.getClass().getDeclaredField( "entity" );
    entityField.setAccessible( true );
    entityField.set( response, null );
    asyncResponse.write( response );
}

Case 1: OK - returns NULL
Case 2: NO OK - NullPointerException because DefaultResponse not allows NULL
Case 3: NO OK - It returns a empty string - I think the correct return should be NULL
Case 4: NO OK - The way I found to return NULL using AsyncResponse

@miere I will send a PR for this issue

miere commented

Yes, I agree 204 is the best code for resources that not returns any content (normally on edit objects - with PUT operation), but, in my case I need to return a valid JSON value (JSON allows null), and in some cases it can return null. I can't create a other status code because I need to change all my clients to create a "IF ELSE" (for status code - 204/200).

The "case 1" has work great for me for a long time, but, recently I changed my app to use AsynResponse and kikaha has banned the resource to return NULL.

miere commented

Miere, thanks for your answer

I understand returning 204 is faster (return 2 bytes less - "" instead null), but, in my case the cost of this (use 200 and 204 in the same resource - and change all my clients) is much bigger than return null.

Is this a good pattern or not, I think kikaha should not be impose any limitation for the kikaha lovers.

I don't understand your last suggestion, but if this solve the problem I think it is a good idea haha. Now I have a new ideia/suggestion: Wouldn't it be easier create additional method on AsynResponse like asynResponse.writeObject( <anyObjectType> ) with the same behavior of the "case 1" (described in the first issue comment)?

And sorry for my English, I'm learning it rsrs

miere commented

@roneigebert I'm leaning to relax the limitations of this "null" issue.
Do you want to proceed with a PR or shall I do it by myself?

Of course @miere . I can't do it now but next week I'll send a PR ;)

miere commented

@roneigebert any thoughts, mate?

Sorry @miere I created a "temporary solution" (maybe not so temporary, I'm using that for a year rsrs) using a custom class/serializer and so I forgot to send a PR.. I'll send a PR tomorrow.

@miere I just removed the NonNull without changing the default response (an empty string). Any other idea or it's ok?

miere commented