couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benoit Chesneau <bchesn...@gmail.com>
Subject Re: trouble with URL rewriting for key/startkey/endkey qs params
Date Tue, 22 Feb 2011 12:27:14 GMT
On Tue, Feb 22, 2011 at 9:52 AM, Benoit Chesneau <bchesneau@gmail.com> wrote:
> On Tue, Feb 22, 2011 at 6:15 AM, Adam Kocoloski <kocolosk@apache.org> wrote:
>> Hi, a user pointed out that a rewrite rule which adds quotes around a variable causes
the variable substution not to happen, e.g.
>>
>> {
>>  "query": { "key": "\":arg\"" }
>> }
>>
>> I had to stare a long time at couch_httpd_rewrite to see why that was the case.  I
found the replace_var/3 function to be awesomely gnarly:
>>
>> https://github.com/apache/couchdb/blob/trunk/src/couchdb/couch_httpd_rewrite.erl#L256
>>
>> There are so many bad things going on in this function:
>>
>> 1) I'm pretty sure the final clause of the case statement can never be reached.  Calls
to replace_var/3 are guarded by is_list/1 and is_binary/1 guards on the Value, so we're always
going to hit one of the previous clauses.
>>
>> 2) Putting that aside for a moment, what was the point of the inner case statement?
 Are we writing code that distinguishes between lists:flatten(?JSON_ENCODE(Value)) and iolist_to_binary(?JSON_ENCODE(Value))
?  I hope not.
>>
>> 3) I found the is_list(Value) clause to be almost a copy of the outer clause, but
not quite.  It looks to me like that clause is being used to handle the situation like "query":{"key":
[":a",":b"]}, but I believe there are better ways to do that.
>
>
>
>>
>> 4) Sure enough, the rewriter requires an exact match on <<":", Var/binary>>
in order to do the substitution, and it does not JSON encode the result, even if the query
key is something that requires a JSON-encoded value.  In effect, I see no way to use the
rewriter to set the value of key to a string.
>
> Not sure I follow, the key is supposed to be a binary here.
>>
>> I started to refactor this function a bit and ended up with http://friendpaste.com/ZzyuR2gM0JVdDmgGhjXbI.
 It still doesn't address the root problem of the JSON-encoding requirement for key/startkey/endkey/etc.,
but it's a start.  I've never had occasion to look closely at the rewriter before, so someone
else should check to make sure I'm not way out in left field here.
>>
>> Adam
>
> Hi, thanks to go back on this topic. I've already send a mail 1 month
> or 2 about that. There are different concerns to handl in the
> rewriter.
>
> - some peope are passing a list values via the query parameters.
> Values can be integer here, in this case last case is use or in array
> the 2 jspon
> - some values can be wildcards detected from the rules
> - values can just be binaries starting by a ":"
> - values can be lists or array
> - values can be string in the case a value is coming from url
> rewriting., values can be lists in an array.
>
> The guard:
> _ when is_list(Value) ->
>            Value1 = lists:foldr(fun(V, Acc) ->
>                V1 = case V of
>                    <<":", VName/binary>> ->
>                        case get_var(VName, Bindings, V) of
>                            V2 when is_list(V2) ->
>                                iolist_to_binary(V2);
>                            V2 -> V2
>                        end;
>                    <<"*">> ->
>                        get_var(V, Bindings, V);
>                    _ ->
>                        V
>                end,
>                [V1|Acc]
>            end, [], Value),
>            to_json(Value1);
>
> was trying to handle the last 2 cases. I didn't found the right way
> for now to detect strings against lists.
>
> It seems your diff handle only 2 cases, the ":" pattern matching
>
> I'm not happy with that and raised the issue long time ago and
> recently I've openened a thread for that on the ml without too much
> reactions:
>
> http://couchdb.markmail.org/thread/47iahuy4po55am4c
>
> The current designer don't handle and can't handle all the cases
> wanted by a user. Main problem is to manage different values coming.
> And for other they want a more complex rewriting rule. At the end I've
> rewritten the rewritter to achieve that:
>
> https://github.com/benoitc/couchapp-ng/
>
> The code of the new rewriter is also a little more simple. It also
> remove some oddities introcuced since I wrote the code like this
> useless atom transformation (to_binding) that I don't quite
> understand, etc. It also introduce some new features like directly
> allowing setup of proxies.  This code online isn't the last version I
> will use in upondata.com .
>
> I'm not claiming this code should replace the current one. We could
> start by changing the way the replace_var is working, but we surely
> will have hard time to do that. The first thing to do would be to find
> a way to detect strings vs lists. The version of couchapp_ng handle
> that by fixing patterns associated to a key.
>
> - benoit
>
i've worked on a better implementation based on your patch. complex
key are still failing but i'm on it:


http://friendpaste.com/7mEZ1v1vnq8TZLW55VI892

- benoît

Mime
View raw message