Added status code 308 #582

mithunsasidharan · 2018-02-12T07:42:47Z

Added status code 308

Fixes issue #573

RFC 7538 describes the behavior of the HTTP status code 308. While this RFC is quite new it solves an actual problem regarding POST redirections.

And it completes quite well the 307 status code found in the HTTP 1.1 RFC (previously described in the obsolete RFC 2616).

mkarg · 2018-02-12T23:24:40Z

-1, as this RFC is a draft only and it won't be possible due to WORA to remove support of 308 in case it never reaches public standard status.

mkarg · 2018-02-13T18:17:18Z

@mithunsasidharan Can you please rebase on master? I had to clean it up so we have a clean starting point for contributions. Thanks, and sorry for the trouble.

mithunsasidharan · 2018-02-16T16:02:21Z

@mkarg : for some reason the commit sign off is failing although I've it in my commit. kindly review once. thanks!

mkarg · 2018-02-16T18:34:09Z

@mithunsasidharan Can you please squash then rebase on master? Maybe this fixes the missing sign-off?

mkarg · 2018-02-19T19:02:35Z

@mithunsasidharan Please rebase against HEAD of branch 3-SNAPSHOT and fix sign-off.

sdaschner · 2018-02-19T19:04:58Z

jaxrs-api/src/main/java/javax/ws/rs/core/Response.java

+     * @throws java.lang.IllegalArgumentException
+     *          if location is {@code null}.
+     */
+    public static ResponseBuilder permanentRedirect(URI location) {


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.

I share this opinion.

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.

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

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.

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

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

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.

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

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

sdaschner · 2018-02-19T19:05:50Z

jaxrs-api/src/main/java/javax/ws/rs/core/Response.java

@@ -1296,6 +1311,10 @@ public default Status toEnum() {
         * 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 Temporary Redirect, see {@link <a href="https://tools.ietf.org/html/rfc7538">HTTP/1.1 documentation</a>}.


The link text should say something like The Hypertext Transfer Protocol Status Code 308 (Permanent Redirect), see RFC 7538, not HTTP/1.1 documentation.

Actually this is not @mithunsasidharan's fault: That style was chosen by Oracle years before, as you can see in the already existing status codes. But I share your opinion, we should not continue that bad style.

I was only referring to the link text. I suggest that it reads 308 Temporary Redirect, see {@link <a href="https://tools.ietf.org/html/rfc7538">The Hypertext Transfer Protocol Status Code 308 (Permanent Redirect)</a>}. or similar

@mkarg @sdaschner : Its the same style we chose so going by that.. I can refactor all in another PR once we decide on it.

It's not about the style, I'm just commenting on the link text; that particular RFC 7538 is called "The Hypertext Transfer Protocol Status Code 308 (Permanent Redirect)".

@sdaschner : Can you check now.. I've fixed that !

Signed-off-by: Mithun Sasidharan <[email protected]>

mithunsasidharan · 2018-02-20T12:36:57Z

@mkarg : rebased and signed off. Please review the PR now.

derekm · 2018-02-20T14:23:56Z

Can jaxrs start to keep a registry of all RFCs that are intended to be satisfied?

This RFC is a "proposed standard," several such RFCs at the same status are already implemented by jaxrs and other JSRs. Since work is commencing here, I guess that's been admitted already.

Still, a registry of RFCs seems lacking, at least from my quick skim of the most recent spec.

jansupol · 2018-02-20T16:40:46Z

@derekm Please file a separate request/Issue for the registry

chkal · 2018-02-21T06:28:13Z

@spericas What is your opinion about adding 308 although the RFC is "proposed standard"?

mkarg

@mithunsasidharan Your PR still asks for merge into master but you have to request merge into 3-SNAPSHOT. According to the PMC we must not accept PRs against master, and as 308 is not yet part of the IETF STD track but only a proposed standard it cannot be accepted in an earlier branch. Please change this now so we can finally vote and (hopefully) merge.

mkarg · 2018-02-21T18:17:04Z

I think we would do a failure adding this 308 at this location. Please have a look at the description of this class:

 /**
     * Commonly used status codes defined by HTTP, see
     * {@link <a href="http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10">HTTP/1.1 documentation</a>}
     * for the complete list. Additional status codes can be added by applications
     * by creating an implementation of {@link StatusType}.
     */
    public enum Status implements StatusType

It clearly says that this class is for HTTP/1.1 only, but extensions have to go into different implementations of the StatusType interface.

Hence -1 for extending the wrong class.

A clean solution for now would be adopting a new class like ProposedStatus implements StatusType which holds all kinds of proposed standard status codes. Remember, we only provide an API here, not a complete framework. Maybe it would be great to simply start a completely separate project for "Non-Standard-JAX-RS-Extensions" as an "official incubator" for such cases? Once the RFC becomes a STD track standard, we can then merge from there to here?

mithunsasidharan · 2018-02-22T04:05:07Z

@mkarg : I will update the PR to point to 3-SNAPSHOT. Besides, I agree with your thoughts above and will make the changes accordingly and update the PR.

chkal · 2018-02-22T13:17:26Z

I really dislike having something like ProposedStatus. We will never be able to remove something from this enum. So if the RFC gets finalized, we will have to add it to Status enum and also keep it in ProposedStatus. That feels weird.

mkarg · 2018-02-22T17:44:59Z

So far we did not reach a concensus on this PR. It would be good if more committers would actively use the Github review tool on this particular issue to cast a vote, so a clear majority can be found. Until then, I simply let this PR open.

mkarg

My requested changes are done (3-SNAPSHOT as target of the PR), but as I am not conviced about the correctness of adding pre-standard 308 to particularly the Response class, I hereby abstain from voting.

mithunsasidharan · 2018-02-23T12:54:37Z

@mkarg : Do we have a consensus on what to be done with this yet ?

mkarg · 2018-02-23T17:23:04Z

@mithunsasidharan Not finally. At the moment there is no majority for adopting the change due to the already discussed concerns. One idea was that I could open a separate project for "near-to-standard" things, or a "external contributions" project. One could then simply add this to his application to adopt such RFCs. I do not say that this is a final decision, as the committers have not finally voted, but at least this is the current status of this PR. Please stay tuned, we still are thinking... :-)

mkarg · 2018-03-12T17:51:43Z

@spericas Why did you close this PR? The discussion was still running, and the PR was assigned to me, not to you.

spericas · 2018-03-12T18:04:47Z

@mkarg I didn't, at least not intentionally :) But I did remove 3-SNAPSHOT. A side effect of that perhaps?

mkarg · 2018-03-12T18:07:08Z

@spericas Understood, but how can I reopen it?

mkarg · 2018-03-12T18:09:04Z

@spericas Just noticed: One has to [edit] then change the target branch, then click [Reopen] in the comments box.

mkarg · 2018-03-17T12:46:08Z

@spericas Can you please create a milestone named 3 so I can keep track of PRs like this one? It seems committers can created branches, tags and labels, but not milestones in Github. Thanks.

spericas · 2018-03-20T17:11:46Z

On Mar 17, 2018, at 8:46 AM, Markus KARG ***@***.***> wrote: @spericas <https://github.com/spericas> Can you please create a milestone named 3 so I can keep track of PRs like this one? It seems committers can created branches, tags and labels, but not milestones in Github. Thanks.

Done. — Santiago

mkarg · 2018-03-23T17:41:43Z

@spericas Thanks! :-)

ronsigal

I left a comment about the 'Proposed Standard" issue on Add status code 308 #573. I don't think it's a concern.
I agree that adding the permanentRedirect() method is useful.

reschke · 2020-05-28T18:15:41Z

FWIW, RFC 7538 has exactly the same standards track status as the base HTTP specs (RFC 723?): "proposed". There really is no problem with that.

Also, status codes are an extension point of HTTP. There's really no point in hard-wiring anything into an API, in particular not as Enums.

reschke · 2020-05-28T18:18:19Z

jaxrs-api/src/main/java/javax/ws/rs/core/Response.java

+        /**
+         * 308 Permanent Redirect, see {@link <a href="https://tools.ietf.org/html/rfc7538">HTTP/1.1 documentation</a>}.
+         */
+        PERMANENT_REDIRECT(308, "Permanent Redirect"),


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

chkal · 2020-05-31T10:38:56Z

The selected milestone for this pull request is 4.0. I wonder if we can get this into the 3.1-SNAPSHOT branch. Thoughts?

mkarg · 2020-05-31T11:40:39Z

The selected milestone for this pull request is 4.0. I wonder if we can get this into the 3.1-SNAPSHOT branch. Thoughts?

Fine for me. Go ahead with that. :-)

chkal · 2020-05-31T13:14:07Z

@mithunsasidharan Are you still interested in working on this? If so, could you rebase against `3.1-SNAPSHOT" and update the pull request accordingly?

spericas · 2021-07-07T14:08:35Z

Last comment is over 1 year old. Closing this one.

mkarg added the enhancement New feature or request label Feb 12, 2018

mkarg force-pushed the master branch from 8caf1e8 to a2f52e8 Compare February 13, 2018 18:14

mkarg self-assigned this Feb 19, 2018

mkarg self-requested a review February 19, 2018 19:01

sdaschner reviewed Feb 19, 2018

View reviewed changes

Added status code 308

af9975e

Signed-off-by: Mithun Sasidharan <[email protected]>

mkarg requested changes Feb 21, 2018

View reviewed changes

mithunsasidharan changed the base branch from master to 3-SNAPSHOT February 22, 2018 04:06

mkarg reviewed Feb 22, 2018

View reviewed changes

mkarg approved these changes Feb 22, 2018

View reviewed changes

spericas approved these changes Feb 26, 2018

View reviewed changes

spericas closed this Mar 12, 2018

mkarg changed the base branch from 3-SNAPSHOT to master March 12, 2018 18:07

spericas reopened this Mar 12, 2018

mkarg added this to the 3.0 milestone Mar 23, 2018

Bo98 mentioned this pull request Dec 10, 2018

Supported RFCs registry #591

Open

ronsigal self-requested a review May 27, 2020 16:59

ronsigal approved these changes May 27, 2020

View reviewed changes

reschke reviewed May 28, 2020

View reviewed changes

spericas closed this Jul 7, 2021

reschke mentioned this pull request Jul 7, 2021

Missing status codes in Response class #993

Closed

Added status code 308 #582

Added status code 308 #582

Conversation

mithunsasidharan commented Feb 12, 2018

mkarg commented Feb 12, 2018

mkarg commented Feb 13, 2018

mithunsasidharan commented Feb 16, 2018

mkarg commented Feb 16, 2018

mkarg commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdaschner Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mithunsasidharan commented Feb 20, 2018

derekm commented Feb 20, 2018

jansupol commented Feb 20, 2018

chkal commented Feb 21, 2018

mkarg left a comment • edited Loading

Choose a reason for hiding this comment

mkarg commented Feb 21, 2018

mithunsasidharan commented Feb 22, 2018

chkal commented Feb 22, 2018

mkarg commented Feb 22, 2018

mkarg left a comment

Choose a reason for hiding this comment

mithunsasidharan commented Feb 23, 2018

mkarg commented Feb 23, 2018

mkarg commented Mar 12, 2018

spericas commented Mar 12, 2018

mkarg commented Mar 12, 2018

mkarg commented Mar 12, 2018

mkarg commented Mar 17, 2018

spericas commented Mar 20, 2018 via email

mkarg commented Mar 23, 2018

ronsigal left a comment

Choose a reason for hiding this comment

reschke commented May 28, 2020

Choose a reason for hiding this comment

chkal commented May 31, 2020

mkarg commented May 31, 2020

chkal commented May 31, 2020

spericas commented Jul 7, 2021

sdaschner Feb 19, 2018 •

edited

Loading

mkarg left a comment •

edited

Loading