On Tue, Feb 22, 2011 at 9:52 AM, Benoit Chesneau <[email protected]> wrote: > On Tue, Feb 22, 2011 at 6:15 AM, Adam Kocoloski <[email protected]> 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
