Skullabs / kikaha

A fast middleware designed for microservices
https://skullabs.github.io/kikaha/
Apache License 2.0
59 stars 13 forks source link

DefaultResponse not allows NULL #263

Closed roneigebert closed 3 years ago

roneigebert commented 5 years ago

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

roneigebert commented 5 years ago

@miere I will send a PR for this issue

miere commented 5 years ago

Ronei,

I don't think that we should be able to return null once HTTP has an specific status code 204 (no content) to return that sort of response. Probably we should improve the documentation to avoid that sort of thing, or maybe impose some verification at run time to notify the user that this is an anti-pattern.

If you pick an null-safe language like Closure, Kotlin or Scala you'd never face this sort of problem once they will not allow you to put as value.

On Fri, Mar 29, 2019 at 7:29 AM Ronei notifications@github.com wrote:

@miere https://github.com/miere I will send a PR for this issue

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Skullabs/kikaha/issues/263#issuecomment-477758925, or mute the thread https://github.com/notifications/unsubscribe-auth/AAf20KtcGWz9xAfWWA7CcwbA2uWC8lY8ks5vbSZEgaJpZM4cREy_ .

-- / Miere L. Teixeira /

roneigebert commented 5 years ago

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 5 years ago

By returning NULL you telling your client that you have nothing to return. I'm afraid that is a clear sign that the original design was flawed. Unlike Spring MVC and JaxRS, Kikaha wants to provide a simple way to avoid uncertainty and undesired side-effects, thus the non-requirement. Checking for null inside the framework would take out from developers hand the possibility of the tuning up the server performance by squashing some bits and hence lowering infrastructure and operational cost. To give up a glimpse, here is a common case where a simple extra operation (by checking ordenation using if/else) have high penalty on the execution performance. By giving the versatility of returning null at the framework level would make calls to the Serializer with non-useful return, where a simple 204 would be enough, with less overhead and would take advantage of Http2 new pipeline.

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)

As an exercise to proper understand your problem, I've made a bit of a research on this regard. I found out that, as I was expecting, the most Java HTTP clients have a proper way to cope this situation by providing some sort of Interceptor API (or result handler).

This is a good way to handle different status code using OkHttp and Retrofit. https://futurestud.io/tutorials/retrofit-2-catch-server-errors-globally-with-response-interceptor#globalerrorhandlerokhttpinterceptor

Another approach that it think is more feasible is to allow developers customize the AsyncResponse API by writing a proper global factory. It would ease the global penalty that I've mentioned earlier and, at the same time, providing you the versatility that you need.

Looking forward for hearing from you soon, so I make a patch for you.

Cheers

roneigebert commented 5 years ago

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 4 years ago

@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?

roneigebert commented 4 years ago

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

miere commented 3 years ago

@roneigebert any thoughts, mate?

roneigebert commented 3 years ago

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.

roneigebert commented 3 years ago

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

miere commented 3 years ago

I suppose it might be a good enough solution. Will be waiting for you PR. :D

On Tue, Feb 9, 2021 at 1:46 AM Ronei notifications@github.com wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Skullabs/kikaha/issues/263#issuecomment-775199803, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD7NUHFGTYDERTR3K5D3PLS572MNANCNFSM4HCEJS7Q .

-- / Miere L. Teixeira /

roneigebert commented 3 years ago

@miere done! https://github.com/Skullabs/kikaha/pull/277