PerlDancer / PerlDancer/Dancer2

Serialization failure is reported as 200 with an empty body; falsy values (0, "") can never be serialized

Open
#1,798 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Bug
Dominant language
Perl
Stars
604
Forks
288
Avg merge
1d 5h
Merged PRs (30d)
5

Description

Summary

When a serializer is configured and serialization does not produce a truthy string, the response is 200 OK with an empty body and Content-Type: text/html. The client is told the request succeeded and handed nothing; the diagnostic is logged at core level, which is below the default log_level of debug, so by default nothing is visible at either end.

Two distinct things are tangled up here, and I think only the first has been discussed before:

  1. A serialization failure is reported to the client as success. This is the case settled in #833 and #1054 — returning a non-reference under the JSON serializer is user error, and allow_nonref exists for those who want it. That decision isn't being reopened. What I don't think was decided is that the failure surfaces as an empty 200 text/html.
  2. A falsy-but-perfectly-valid serialized value can never be returned by any serializer0 and "" are lost even with allow_nonref => 1, because two guards treat "false" as "failed". I can't find this reported anywhere.

Reproducing

Self-contained, no config files needed:

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

{
    package App;
    use Dancer2;
    set serializer => 'JSON';
    get '/string' => sub { 'plain string' };
    get '/number' => sub { 42 };
    get '/zero'   => sub { 0 };
    get '/empty'  => sub { '' };
    get '/hash'   => sub { { a => 1 } };
}

my $test = Plack::Test->create( App->to_app );
for my $path (qw< /string /number /zero /empty /hash >) {
    my $res = $test->request( GET $path );
    printf "%-8s -> %s  %-18s len=%-3s %s\n",
        $path, $res->code, $res->header('Content-Type'),
        $res->header('Content-Length') // '-',
        length($res->content) ? "'".$res->content."'" : '*** EMPTY ***';
}
set serializer => 'JSON'
/string  -> 200  text/html          len=0   *** EMPTY ***
/number  -> 200  text/html          len=0   *** EMPTY ***
/zero    -> 200  text/html          len=0   *** EMPTY ***
/empty   -> 200  text/html          len=0   *** EMPTY ***
/hash    -> 200  application/json   len=7   '{"a":1}'
set serializer => 'JSON' with allow_nonref => 1
set engines => { serializer => { JSON => { allow_nonref => 1 } } };
set serializer => 'JSON';
/string  -> 200  application/json   len=14  '"plain string"'
/number  -> 200  application/json   len=2   '42'
/zero    -> 200  text/html          len=0   *** EMPTY ***     <-- still lost
/empty   -> 200  text/html          len=0   *** EMPTY ***     <-- still lost
/hash    -> 200  application/json   len=7   '{"a":1}'

0 and "" are valid JSON documents, and the serializer itself produces them correctly when called directly:

my $s = Dancer2::Serializer::JSON->new( config => { allow_nonref => 1 } );
$s->serialize(0);   # -> '0'   (correct)

They are lost above the serializer, not inside it.

Other serializers, for contrast

YAML and Dumper serialize non-references happily with no extra configuration, so "serializers need a reference" is a JSON constraint rather than a framework rule:

                      JSON            YAML                 Dumper
return 'plain string'  *** EMPTY ***   '--- plain string'   "$VAR1 = 'plain string';"
return 42              *** EMPTY ***   '--- 42'             '$VAR1 = 42;'
return 0               *** EMPTY ***   *** EMPTY ***        *** EMPTY ***
return ''              *** EMPTY ***   *** EMPTY ***        *** EMPTY ***
return { a => 1 }      '{"a":1}'       '---\na: 1\n'        "$VAR1 = {'a' => 1};"

Note the 0 and '' rows: every serializer loses them, including the ones that handle non-references fine.

Mechanism

Three places, each individually reasonable:

lib/Dancer2/Core/Role/Serializer.pm:42 — falsy content is returned unserialized, before the encoder is ever asked:

$content or return $content;

lib/Dancer2/Core/Response.pm:301 — a falsy result is treated as failure, so the early return skips the content_type call on the next line but one:

$content = $serializer->serialize($content)
    or return;

$self->content_type( $serializer->content_type );

Both conflate "false" with "failed". 0, "" and false are all legitimate JSON documents.

lib/Dancer2/Core/Role/Serializer.pm:50-57 — a genuine encoder exception is caught and logged at core:

$self->log_cb->( core => "Failed to serialize content: $error" );

core is -10 against debug's 1, and log_level defaults to debug, so this is filtered out by default. The scaffold's environments/development.yml sets log: "core", which is why the reporters in #833 and #1054 saw the message at all; under production.yml (log: "warning") or the framework default, nothing appears. The message itself is good when you can see it:

Failed to serialize content: hash- or arrayref expected (not a simple scalar,
use allow_nonref to allow this) at lib/Dancer2/Serializer/JSON.pm line 47.

Because Response::serialize returned early, content_type is never set, so the response keeps text/html, and around content stores '' — hence 200, empty, text/html.

Prior discussion

  • #833 and #1054 — same symptom via the scaffolded app, closed with the explanation that a serializer expects a reference and the guidance to split into two apps or use send_as.
  • Dancer2::Serializer::JSON documents allow_nonref for exactly this ("With this set the 'Hello, World!' handler returning a string will be dealt with properly").

I'm not asking to revisit either. The question is what should happen when serialization doesn't produce output.

What I think needs deciding

(a) How should a serialization failure be reported? A 200 with an empty body is the one answer that can't be right — it tells the client the request succeeded. Options, roughly in order of how disruptive they are:

  • Raise the log level from core to error or warning. Cheapest possible change, makes the existing message visible by default, changes no response.
  • Return 500 when the serializer was asked for output and produced none. Correct in my view — the app has failed to answer — but it converts a silently-empty 200 into a visible error for anyone currently relying on the status code, so it wants a deprecation cycle or at minimum a Changes note.
  • Leave the response alone and document it. Cheapest, but leaves the "success with no body" shape in place.

(b) Should 0 and "" be serializable? They're valid documents in every format Dancer2 ships, and the serializers already handle them correctly when called directly — only the two or return guards lose them. Fixing this means testing defined rather than truth in both places. The risk is that some code depends on a falsy return meaning "no content"; that would need checking against the existing suite, and it interacts with how a route returning undef is meant to behave.

(c) Is the JSON/YAML/Dumper divergence intended? If serializers are meant to require references, YAML and Dumper don't enforce it. If they aren't, JSON is the odd one out and allow_nonref could arguably default to on. Either is defensible, but right now the answer depends on which serializer you picked.

Happy to put a PR together for whichever way you want to go — (a) as a log-level bump is a one-liner, and (b) is small but wants a decision on undef first.

Environment

  • Dancer2 main at 23baea04
  • perl 5.32.1
  • JSON::MaybeXS 1.004003, backend Cpanel::JSON::XS

Found while reviewing a batch of findings from a test-suite audit by Curtis "Ovid" Poe; this one wasn't in the audit and is filed separately as it looks like a decision for the team rather than a bug with an obvious fix.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read lib/Dancer2/Core/Role/Serializer.pm and lib/Dancer2/Core/Response.pm, focusing on the guards and error logging described in the issue. Review the existing serializer and response test coverage before choosing behavior for serialization failures, undef, 0, and empty strings. Done requires an agreed response and tests covering the selected behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
perl
Domain
api, backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.