Bug #23772 for Catalyst-Action-REST: Accept support finds the first defined serializer, not the first one that works.

Mon Dec 04 19:52:02 2006 holoway [...] cpan.org - Ticket created

Subject:

Accept support finds the first defined serializer, not the first one that works.

The Accept header support finds the first serizlier defined in the map; not the first serializer that actually is capable of serializing something. Fix likely involves re-factoring when we do the serialize request.

Thu Oct 18 04:13:21 2007 sjn [...] cpan.org - Correspondence added

From:

sjn [...] cpan.org

On Man. 04. Des. 2006 19:52:02, HOLOWAY wrote: Show quoted text

> The Accept header support finds the first serizlier defined in the map; > not the first serializer that actually is capable of serializing

something. Show quoted text

> > Fix likely involves re-factoring when we do the serialize request.

I've found the same problem. Currently, C::R::REST prioritize the types found in the Accept header as "If it's mentioned first, we'll use that". According to RFC2616 section 14.1, the Accept header handling should be quite a bit more flexible. Here's a suggestion for what to do about it: 1) Add a configuration for secondary content-type priority, giving the developer the option to prioritize when the client declines to do so. 2) Use this priority list to establish a complete prioritization list, and remove all content types the server isn't set up to handle. (a quick grep ought to work nicely.) All this should be possible to do in the Catalyst::Request::REST::accepted_content_types method. :-)

Thu Oct 18 04:13:22 2007 The RT System itself - Status changed from 'new' to 'open'

Thu Oct 18 05:14:15 2007 sjn [...] cpan.org - Correspondence added

On Tor. 18. Okt. 2007 04:13:21, sjn wrote: Show quoted text

> > 1) Add a configuration for secondary content-type priority, giving the > developer the option to prioritize when the client declines to do so. > > 2) Use this priority list to establish a complete prioritization list, > and remove all content types the server isn't set up to handle. (a quick > grep ought to work nicely.)

Oh yay. I see this is basically what it says in the TODO file. I'll look into doing something about it then. :-\

Tue Oct 30 21:23:02 2007 adam [...] stalecoffee.org - Correspondence added

CC:	HOLOWAY [...] cpan.org
Subject:	Re: [rt.cpan.org #23772] Accept support finds the first defined serializer, not the first one that works.
Date:	Tue, 30 Oct 2007 18:22:46 -0700
To:	bug-Catalyst-Action-REST [...] rt.cpan.org
From:	"Adam Jacob" <adam [...] stalecoffee.org>

Yeah, a patch for fixing this would be gladly accepted. On 10/18/07, Salve J. Nilsen via RT <bug-Catalyst-Action-REST@rt.cpan.org> wrote: Show quoted text

> > > <URL: http://rt.cpan.org/Ticket/Display.html?id=23772 > > > On Tor. 18. Okt. 2007 04:13:21, sjn wrote:

> > > > 1) Add a configuration for secondary content-type priority, giving the > > developer the option to prioritize when the client declines to do so. > > > > 2) Use this priority list to establish a complete prioritization list, > > and remove all content types the server isn't set up to handle. (a quick > > grep ought to work nicely.)

> > Oh yay. I see this is basically what it says in the TODO file. I'll look > into doing something about it then. :-\ > > > >

Sat Nov 03 21:02:41 2007 claco [...] cpan.org - Correspondence added

From:

CLACO [...] cpan.org

On Mon Dec 04 19:52:02 2006, HOLOWAY wrote: Show quoted text

> The Accept header support finds the first serizlier defined in the map; > not the first serializer that actually is capable of serializing

something. Show quoted text

> > Fix likely involves re-factoring when we do the serialize request.

On a somewhat related note, I think. Firefox accept lists text/xml first, which matches because it's defined and installed. Of course, the default key is set to something else, but is nver hit. I think in the end, what's needed is a way to say use these mapped types, and these only, and in this order. Sounds like he TODO is on that path. This seems esp. important when you start using REST on most actions that also double as browser/html methods rather than just using REST to expose a remote API. For now, an option is to simply remap those unwanted content types to the default too.

Thu Nov 22 12:17:46 2007 sjn [...] cpan.org - Correspondence added

On Lør. 03. Nov. 2007 21:02:41, CLACO wrote: Show quoted text

> On Mon Dec 04 19:52:02 2006, HOLOWAY wrote:

> > The Accept header support finds the first serizlier defined in > > the map; not the first serializer that actually is capable of > > serializing something. > > > > Fix likely involves re-factoring when we do the serialize request.

> > On a somewhat related note, I think. Firefox accept lists text/xml > first, which matches because it's defined and installed. Of course, > the default key is set to something else, but is nver hit. > > I think in the end, what's needed is a way to say use these mapped > types, and these only, and in this order. Sounds like he TODO is on > that path. > > This seems esp. important when you start using REST on most actions > that also double as browser/html methods rather than just using REST > to expose a remote API. For now, an option is to simply remap those > unwanted content types to the default too.

I've added a naĩve patch that allows the server to set up it's own content-type priority list which can be used to select one content type when the client accepts several types, but weighted identically. (See Catalyst-Action-REST-0.50-Accept.patch for details. Not that this priority list should be user-configurable, something the patch doesn't do.) Hope this helps in the right direction! :) - Salve

--- lib/Catalyst/Request/REST.pm~ 2007-03-09 21:44:54.000000000 +0100 +++ lib/Catalyst/Request/REST.pm 2007-10-22 15:59:24.000000000 +0200 @@ -80,15 +80,22 @@ return $self->{content_types} if $self->{content_types}; - my %types; + my %requested_types; + my %provided_types = ( # FIXME: Make configurable/reflect what's installed + 'text/html' => 1, + 'application/xhtml+xml' => 0.9, + 'text/xml' => 0.8, + 'application/xml' => 0.79, + 'image/png' => 0.1, + ); # First, we use the content type in the HTTP Request. It wins all. - $types{ $self->content_type } = 3 + $requested_types{ $self->content_type } = 3 if $self->content_type; if ($self->method eq "GET" && $self->param('content-type')) { - $types{ $self->param('content-type') } = 2; + $requested_types{ $self->param('content-type') } = 2; } # Third, we parse the Accept header, and see if the client @@ -96,25 +103,25 @@ # # This is taken from chansen's Apache2::UploadProgress. if ( $self->header('Accept') ) { - $self->accept_only(1) unless keys %types; + $self->accept_only(1) unless keys %requested_types; my $accept_header = $self->header('Accept'); - my $counter = 0; foreach my $pair ( split_header_words($accept_header) ) { my ( $type, $qvalue ) = @{$pair}[ 0, 3 ]; - next if $types{$type}; + next if $requested_types{$type}; - unless ( defined $qvalue ) { - $qvalue = 1 - ( ++$counter / 1000 ); - } + $qvalue = 1 unless defined $qvalue; - $types{$type} = sprintf( '%.3f', $qvalue ); + $requested_types{$type} = sprintf( '%.1f', $qvalue ); } } return $self->{content_types} = - [ sort { $types{$b} <=> $types{$a} } keys %types ]; + [ sort { + $requested_types{$b} <=> $requested_types{$a} || + $provided_types{$b} <=> $provided_types{$a} + } keys %requested_types ]; } =item preferred_content_type --- t/catalyst-request-rest.t 2007-03-09 21:28:30.000000000 +0100 +++ t/catalyst-request-rest.t 2007-10-22 16:06:50.000000000 +0200 @@ -66,15 +66,18 @@ ); is_deeply( $request->accepted_content_types, - [ qw( text/xml application/xml application/xhtml+xml + [ qw( + application/xhtml+xml + text/xml + application/xml image/png text/html text/plain */* ) ], 'accept header is parsed properly' ); - is( $request->preferred_content_type, 'text/xml', - 'preferred content type is text/xml' ); + is( $request->preferred_content_type, 'application/xhtml+xml', + 'preferred content type is application/xhtml+xml' ); ok( $request->accept_only, 'accept_only is true' ); ok( $request->accepts('text/html'), 'accepts text/html' ); ok( $request->accepts('image/png'), 'accepts image/png' ); @@ -95,8 +98,11 @@ ); is_deeply( $request->accepted_content_types, - [ qw( text/x-json - text/xml application/xml application/xhtml+xml + [ qw( + text/x-json + application/xhtml+xml + text/xml + application/xml image/png text/html text/plain

Thu Aug 13 03:15:33 2015 perl [...] evancarroll.com - Correspondence added

Thanks for the patch SJN. It'll probably sit here for another decade before you get feedback or acceptance. It's perl! On Thu Nov 22 11:17:46 2007, sjn wrote: Show quoted text

> On Lør. 03. Nov. 2007 21:02:41, CLACO wrote:

> > On Mon Dec 04 19:52:02 2006, HOLOWAY wrote:

> > > The Accept header support finds the first serizlier defined in > > > the map; not the first serializer that actually is capable of > > > serializing something. > > > > > > Fix likely involves re-factoring when we do the serialize request.

> > > > On a somewhat related note, I think. Firefox accept lists text/xml > > first, which matches because it's defined and installed. Of course, > > the default key is set to something else, but is nver hit. > > > > I think in the end, what's needed is a way to say use these mapped > > types, and these only, and in this order. Sounds like he TODO is on > > that path. > > > > This seems esp. important when you start using REST on most actions > > that also double as browser/html methods rather than just using REST > > to expose a remote API. For now, an option is to simply remap those > > unwanted content types to the default too.

> > I've added a naĩve patch that allows the server to set up it's own > content-type priority list which can be used to select one content type > when the client accepts several types, but weighted identically. > > (See Catalyst-Action-REST-0.50-Accept.patch for details. Not that this > priority list should be user-configurable, something the patch doesn't do.) > > Hope this helps in the right direction! :) > > > - Salve

-- Evan Carroll System Lord of the Internets http://www.evancarroll.com