tokuhirom / p6-Crust

PSGI library stack for Perl6
Artistic License 2.0
66 stars 18 forks source link

Something subtle has changed in .pairup, this fixes it #99

Open jonathanstowe opened 7 years ago

jonathanstowe commented 7 years ago

Not quite sure what has changed in .pairup but it was getting

P6opaque: no such attribute '$!value' in type Pair when trying to get a value
  in block <unit> at -e line 1

when it encountered the pairs in the headers. This papers over that for the time being.

skaji commented 7 years ago

Could you give us a p6w app that emits an error in pairup?

jonathanstowe commented 7 years ago

It fails the t/Crust-Middleware/lint.t :+1:

        not ok 1 - 
        # Failed test at t/Crust-Middleware/lint.t line 113
        # The number of response headers needs to be even, not odd(Content-Typetext/plain Content-Length    123)
        1..1
        # Looks like you failed 1 test of 1
    not ok 1 - Should works fine
    # Failed test 'Should works fine'
    # at t/Crust-Middleware/lint.t line 104

But the actual reason isn't evident because the CATCH in the block there, if the exception is printed in the default case there it becomes evident that every invocation of validate-ret gives rise to the same error:

P6opaque: no such attribute '$!value' in type Pair when trying to get a value
  in sub validate-ret at /home/jonathan/devel/perl6/3rdparty-modules/p6-Crust/lib/Crust/Middleware/Lint.pm6 (Crust::Middleware::Lint) line 67
  in block  at /home/jonathan/devel/perl6/3rdparty-modules/p6-Crust/lib/Crust/Middleware/Lint.pm6 (Crust::Middleware::Lint) line 113

This only gives rise to one test failure because the remainder of the tests expect it do throw some exception, but doesn't check what exception :)

I may have a deeper look at this later because there's a possibility that those tests are passing accidentally because the resulting exceptions aren't being checked very deeply.

skaji commented 7 years ago

I confirmed that t/Crust-Middleware/lint.t failed; we can reduce that failure to a simple case:

❯ perl6 -v
This is Rakudo version 2017.08-30-g94fe65dbe built on MoarVM version 2017.08.1-19-g151a25634
implementing Perl 6.c.
❯ perl6 -e 'my $a = [ Content-Length => 10 ]; $a.pairup'
P6opaque: no such attribute '$!value' in type Pair when trying to get a value
  in block <unit> at -e line 1

According to the spec of P6W and an implementation of P6W server, HTTP response headers must be Lists of Paris.

So I think we should check that by something like

diff --git lib/Crust/Middleware/Lint.pm6 lib/Crust/Middleware/Lint.pm6
index 892e7a9..0c490b7 100644
--- lib/Crust/Middleware/Lint.pm6
+++ lib/Crust/Middleware/Lint.pm6
@@ -63,13 +63,8 @@ my sub validate-ret(@ret) {

     my $copy = @ret[1];

-    {
-        $copy.pairup();
-        CATCH {
-            default {
-                die 'The number of response headers needs to be even, not odd(', $copy, ')';
-            }
-        }
+    unless all(@($copy)) ~~ Pair {
+        die 'Response headers must be a List of Pairs, but ', $copy;
     }

     for $copy.kv -> $i, $v {
diff --git t/Crust-Middleware/lint.t t/Crust-Middleware/lint.t
index d4ffd2b..8fbd17e 100644
--- t/Crust-Middleware/lint.t
+++ t/Crust-Middleware/lint.t
@@ -146,7 +146,7 @@ subtest {
             sub (%env) { start { 200, ['invalid'], ['hello'] } }
         );
         dies-ok({await $code(%env)});
-    }, 'Should die because response header has odd elements';
+    }, 'Should die because response header is not a List of Pairs';

     subtest {
         my $code = Crust::Middleware::Lint.new(
skaji commented 7 years ago

The spec also says that

The headers MUST be a List of Pairs or an object that when coerced into a List becomes a List of Pairs.

What does "coerced" mean here? :)

jonathanstowe commented 7 years ago

I guess you could have a type that implements a .list method which returns a list of pars.

The previous implementation would allow an even sized list of key, value items to be acceptable. So <a 1 b 2> would be acceptable.