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.

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

Need to sort out the behaviour for status_message with unknown status codes #114

Open
robrwo opened this issue Jun 6, 2018 · 5 comments

Comments

@robrwo
Copy link
Contributor

robrwo commented Jun 6, 2018

@oalders I'm unclear where we are on this.

The status_message should never be undef for a code between 100 and 599, and the RFC recommends some fallback values depending on the code range.

Only the numeric code value is to be used for understanding the response. The status code message is intended as a human-readable value, and the values given are standard defaults, but the web client is free to change it to be something else.

A problem is that some downstream modules that use this do not handle undefined values for this, which can cause further problems.

We should be able to have default values, so that newer- or app-specific status codes can be used without having to break anything.

If there is a need to check for known status codes, that should be a separate function.

@vanHoesel
Copy link
Member

Hi @robrwo ,

I am not entirely sure which RFC you are referring to when you mention «the RFC recommends some fallback values depending on the code range».

For all I can read from RFC 7230, section 3.1.2 --- Status Line is:

The first line of a response message is the status-line, consisting of the protocol version, a space (SP), the status code, another space, a possibly empty textual phrase describing the status code, and ending with CRLF.

and at the end of that section:

A client SHOULD ignore the reason-phrase content.

I'd like to emphasis "possible empty textual phrase".

Then, in RFC 7232, section 6 --- Response Status Code:

HTTP clients are not required to understand the meaning of all registered status codes, though such understanding is obviously desirable. However, a client MUST understand the class of any status code, as indicated by the first digit, and treat an unrecognized status code as being equivalent to the x00 status code of that class, with the exception that a recipient MUST NOT cache a response with an unrecognized status code.

For example, if an unrecognized status code of 471 is received by a client, the client can assume that there was something wrong with its request and treat the response as if it had received a 400 (Bad Request) status code. The response message will usually contain a representation that explains the status.

In short:

  • clients MUST understand the class
  • the response message will usually contain a representation that explains the status

So far, I have seen no «recommendation for some fallback value», I'm sorry

Oh ... RTFRFC is something I showed in huge capitals during a presentation during YAPC Granada, so yes, I tend to be quite picky on these matter

So far about your first statement...

@vanHoesel
Copy link
Member

vanHoesel commented Jun 6, 2018

@robrwo , something next, you wrote «A problem is that some downstream modules that use this do not handle undefined values for this, which can cause further problems.»

My first response is that it is their problem, the problem off the consuming code, not a problem caused by status_message. Consuming code, first of all (as per RFC) SHOULD ignore the reason-phrase content and it MUST understand the class. Therefore HTTP::Status defines a bunch of helper functions like is_redirect etc.

Secondly, the moment we are going to return anything but undef for non IANA registered codes we may cause a whole chain reaction of downstream modules that will start to fail. Some of those modules may, or may not, rely on that specific behaviour.

I'd rather not break the internet or any life code because of that change.

As of your statement «We should be able to have default values, so that newer- or app-specific status codes can be used without having to break anything.» I'd say, again: Clients SHOULD ignore the reason phrase . And 'newer- or app-specific status codes' will simply work if one would follow the RFC recommendations.

@vanHoesel
Copy link
Member

vanHoesel commented Jun 6, 2018

@robrwo 👍 ... «If there is a need to check for known status codes, that should be a separate function.»

I would agree on that, but than in conjunction with something I worked on earlier in march and been rejected by @karenetheridge, for good reasons.

I was trying to make it such that we would have separate HTTP::Status::RFC modules. Each module nicely describing which status codes were being define, with a little bit of POD, the human readable message (if any) etc.

Then anyone that needed to extend the list had a simple way of doing it. Official ones, would go not the official package, others could be included locally.

I was thinking off something like:

use HTTP::Status 'rfc7232';

or

use HTTP::Status 'no rfc2324';

and default to:

use HTTP::Status ':standard';

and then, yes it would be lovely to see

print is_registerd('451'); # "rfc7725"

that would be an awesome solution

robrwo added a commit to robrwo/HTTP-Message that referenced this issue Oct 17, 2018
@vanHoesel
Copy link
Member

vanHoesel commented Oct 17, 2018

I'd suggest to make a new function:

status_class which will return a nXX string.

Once we have that, we could add the nXX keys to the long list of combinations. Than you could take a very explicit approach to your problem inside the calling code:

my $message = status_message($code);
unless ($message) {
    my $class_code = status_class($code);
    $message = status_message($class_code);
    croak "Status code not understood [$code], assuming class [$class_code] ($message)";
}

or

my $message = status_message($code) || status_message( status_class($code) );

I would feel way more comfortable reading these explicit things the a nifty default like status_message_with_fallback.

NB. this could break the is_ . . . functions, they need to be safeguarded for special situations that there are now non-numerical codes if one would want to know what class the code is in and would do is_redirect($class_code)... but that of course only applies to $class_code, had one used the original $code then everything would be perfectly fine.

Oh...

and if that is too much disrupting the calling code, one could always create there and then a local function status_message_with_fallback like:

sub status_message_with_fallback {
    my $code = shift;
    
    my $message = status_message($code) and return $message;
    
    my $class_code = status_class($code);
    $message = status_message($class_code);
    croak "Status code not understood [$code], assuming class [$class_code] ($message)";
    
    return $message;
}

and the later:

my $untrusted_message = status_message_with_fallback($code);

Cheers

@vanHoesel
Copy link
Member

okay, maybe I did approach it the wrong way, but I still will disagree with a new method that potentially will be the go-to thing for the future, since it's so easy to use.

May I suggest a different approach, and rename your function to status_message_fallback so that it only provides a fallback message for those codes not in the list. This way, it is not possible to use it as a default goto method and in calling code you can still do something more easy like:

my $message = status_message($code);
unless ($message) {
    $message = status_message_fallback($code);
    croak "Status code not understood [$code], assuming ($message)";
}

or

my $message = status_message($code) || status_message_fallback($code);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants