Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

make HTTP::Message::PSGI complain loudly about invalid PSGI responses #437

Merged
merged 1 commit into from Dec 3, 2013

Conversation

Projects
None yet
4 participants
Contributor

wchristian commented Nov 22, 2013

Without this, HTTP::Message::PSGI will silently suppress invalid responses, leading to confusing error messages from inside Plack::Test::MockHTTP.

@wchristian wchristian make HTTP::Message::PSGI complain loudly about invalid PSGI responses
Without this, HTTP::Message::PSGI will silently suppress invalid responses,
leading to confusing error messages from inside Plack::Test::MockHTTP.
1dffb9c

Coverage Status

Coverage decreased (-0.03%) when pulling 1dffb9c on wchristian:http-msg-invalid-response into eba6ba8 on plack:master.

Owner

miyagawa commented Nov 22, 2013

I'm from mobile and haven't looked at the diff but in general we should
avoid duplicating psgi compatible check in favor of Lint middleware and
alike.

What is the actual "confusing error message" and the use case that triggers
that?

Contributor

wchristian commented Nov 22, 2013

The specific message is:

Can't call method "request" on an undefined value at C:/Perl/site/lib/Plack/Test/MockHTTP.pm line 29.

It is caused, for example, by causing Web::Simple to return invalid PSGI responses in testing. If you'd like to run it, this is code that triggers it: https://metacpan.org/source/MSTROUT/Web-Simple-0.020/t/dispatch_misc.t#L81

Contributor

wchristian commented Nov 22, 2013

Also of note, i was inspired by this already existing check in HTTP::Server::PSGI:

https://github.com/plack/Plack/blob/master/lib/HTTP/Server/PSGI.pm#L181

Owner

miyagawa commented Nov 22, 2013

I agree the message could be improved but in general a framework is
responsible to return PSGI compatible response. If Web::Simple allows
returning PSGI response directly without validating it it should encourage
the use of Lint middleware etc in development and testing.

Contributor

wchristian commented Nov 22, 2013

Thanks for that feedback, that's helpful. I opened a bug on W::S to see what we want to do with this:

https://rt.cpan.org/Ticket/Display.html?id=90629&results=99f53a07c0e1ec331f66a6a7468a349f

As for this pull request, i understand your message such that you won't mind, in principle, changing the message. I assume you will be looking at it once you're on a bigger device and let me know if you'd like modifications to my pull request.

Is this correct?

Owner

miyagawa commented Nov 22, 2013

I'm OK with using Lint middleware (or similar) automatically in tests. I am
not OK with duplicating the invalidation of PSGI since it is fundamentally
a framework responsibility.

I am with a big size device but the current network doesn't get to
Github.com.

All this commit does is make it so if you get a reftype the code doesn't understand it throws an error rather than falling off the end of the sub and thereby returning undef. I absolutely agree that any non-trivial checking should exist only in the Lint middleware, but making it so the code dies with "Can't inflate undef as a PSGI response" or similar would be a huge improvement over "Can't call ->request on an undefined value", which makes no damn sense and takes half an hour to debug every time - and I've seen it on more than one framework now.

As such, while I concur that the responsibility to get it right is the framework's, it would be really nice if the test tools were a little kinder about pointing out what's wrong (you should've seen the utter confusion when there was a similar bug in Dancer's PSGI support until the core team gave up and screamed to me for help :)

Given that, I'd argue this PR should be applied - at least once you've had a chance to use a network that lets you look at it properly :D

@miyagawa miyagawa added a commit that referenced this pull request Dec 3, 2013

@miyagawa miyagawa Merge pull request #437 from wchristian/http-msg-invalid-response
make HTTP::Message::PSGI complain loudly about invalid PSGI responses
d3fc874

@miyagawa miyagawa merged commit d3fc874 into plack:master Dec 3, 2013

1 check passed

default The Travis CI build passed
Details

@wchristian wchristian deleted the wchristian:http-msg-invalid-response branch Dec 11, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment