unexpected side effects in Mojo::URL/Mojo::Parameters

[Scott Wiersdorf]

Hi! I asked about this on May 4, but didn't have the simplest-possible example to illustrate my problem, but I do now:

use Mojo::URL;
my $url = Mojo::URL->new('/v1/accounts?account=joe%40schmoe.org');
print "URL: " . $url->to_string . "\n";  ## account=joe%40schmoe.org

my @params = $url->query->param;  ## damage done
print "URL: " . $url->to_string . "\n";  ## account=j...@schmoe.org

In the first $url->to_string(), the URL stringifies to its original form.

In the second $url->to_string(), the URL stringifies with its query parameters *url-unescaped.*

The cause is that Mojo::Parameters->param calls to_hash(), which calls params(). In params(), we have this:

      # Unescape                                                                                                        
      $name  = url_unescape $name;
      $name  = decode($charset, $name) // $name if $charset;
      $value = url_unescape $value;
      $value = decode($charset, $value) // $value if $charset;
      push @$params, $name, $value;

Decoding the parameters is a Good Thing, but pushing them back into the $params makes it impossible to retrieve the original URL (and seems out of spirit with RFC 3986 to me).

I'm not sure what to do here; my instinct says that there should be a separate member of Mojo::Parameters called, say, "unescaped" where we can preserve these nicely unescaped parameters. Alternatively (in params()), instead of deleting the 'string' parameter, we keep it and check to see if params is defined instead. Yet another alternative would be to fix Mojo::URL to keep a copy of the original query string and use that instead of Mojo::Parameter's query method in to_string(). Other ideas?

I'm happy to code this with tests, etc. but could use some advice on what would be the Mojo way.

Scott

[Sebastian Riedel]

> I'm not sure what to do here; my instinct says that there should be a separate member of Mojo::Parameters called, say, "unescaped" where we can preserve these nicely unescaped parameters. Alternatively (in params()), instead of deleting the 'string' parameter, we keep it and check to see if params is defined instead. Yet another alternative would be to fix Mojo::URL to keep a copy of the original query string and use that instead of Mojo::Parameter's query method in to_string(). Other ideas?

Breaking changes like that are currently out of the question, i don't see how you could keep "$url->query->params->[0] .= 'foo'" working.

--
Sebastian Riedel
http://twitter.com/kraih
http://mojolicio.us

[Scott Wiersdorf]

On Thursday, September 13, 2012 12:05:33 AM UTC-6, Sebastian Riedel wrote:

> I'm not sure what to do here; my instinct says that there should be a separate member of Mojo::Parameters called, say, "unescaped" where we can preserve these nicely unescaped parameters. Alternatively (in params()), instead of deleting the 'string' parameter, we keep it and check to see if params is defined instead. Yet another alternative would be to fix Mojo::URL to keep a copy of the original query string and use that instead of Mojo::Parameter's query method in to_string(). Other ideas?

Breaking changes like that are currently out of the question, i don't see how you could keep "$url->query->params->[0] .= 'foo'" working. 

Fair enough... I can't see nice way around it either. Perhaps this thread can serve as a heads up to anyone who needs the original URL's escaping (I need it so I can hmac sign it and compare it with the incoming signature to check for tampering): you'll have to save it away somewhere with $url->to_string and use that for your comparison *unless* you haven't called $url->query->param (or to_hash, params, or parse), in which case $url->to_string uses the original unescaped query string.

Scott

[sri]

> Fair enough... I can't see nice way around it either.

I've never been a big fan of the current implementation, if there's a
better proposal we can consider it for 4.0.

--
sebastian

[John Stoffel]

>> Fair enough... I can't see nice way around it either.

sri> I've never been a big fan of the current implementation, if
sri> there's a better proposal we can consider it for 4.0.

What's going to break if you change it to fix his issue? I saw your
complain about breaking .= to a param, but honeslty didn't see what
the problem was.

I would think that keeping the params escaped at all times would be
ideal, and just force the user to decode them when they want them
decoded, not in the current un-obvious manner.

John

[sri]

> What's going to break if you change it to fix his issue?  I saw your
> complain about breaking .= to a param, but honeslty didn't see what
> the problem was.
>
> I would think that keeping the params escaped at all times would be
> ideal, and just force the user to decode them when they want them
> decoded, not in the current un-obvious manner.

Then try making that change without breaking any unit tests. :)

--
sebastian

[John Stoffel]

>>>>> "sri" == sri <kra...@googlemail.com> writes:

>> What's going to break if you change it to fix his issue?  I saw your
>> complain about breaking .= to a param, but honeslty didn't see what
>> the problem was.
>>
>> I would think that keeping the params escaped at all times would be
>> ideal, and just force the user to decode them when they want them
>> decoded, not in the current un-obvious manner.

sri> Then try making that change without breaking any unit tests. :)

Heh... I'm surprised any of the tests depend on this behavior. And I
honestly don't have the time or deep perl knowledge to make changes
like this. I just keep wanting to use Mojolicious in my own work, but
keep getting side tracked by CSS, JS and other issues. And since it's
all just spare time play work... it gets pushed way down the stack.

John

[Scott Wiersdorf]

Perhaps the Mojo::Parameters object could track whether a parameter's name and value were url-escaped when they came in, then use that as a default during stringification? I'll tinker with that when I have some time.

Scott