You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by eiri <gi...@git.apache.org> on 2016/06/07 17:53:40 UTC
[GitHub] couchdb-couch-mrview pull request #47: Don't try to decode non-stringified J...
GitHub user eiri opened a pull request:
https://github.com/apache/couchdb-couch-mrview/pull/47
Don't try to decode non-stringified JSON in params
It is possible for the params "key", "keys", "startkey" and "endkey" to receive already decoded JSON array, if the query was done with `POST` and "keys" or "queries" payload.
COUCHDB-3031
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-couch-mrview 67171-dont-decode-json-in-params
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-couch-mrview/pull/47.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #47
----
commit 3a1cb4494b8a68187ea290779261cca07784bd81
Author: Eric Avdey <ei...@eiri.ca>
Date: 2016-06-07T17:47:32Z
Don't try to decode non stringified JSON in params
It is possible for the params "key", "keys", "startkey" and "endkey"
to receive already decoded JSON array, if the query was done with POST
and "keys" or "queries" payload.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #47: Don't try to decode non-stringified JSON in ...
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/47
I believe the is_list/1 check was an attempt at seeing if the value was a list-of-characters-string that could be passed to jiffy for decoding. Ie, it was an earlier attempt that forgot that startkey and endkey can be arbitrary JSON values and as such, lists of things as you discovered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #47: Don't try to decode non-stringified J...
Posted by eiri <gi...@git.apache.org>.
Github user eiri closed the pull request at:
https://github.com/apache/couchdb-couch-mrview/pull/47
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #47: Don't try to decode non-stringified JSON in ...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/47
nvm, got a failing query now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #47: Don't try to decode non-stringified JSON in ...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/47
I am struggling to reproduce the issue;
```
curl foo:bar@localhost:5984/db1/_design/foo/_view/bar -XPOST -Hcontent-type:application/json -d '{"queries":[{"key":"foo"}, {}]}'
{"results":[
{"total_rows":1,"offset":1,"rows":[
]},
{"total_rows":1,"offset":0,"rows":[
{"id":"doc1","key":null,"value":null}
]}
]}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #47: Don't try to decode non-stringified JSON in ...
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/47
I agree with @rnewson on this one. Seems like we should figure out why we're calling JSON_DECODE too many times rather than ignoring that and just avoiding the second invocation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #47: Don't try to decode non-stringified J...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/47#discussion_r66250044
--- Diff: src/couch_mrview_http.erl ---
@@ -576,6 +576,9 @@ check_view_etag(Sig, Acc0, Req) ->
parse_json(V) when is_list(V) ->
- ?JSON_DECODE(V);
+ case lists:all(fun is_binary/1, V) of
--- End diff --
specifically, in the lists:foldl, the original code parsed the arguments for each query.
parse_json should not have to defend itself against being called twice, the error is calling twice and we should address that directly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #47: Don't try to decode non-stringified JSON in ...
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/47
I think you misunderstood me about calling JSON_DECODE twice. Once was when we parse the POST body and the second is trying to parse the value. Not that parse_param is calling it twice.
Second, the worrisome bit to me is that your check for all items being a binary seemed wrong to me. The check that we need is "is_json" or "is_decoded_json" that is able to discern all possible values between needing decoding and not. What bothered me with this approach was I couldn't think of a solid way to write that. Yesterday I was contemplating a few variations using something like `io_lib:printable_unicode_list/1` or even finally adding a function to jiffy for something like `is_valid_json/1`. However neither of those seemed quite right and this morning I realized its because its impossible to tell that a provided term was decoded from JSON or not, because due to JSON you can embed values that could be decoded twice (ie, a string that decodes into a string that can be decoded, or an array of integers that decodes into what Erlang considers a string that can be decoded).
So I think the best solution to this is to add a parameter to parse_params/3 along the lines of "IsDecoded" that is threaded through to the parse_json/1 call which determines whether the value should be decoded or not.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #47: Don't try to decode non-stringified JSON in ...
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/47
Ah yes, I thought we are talking about this particular JSON_DECODE. So the guard on `is_list` here is not to tell json from a term apart, but for optimization reasons?
Passing options to `parse_params/3` sounds good. It might get a little bit awkward, since we are coupling validation with decoding in parse_param, but I'll look into it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #47: Don't try to decode non-stringified J...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/47#discussion_r66249692
--- Diff: src/couch_mrview_http.erl ---
@@ -576,6 +576,9 @@ check_view_etag(Sig, Acc0, Req) ->
parse_json(V) when is_list(V) ->
- ?JSON_DECODE(V);
+ case lists:all(fun is_binary/1, V) of
--- End diff --
this feels like a nasty hack to me, with potential for unintended side-effects. The original multi-query feature in the cloudant contribution did not suffer this problem.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request #47: Don't try to decode non-stringified J...
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/47#discussion_r66324338
--- Diff: src/couch_mrview_http.erl ---
@@ -576,6 +576,9 @@ check_view_etag(Sig, Acc0, Req) ->
parse_json(V) when is_list(V) ->
- ?JSON_DECODE(V);
+ case lists:all(fun is_binary/1, V) of
--- End diff --
the original cloudant multi-query solved it by cut'n'pasting couch_mrview_http:parse_params into chttpd_view.erl and removing json decode from the keys in question. to me it looks like more of a hack, but do you believe this is better approach?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview issue #47: Don't try to decode non-stringified JSON in ...
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the issue:
https://github.com/apache/couchdb-couch-mrview/pull/47
We are _not_ calling JSON_DECODE twice, this is not the problem here. We are getting list of query attributes as a json payload, decoding it [here](https://github.com/apache/couchdb-chttpd/blob/master/src/chttpd_view.erl#L56) and then passing them to `couch_mrview_http:parse_params/3`, assuming that it is idempotent to values of the attributes to be string of json or already decoded terms. And `parse_json` respects this for ints, strings, booleans or hashes, but assumes that _any_ list is a stringified json, while jiffy itself treat list of binaries as a decoded json array (`[<<"a">>, <<"b">>] = jiffy:decode(jiffy:encode([<<"a">>, <<"b">>]))`).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---