improve parser errors to be more consistent with connection errors in…

… Mojo::Message::Request and Mojo::Message::Response
mojolicious · Jan 23, 2015 · bc79ba7 · bc79ba7
1 parent c814925
commit bc79ba7
Show file tree

Hide file tree

Showing 7 changed files with 14 additions and 32 deletions.
diff --git a/Changes b/Changes
@@ -1,5 +1,7 @@
 
 5.74  2015-01-25
+  - Improved parser errors to be more consistent with connection errors in
+    Mojo::Message::Request and Mojo::Message::Response.
 
 5.73  2015-01-24
   - Deprecated Mojolicious::Routes::Route::bridge in favor of

diff --git a/lib/Mojo/Message.pm b/lib/Mojo/Message.pm
@@ -156,7 +156,7 @@ sub parse {
     # Check start-line size
     my $len = index $self->{buffer}, "\x0a";
     $len = length $self->{buffer} if $len < 0;
-    return $self->_limit('Maximum start-line size exceeded', 431)
+    return $self->_limit('Maximum start-line size exceeded')
       if $len > $self->max_line_size;
 
     $self->{state} = 'content' if $self->extract_start_line(\$self->{buffer});
@@ -169,15 +169,15 @@ sub parse {
 
   # Check message size
   my $max = $self->max_message_size;
-  return $self->_limit('Maximum message size exceeded', 413)
+  return $self->_limit('Maximum message size exceeded')
     if $max && $max < $self->{raw_size};
 
   # Check header size
-  return $self->_limit('Maximum header size exceeded', 431)
+  return $self->_limit('Maximum header size exceeded')
     if $self->headers->is_limit_exceeded;
 
   # Check buffer size
-  return $self->_limit('Maximum buffer size exceeded', 400)
+  return $self->_limit('Maximum buffer size exceeded')
     if $self->content->is_limit_exceeded;
 
   return $self->emit('progress')->content->is_finished ? $self->finish : $self;
@@ -253,11 +253,7 @@ sub _cache {
   return $all ? $objects : $objects->[-1];
 }
 
-sub _limit {
-  my ($self, $msg, $code) = @_;
-  $self->{limit} = 1;
-  return $self->error({message => $msg, advice => $code});
-}
+sub _limit { ++$_[0]{limit} and return $_[0]->error({message => $_[1]}) }
 
 sub _parse_formdata {
   my ($self, $upload) = @_;
@@ -499,17 +495,14 @@ make sure it is not excessively large, there's a 10MB limit by default.
 =head2 error
 
   my $err = $msg->error;
-  $msg    = $msg->error({message => 'Parser error', advice => 500});
+  $msg    = $msg->error({message => 'Parser error'});
 
 Get or set message error, an C<undef> return value indicates that there is no
 error.
 
-  # Connection error
+  # Connection or parser error
   $msg->error({message => 'Connection refused'});
 
-  # Parser error (recommending a 413 response)
-  $msg->error({message => 'Maximum message size exceeded', advice => 413});
-
   # 4xx/5xx response
   $msg->error({message => 'Internal Server Error', code => 500});
 

diff --git a/lib/Mojo/Message/Request.pm b/lib/Mojo/Message/Request.pm
@@ -60,7 +60,7 @@ sub extract_start_line {
   return undef unless $$bufref =~ s/^\s*(.*?)\x0d?\x0a//;
 
   # We have a (hopefully) full request-line
-  return !$self->error({message => 'Bad request start-line', advice => 400})
+  return !$self->error({message => 'Bad request start-line'})
     unless $1 =~ $START_LINE_RE;
   my $url = $self->method($1)->version($3)->url;
   return !!($1 eq 'CONNECT' ? $url->authority($2) : $url->parse($2));

diff --git a/t/mojo/request.t b/t/mojo/request.t
@@ -78,7 +78,6 @@ $req = Mojo::Message::Request->new;
 $req->parse("12345\x0d\x0a");
 ok $req->is_finished, 'request is finished';
 is $req->error->{message}, 'Bad request start-line', 'right error';
-is $req->error->{advice}, 400, 'right advice';
 ok !$req->is_limit_exceeded, 'limit is not exceeded';
 
 # Parse broken HTTP 1.1 message with header exceeding line limit
@@ -89,7 +88,6 @@ ok !$req->is_limit_exceeded, 'limit is not exceeded';
 $req->parse("Foo: @{['a' x 10240]}");
 ok $req->is_finished, 'request is finished';
 is $req->error->{message}, 'Maximum header size exceeded', 'right error';
-is $req->error->{advice}, 431, 'right advice';
 ok $req->is_limit_exceeded, 'limit is exceeded';
 is $req->method,            'GET', 'right method';
 is $req->version,           '1.1', 'right version';
@@ -352,12 +350,10 @@ is $req->headers->content_length, undef,        'no "Content-Length" value';
   $req->parse('GET /foo/bar/baz.html HTTP/1');
   ok $req->is_finished, 'request is finished';
   is $req->error->{message}, 'Maximum start-line size exceeded', 'right error';
-  is $req->error->{advice}, 431, 'right advice';
   ok $req->is_limit_exceeded, 'limit is exceeded';
   ok $limit, 'limit is exceeded';
   $req->error({message => 'Nothing important.'});
   is $req->error->{message}, 'Nothing important.', 'right error';
-  is $req->error->{advice}, undef, 'no advice';
   ok $req->is_limit_exceeded, 'limit is still exceeded';
 }
 
@@ -370,7 +366,6 @@ is $req->headers->content_length, undef,        'no "Content-Length" value';
   $req->parse("Content-Type: text/plain\x0d\x0a");
   ok $req->is_finished, 'request is finished';
   is $req->error->{message}, 'Maximum header size exceeded', 'right error';
-  is $req->error->{advice}, 431, 'right advice';
   ok $req->is_limit_exceeded, 'limit is exceeded';
 }
 
@@ -384,7 +379,6 @@ is $req->headers->content_length, undef,        'no "Content-Length" value';
   $req->parse('GET /foo/bar/baz.html HTTP/1');
   ok $req->is_finished, 'request is finished';
   is $req->error->{message}, 'Maximum message size exceeded', 'right error';
-  is $req->error->{advice}, 413, 'right advice';
   ok $req->is_limit_exceeded, 'limit is exceeded';
   ok $limit, 'limit is exceeded';
 }
@@ -397,7 +391,6 @@ is $req->headers->content_length, undef,        'no "Content-Length" value';
   $req->parse("Content-Type: text/plain\x0d\x0a");
   ok $req->is_finished, 'request is finished';
   is $req->error->{message}, 'Maximum message size exceeded', 'right error';
-  is $req->error->{advice}, 413, 'right advice';
   ok $req->is_limit_exceeded, 'limit is exceeded';
 }
 
@@ -411,7 +404,6 @@ is $req->headers->content_length, undef,        'no "Content-Length" value';
   $req->parse('Hello World!');
   ok $req->is_finished, 'request is finished';
   is $req->error->{message}, 'Maximum message size exceeded', 'right error';
-  is $req->error->{advice}, 413, 'right advice';
   ok $req->is_limit_exceeded, 'limit is exceeded';
 }
 
@@ -426,7 +418,6 @@ is $req->headers->content_length, undef,        'no "Content-Length" value';
   $req->parse("D: d\x0d\x0a\x0d\x0a");
   ok $req->is_finished, 'request is finished';
   is $req->error->{message}, 'Maximum header size exceeded', 'right error';
-  is $req->error->{advice}, 431, 'right advice';
   ok $req->is_limit_exceeded, 'limit is exceeded';
   is $req->method,            'GET', 'right method';
   is $req->version,           '1.1', 'right version';

diff --git a/t/mojo/response.t b/t/mojo/response.t
@@ -373,7 +373,6 @@ is $res->headers->content_length, undef, 'right "Content-Length" value';
   ok $res->is_finished, 'response is finished';
   ok $res->content->is_finished, 'content is finished';
   is $res->error->{message}, 'Maximum buffer size exceeded', 'right error';
-  is $res->error->{advice}, 400, 'right advice';
   ok $res->is_limit_exceeded, 'limit is not exceeded';
   is $res->code,              200, 'right status';
   is $res->message,           'OK', 'right message';
@@ -396,7 +395,6 @@ is $res->headers->content_length, undef, 'right "Content-Length" value';
   ok $res->is_finished, 'response is finished';
   ok $res->content->is_finished, 'content is finished';
   is $res->error->{message}, 'Maximum buffer size exceeded', 'right error';
-  is $res->error->{advice}, 400, 'right advice';
   is $res->code,    200,   'right status';
   is $res->message, 'OK',  'right message';
   is $res->version, '1.1', 'right version';
@@ -420,7 +418,6 @@ is $res->headers->content_length, undef, 'right "Content-Length" value';
   ok $res->is_finished, 'response is finished';
   ok $res->content->is_finished, 'content is finished';
   is $res->error->{message}, 'Maximum buffer size exceeded', 'right error';
-  is $res->error->{advice}, 400, 'right advice';
   is $res->code,    200,   'right status';
   is $res->message, 'OK',  'right message';
   is $res->version, '1.1', 'right version';

diff --git a/t/mojo/user_agent.t b/t/mojo/user_agent.t
@@ -335,8 +335,7 @@ $ua->once(
 $tx = $ua->get('/echo' => 'Hello World!');
 ok !$tx->success, 'not successful';
 is $tx->error->{message}, 'Maximum message size exceeded', 'right error';
-is $tx->error->{advice},  413,                             'right advice';
-is $tx->error->{code},    undef,                           'no status';
+is $tx->error->{code}, undef, 'no status';
 ok $tx->res->is_limit_exceeded, 'limit is exceeded';
 
 # 404 response

diff --git a/t/mojolicious/lite_app.t b/t/mojolicious/lite_app.t
@@ -789,8 +789,8 @@ $t->app->log->unsubscribe(message => $cb);
   local $ENV{MOJO_MAX_MESSAGE_SIZE} = 1024;
   $t->get_ok('/', '1234' x 1024)->status_is(200)
     ->header_is(Connection => 'close')
-    ->content_is(
-    "413\n/root.html\n/root.html\n/root.html\n/root.html\n/root.html\n");
+    ->content_is("Maximum message size exceeded\n"
+      . "/root.html\n/root.html\n/root.html\n/root.html\n/root.html\n");
 }
 
 # Relaxed placeholder
@@ -1102,7 +1102,7 @@ Test ok!
 
 @@ root.html.epl
 % my $c = shift;
-<% if (my $err = $c->req->error) { =%><%= "$err->{advice}\n" %><% } =%>
+<% if (my $err = $c->req->error) { =%><%= "$err->{message}\n" %><% } =%>
 %== $c->url_for('root_path')
 %== $c->url_for('root_path')
 %== $c->url_for('root_path')