Permalink
Browse files

Add tests to illustrate writability of query_parameters(), body_param…

…eters() and parameters()

While all the tests are written to pass, they illustrate an
inconsistency in the API which could stand to be clarified in the code
or docs.

The hashref returned by query_parameters() and body_parameters() and
parameters() are all read/write -- subsequent calls to the same method
return the modified hashref.

However, modifying the hashes returned by body_parameters() or
query_paremeters() does not modify the hashref returned by
parameters(), which claims to be a merger of the two.

It seems that either all the return values should be read-only,
( always returning the same values ), or if modifying them is supported
then the parameters() hash should be updated when either of the
body_parameters() or query_parameters() hashes are updated.
  • Loading branch information...
1 parent 5391528 commit 0aefed0900eeb2281eb0fe925bd18dfd60076b1e @markstos committed Dec 25, 2010
Showing with 45 additions and 0 deletions.
  1. +7 −0 t/Plack-Request/body.t
  2. +11 −0 t/Plack-Request/parameters.t
  3. +27 −0 t/Plack-Request/query_parameters.t
View
@@ -9,6 +9,13 @@ my $app = sub {
my $req = Plack::Request->new(shift);
is_deeply $req->body_parameters, { foo => 'bar' };
is $req->content, 'foo=bar';
+
+ my $b = $req->body_parameters;
+ $b->{foo} = 'body-updated';
+
+ my $b2 = $req->body_parameters;
+ is ($b2->{foo}, "body-updated", "body parameters are read-write");
+
$req->new_response(200)->finalize;
};
@@ -14,6 +14,17 @@ my $app = sub {
is_deeply $req->parameters, { foo => 'bar', 'bar' => 'baz' };
+ $b->{foo} = 'body-updated';
+ $b->{bar} = 'query-updated';
+
+ # is_deeply $req->parameters,
+ # { foo => 'body-updated', 'bar' => 'query-updated' },
+ # "body and and query parameters can be modified";
+
+ is_deeply $req->parameters,
+ { foo => 'bar', 'bar' => 'baz' },
+ "changes to values returned by parameters() do not persist.";
+
$req->new_response(200)->finalize;
};
@@ -0,0 +1,27 @@
+use strict;
+use warnings;
+use Test::More;
+use Plack::Test;
+use Plack::Request;
+use HTTP::Request::Common;
+
+my $app = sub {
+ my $req = Plack::Request->new(shift);
+ is_deeply $req->query_parameters, { foo => 'bar' };
+
+ my $b = $req->query_parameters;
+ $b->{foo} = 'query-updated';
+
+ my $b2 = $req->query_parameters;
+ is ($b2->{foo}, "query-updated", "query parameters are read-write");
+
+ $req->new_response(200)->finalize;
+};
+
+test_psgi $app, sub {
+ my $cb = shift;
+ my $res = $cb->(GET "/?foo=bar" );
+ ok $res->is_success;
+};
+
+done_testing;

4 comments on commit 0aefed0

Owner

markstos replied Dec 25, 2010

Maybe body_parameters(), query_parameters() and uploads() should all call ->clone on their return values?

That won't prevent people from modifying the hashref returned, but it will keep the return-values of the method consistent. This seems consistent with the design of having a maximal amount of the interface be read-only.

Owner

markstos replied Feb 5, 2011

Note that there are more tests for writability of parameters() in the neighboring commit: c9029ca

Owner

markstos replied Sep 27, 2012

It's been a couple of years now... is there any feedback on this contribution?

markstos: I think you need to comment on the original pull request (I don't see it - where is it? https://github.com/plack/Plack is the canonical repository now) for anyone else to be aware of it.

Personally I find the potential inconsistencies between parameters(), body_parameters() and query_parameters() to be a hole that should be closed.

Please sign in to comment.