data_handlers are not invoked when the HTTP Method is not all uppcrcase

[simbabque]

Usually in a HTTP request the method should be all caps, like GET or POST. That's documented in the RFC. But Catalyst is nice to us and allows get or post for regular request. If this is a bug or a feature is not the issue of this ticket, but it is important to acknowledge it as behaviour that has been there for a while.

If there is a data_handlers handler present and the Conent-Type matches, Catalyst uses that handler to lazily build body_data in the request object. But it only does that if the HTTP Method is uppercase. If not, that thing is never called.

I came across this while writing a unit test. I didn't pay close attention and figured HTTP::Request would figure out on its own what I want. I was wrong.

Following is a test-script that will show how it is possible to use both GET and get as a HTTP method and receive the same response. It also has two calls that resemble a REST call with Content-Type and Accept headers of application/json. Those are sent with PUT and put. The latter fails.

use strict;
use warnings;
use Test::More;
use Plack::Test;
use HTTP::Request;

package MyApp::Controller::Root {
    use base 'Catalyst::Controller';
    use Data::Dumper;
    $Data::Dumper::Indent = 0;

    sub test_put : Local : Args(0) : Method('PUT') : Consumes('application/json') {
        my ( $self, $c ) = @_;

        $c->res->body( Dumper( $c->req->body_data ) );
    }

    sub test_get : Local {
        my ( $self, $c ) = @_;

        $c->res->body('Hello World!');
    }
};

package MyApp {
    use Catalyst;
    use JSON 'decode_json';
    use Test::Simple;

    __PACKAGE__->config(
        data_handlers => {

            # This data_handler is just here to explicitly illustrate that there is one.
            # It is the example one from https://metacpan.org/pod/Catalyst#DATA-HANDLERS
            # that is there by default.
            'application/json' => sub {
                ok 1, 'in data_handler';    # we have a plan!
                local $/;
                decode_json $_->getline;
            },
        },
    );

    MyApp->setup;
};

ok my $psgi = MyApp->psgi_app, 'build psgi app';

test_psgi $psgi, sub {
    my $cb = shift;

    # the first two subtests are to illustrate that for a regular
    # request that does not go through the data_handlers the case
    # (upper/lower) of the HTTP Method is irrelevant
    subtest 'GET /root/test_get' => sub {
        my $res = $cb->( HTTP::Request->new( GET => '/root/test_get' ) );
        is $res->code,            200,            'OK';
        is $res->decoded_content, 'Hello World!', 'says "Hello World!"';
    };

    subtest 'get /root/test_get' => sub {
        my $res = $cb->( HTTP::Request->new( get => '/root/test_get' ) );
        is $res->code,            200,            'OK';
        is $res->decoded_content, 'Hello World!', 'says "Hello World!"';
    };

    # All is well if our HTTP Method is all-uppercase. The data_handler
    # gets it and unjsons it.
    subtest 'PUT /root/test_put' => sub {
        plan tests => 3;

        my $res = $cb->(
            HTTP::Request->new(
                PUT => '/root/test_put',
                HTTP::Headers->new(
                    'Accept'       => 'application/json',
                    'Content-Type' => 'application/json'
                ),
                '{ "foo" : "bar" }',
            )
        );
        is $res->code, 200, 'OK';
        is $res->decoded_content, q($VAR1 = {'foo' => 'bar'};), '$VAR1 contains a hashref';
    };

    # But if the HTTP Method is lowercase it gets ignored.
    subtest 'put json all lowercase' => sub {
        plan tests => 3;

        my $res = $cb->(
            HTTP::Request->new(
                put => '/root/test_put',
                HTTP::Headers->new(
                    'Accept'       => 'application/json',
                    'Content-Type' => 'application/json'
                ),
                '{"foo" : "bar" }',
            )
        );
        is $res->code, 200, 'OK';
        is $res->decoded_content, q($VAR1 = {'foo' => 'bar'};), '$VAR1 contains a hashref';
    };

};

done_testing;

This can simply be run with prove or perl. Here's my output.

$ perl catalyst_test_lowercase_http_handler.pl 
ok 1 - build psgi app
    # Subtest: GET /root/test_get
    ok 1 - OK
    ok 2 - says "Hello World!"
    1..2
ok 2 - GET /root/test_get
    # Subtest: get /root/test_get
    ok 1 - OK
    ok 2 - says "Hello World!"
    1..2
ok 3 - get /root/test_get
    # Subtest: PUT /root/test_put
    1..3
    ok 1 - in data_handler
    ok 2 - OK
    ok 3 - $VAR1 contains a hashref
ok 4 - PUT /root/test_put
    # Subtest: put json all lowercase
    1..3
    ok 1 - OK
    not ok 2 - $VAR1 contains a hashref
    #   Failed test '$VAR1 contains a hashref'
    #   at catalyst_test_lowercase_http_handler.pl line 101.
    #          got: '$VAR1 = undef;'
    #     expected: '$VAR1 = {'foo' => 'bar'};'
    # Looks like you planned 3 tests but ran 2.
    # Looks like you failed 1 test of 2 run.
not ok 5 - put json all lowercase
#   Failed test 'put json all lowercase'
#   at catalyst_test_lowercase_http_handler.pl line 102.
1..5
# Looks like you failed 1 test of 5.

At the beginning of this ticket we have noted that Catalyst allows lowercase HTTP methods in general. According to the RFC, the method should be upper case. As there is a response to the put call where the body is $VAR1 = undef; shows that it lets the wrong call through, but the data_handlers behaviour breaks.

So there is clearly something wrong here. I am not sure which part exactly is the wrong behaviour. We could argue either of these two cases:

• in general, only uppercase methods should be supported as only those are in the RFC
• since lowercase is supported in general, for data_handlers it should also be supported because of backwards compatibility

However, the way it is now it is inconsistent, so something should be done about it.

[simbabque]

And I found the reason. It only allows POST and PUT verbatim. See https://metacpan.org/source/JJNAPIORK/Catalyst-Runtime-5.90096/lib/Catalyst/Request.pm#L126:

sub _build_body_data {
    my ($self) = @_;

    # Not sure if these returns should not be exceptions...
    my $content_type = $self->content_type || return;
    return unless ($self->method eq 'POST' || $self->method eq 'PUT');