Skip to content
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.

Sign up for GitHub

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

Added status code 308 #582

Closed
wants to merge 1 commit into from
Closed

Added status code 308 #582

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions jaxrs-api/src/main/java/javax/ws/rs/core/Response.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,21 @@ public static ResponseBuilder temporaryRedirect(URI location) {
return status(Status.TEMPORARY_REDIRECT).location(location);
}

/**
* Create a new ResponseBuilder for a permanent redirection.
*
* @param location the redirection URI. If a relative URI is
* supplied it will be converted into an absolute URI by resolving it
* relative to the base URI of the application (see
* {@link UriInfo#getBaseUri}).
* @return a new response builder.
* @throws java.lang.IllegalArgumentException
* if location is {@code null}.
*/
public static ResponseBuilder permanentRedirect(URI location) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that a shortcut method permanentRedirect is required. A lot of other, IMO more important, status codes don't have shortcut methods either. I would rather not bloat this class with another method -- if we decide to add the enum.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I share this opinion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, these shortcut methods are especially important for status codes that need additional headers to be set. Playing devil's advocate here, actually 308 would be a good example for a status method for which a shortcut method would make sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkarg @sdaschner : I suggest for now, lets keep it as is ...ie to have the shortcut method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only consider adding this method if a majority agrees on its usefulness. And currently most comments questioned it. So let's wait for other comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spericas Does that mean "plus one" or "zero" from your side? Just for correctly counting votes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdaschner "Minus one" due to the shortcut method, or "zero" because it was only a comment? Just for counting votes correctly.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zero then. When I think about it more, I do see the advantage that you want to add an URI, similar to created, what @chkal mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkarg It is a +1 for me. This error code is widely supported in all major browsers, that is good enough for me. As I said, I also don't have a problem keeping the static method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sum so far (as not everybody used the Github review tool): chkal 0, mkarg -1, spericas +1, sdaschner 0 => result 0.

return status(Status.PERMANENT_REDIRECT).location(location);
}

/**
* Create a new ResponseBuilder for a not acceptable response.
*
Expand Down Expand Up @@ -1296,6 +1311,10 @@ public enum Status implements StatusType {
* 307 Temporary Redirect, see {@link <a href="http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3.8">HTTP/1.1 documentation</a>}.
*/
TEMPORARY_REDIRECT(307, "Temporary Redirect"),
/**
* 308 Permanent Redirect, see {@link <a href="https://tools.ietf.org/html/rfc7538">HTTP/1.1 documentation</a>}.
*/
PERMANENT_REDIRECT(308, "Permanent Redirect"),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, but still: what's the point in citing RFC 2616? It has been obsoleted years ago.

/**
* 400 Bad Request, see {@link <a href="http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.1">HTTP/1.1 documentation</a>}.
*/
Expand Down