New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SetStatus (int sc, java.lang.String sm) of HttpServletResponse is deprecated #709

Closed
magnolia-k opened this Issue Oct 8, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@magnolia-k
Contributor

magnolia-k commented Oct 8, 2017

[warn] /Users/xxx/scalatra/core/src/main/scala/org/scalatra/servlet/RichResponse.scala:21: method setStatus in trait HttpServletResponse is deprecated: see corresponding Javadoc for more information.
[warn]     res.setStatus(statusLine.code, statusLine.message)
[warn]         ^

SetStatus with two arguments is deprecated.

If resutl code is 4xx or 5xx, if you want to set status line, you should change body so that it does not send body, but directly returns result using sendError method.

However, because of the HTTP protocol standard, there is no such limitation in the status line, so I do not know if it is correct to be deprecated...

@magnolia-k

This comment has been minimized.

Show comment
Hide comment
@magnolia-k

magnolia-k Oct 8, 2017

Contributor

It is not inconsistent with the existing test code in the following changes

core/src/main/scala/org/scalatra/ScalatraBase.scala

-          response.status = ResponseStatus(status, reason)
+          response.sendError(status, reason)

core/src/main/scala/org/scalatra/servlet/RichResponse.scala

-    res.setStatus(statusLine.code, statusLine.message)
+    res.setStatus(statusLine.code)
Contributor

magnolia-k commented Oct 8, 2017

It is not inconsistent with the existing test code in the following changes

core/src/main/scala/org/scalatra/ScalatraBase.scala

-          response.status = ResponseStatus(status, reason)
+          response.sendError(status, reason)

core/src/main/scala/org/scalatra/servlet/RichResponse.scala

-    res.setStatus(statusLine.code, statusLine.message)
+    res.setStatus(statusLine.code)
@magnolia-k

This comment has been minimized.

Show comment
Hide comment
@magnolia-k

magnolia-k Oct 12, 2017

Contributor

#711

In the future, I think that the place to set the reason-phrase should be limited (only when returning the error immediately without the body part) or not to set it at all, but I think that should not change suddenly So I created a PR that warns on scenes where both reason-phrase and body are set.

Contributor

magnolia-k commented Oct 12, 2017

#711

In the future, I think that the place to set the reason-phrase should be limited (only when returning the error immediately without the body part) or not to set it at all, but I think that should not change suddenly So I created a PR that warns on scenes where both reason-phrase and body are set.

@takezoe takezoe added this to the 2.6.0 milestone Oct 13, 2017

@takezoe takezoe added the core label Oct 13, 2017

@takezoe

This comment has been minimized.

Show comment
Hide comment
@takezoe

takezoe Nov 7, 2017

Member

Fixed in #732.

Member

takezoe commented Nov 7, 2017

Fixed in #732.

@takezoe takezoe closed this Nov 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment