Support status 500

[nykolaslima]

closes #912

[Turini]

You need this to use with services or something? (because we typically only throw
an exception to send a server error status). But your code looks great, it breaks the
public interface Status, but where're going to 4.2, so I don't think it's a big problem.

[nykolaslima]

Hi Turini,

Yes I need it. If you are using a micro services architecture, your
endpoint may call another API and if something goes wrong you want to
return a internal server error. It's more explicit than throw any
exception. Throwing an exception to return 500 looks like a hack, don't you
think?
On qui, 15 de jan de 2015 at 09:35 Rodrigo Turini [email protected]
wrote:

You need this to use with services or something? (because we typically
only throw
an exception to send a server error status). But your code looks great, it
breaks the
public interface Status, but where're going to 4.2, so I don't think it's
a big problem.

—
Reply to this email directly or view it on GitHub
#926 (comment).

[Turini]

Sure, it makes sense for me (explicitly sending an 500 status code).
Lets wait a few hours to see if we get more reviews, so we can 🚀 it

[nykolaslima]

Ok, thank you! Let's wait for more reviews.
On qui, 15 de jan de 2015 at 09:46 Rodrigo Turini [email protected]
wrote:

Sure, it makes sense for me (explicitly sending an 500 status code).
Lets wait a few hours to see if we get more reviews, so we can [image:
🚀] it

—
Reply to this email directly or view it on GitHub
#926 (comment).

[lucascs]

I disagree.
If something goes wrong, and it's the client's fault, we return a 4xx status.
If it's the server's fault, it should not be something normal, i.e, it's an exception!!!

So throwing an Exception is one correct way of modeling it, specially because the client can't do anything about it.

[nykolaslima]

Lucas, if you are the server and need to consume a 3rd party API. You know
it can return 500 and lets assume that if it returns 500, we, the server,
want to thrown an APIException.
We cant just throw it way with nobody to catch it.
So we create a interceptor, that will catch this exception and decide what
to do, in my case we want to return 500 and log the error.

We cant just thrown an exception without anyone to catch it, expecting
vraptor to catch it and return 500.
On qui, 15 de jan de 2015 at 23:55 Lucas Cavalcanti <
[email protected]> wrote:

I disagree.
If something goes wrong, and it's the client's fault, we return a 4xx
status.
If it's the server's fault, it should not be something normal, i.e, it's
an exception!!!

So throwing an Exception is one correct way of modeling it, specially
because the client can't do anything about it.

—
Reply to this email directly or view it on GitHub
#926 (comment).

[lucascs]

Do you want to return 500 with an empty response?
Do you want to return a XML/JSON or HTML body with some kind of explanation?

If we want to add this method, we need to serialize something. Otherwise letting the exception blow up will return 500 with the 500 page configured on the server, that's almost equivalent to having an empty response.

Can you add a cookbook with the exception interceptor you have in mind? Serializing something about the interceptor in JSON, for example.

[nykolaslima]

@lucascs Actually, in my use case, I want to return an empty response.

But I want to log the error and be explicit in my code that I'm returning 500 as response. I think that just throwing an exception without anyone to handle it can confuse other developers. They will think: "what's happening with this exception? Who catch this? What's the response after this?"

About the 500 page, the server will not render it because the client will ask for a JSON with the header accepts: application/json

[lucascs]

There are two types of exceptions: recoverable and not recoverable
For the recoverable ones, you expect someone to catch it and do something else. The not recoverable ones you should just throw and let it go. The dev doesn't have to care about what's happening to it. He can just assume this will generate a "generic error" response.

Anyway, not opposed to add this method, but it would be nice to have the docs/cookbook along with the PR.

[nykolaslima]

Do we have docs for others statuses?
On sex, 16 de jan de 2015 at 13:22 Lucas Cavalcanti <
[email protected]> wrote:

There are two types of exceptions: recoverable and not recoverable
For the recoverable ones, you expect someone to catch it and do something
else. The not recoverable ones you should just throw and let it go. The dev
doesn't have to care about what's happening to it. He can just assume this
will generate a "generic error" response.

Anyway, not opposed to add this method, but it would be nice to have the
docs/cookbook along with the PR.

—
Reply to this email directly or view it on GitHub
#926 (comment).

[csokol]

oops, sorry

[csokol]

Do you guys think that makes sense to send a 502 response to the client?

10.5.3 502 Bad Gateway
The server, while acting as a gateway or proxy, received an invalid response from the upstream server it accessed in attempting to fulfill the request.

[nykolaslima]

I think that makes sense. Indeed, I think that VRaptor's users should be able to return any status that they want to. We have many scenarios in web applications and thinking about REST services we are using HTTP status codes more and more.

[csokol]

I think you can do something like result.use(Results.http()).status(xxx).

On Fri Jan 16 2015 at 2:40:26 PM Nykolas Laurentino de Lima <
[email protected]> wrote:

I think that makes sense. Indeed, I think that VRaptor's users should be
able to return any status that they want to. We have many scenarios in web
applications and thinking about REST services we are using HTTP status
codes more and more.

—
Reply to this email directly or view it on GitHub
#926 (comment).

[nykolaslima]

@csokol but if we already have status that gives us a more "semantic" way to do it, why don't allow it?

[csokol]

Ok, I think that makes sense to create this new method. But then I think we
should create methods for all the others http response codes, right?

On Fri Jan 16 2015 at 2:52:31 PM Nykolas Laurentino de Lima <
[email protected]> wrote:

@csokol https://github.com/csokol but if we already have status that
gives us a more "semantic" way to do it, why don't allow it?

—
Reply to this email directly or view it on GitHub
#926 (comment).

[nykolaslima]

But then I think we
should create methods for all the others http response codes, right?

I think this will be the right think to do!

[garcia-jj]

We have almost a hundred http status codes. So will be too hard to add all these codes as methods. You can use HttpServletResponse, that have all codes in constants, instead of creating methods for all http error codes.

[nykolaslima]

I agree with you Otavio. But i think we can implement the most common
statuses. What do you think?
On sex, 16 de jan de 2015 at 20:31 Otávio Garcia [email protected]
wrote:

We have almost a hundred http status codes. So will be too hard to add all
these codes as methods. You can use HttpServletResponse, that have all
codes in constants, instead of creating methods for all http error codes.

—
Reply to this email directly or view it on GitHub
#926 (comment).

[Turini]

we should create methods for all the others http response codes, right?

Sink or swim? I don't think we should add all the other response codes until
users ask for it (and maybe even if they ask). It's just a syntax sugar for the:

result.use(http()).setStatusCode(//anything);

For me it makes sense to send an exception when something goes wrong...
but Nykolas' argument is valid. If you're working with services and want to
explictly send an 500, sounds fair :) About adding docs: the more, the better.

[garcia-jj]

But i think we can implement the most common statuses. What do you think?

We already have: br.com.caelum.vraptor.view.Status.

https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/main/java/br/com/caelum/vraptor/view/Status.java#L32

[nykolaslima]

@garcia-jj I know we have Status class, this PR is about adding it a new method.

[garcia-jj]

@nykolaslima I know the goal of this pull request. I'm only answering your previously question that is:

But i think we can implement the most common statuses. What do you think?

And my anwer is: we already have.

[nykolaslima]

@garcia-jj I see your point but I believe that status 500 is a common status and IMHO it should be in Status class. That is what I meant.

[garcia-jj]

About this point, I agree with @lucascs. Application should send only 4xx status. If something wrong, an exception should be thrown. IMHO, this pull request should not be merged.

[nykolaslima]

@garcia-jj but don't you think that throwing an exception explicit that you know that no body is gonna catch is a good think?

[garcia-jj]

Throwing an exception, IMHO, is better because all servlet implementations converts exceptions in a 5xx headers, and returns properly info to client. I'm not against to add a new method, but I think that application should never calls 500 errors directly.

[nykolaslima]

I disagree because throwing an exception with nobody to catch is a bad
thing. You dont know for sure whats gonna happen. If you can explicit say
that you are returning a 500, IMHO, its better.

But anyway, if nobody is against to add this new method, can we merge it?
On dom, 18 de jan de 2015 at 01:19 Otávio Garcia [email protected]
wrote:

Throwing an exception, IMHO, is better because all servlet implementations
converts exceptions in a 5xx headers, and returns properly info to
client. I'm not against to add a new method, but I think that application
should never calls 500 errors directly.

—
Reply to this email directly or view it on GitHub
#926 (comment).

[Turini]

if nobody is against to add this new method, can we merge it?

+1 to merge it and be happy :) that's just about adding a new short way to send
a status (and it's not that bad, if the user want to send a 500, let's simplify it).

[csokol]

+1 merge

Em 12:22 dom, 18/01/2015, Rodrigo Turini [email protected]
escreveu:

if nobody is against to add this new method, can we merge it?

+1 to merge it and be happy :) that's just about adding a new short way to
send
a status (and it's not that bad, if the user want to send and 500, let's
simplify it).

—
Reply to this email directly or view it on GitHub
#926 (comment).

[lucascs]

🚢

[nykolaslima]

@lucascs @csokol @Turini thank you guys!

[Turini]

Thank you, @nykolaslima!

[nykolaslima]

❤️ 💛