You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by jaydoane <gi...@git.apache.org> on 2015/06/29 21:52:51 UTC
[GitHub] couchdb-couch-replicator pull request: Handle case when replicatio...
GitHub user jaydoane opened a pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12
Handle case when replication param "doc_ids": "null"
BugzID: 48602
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-couch-replicator 48602-handle-doc_ids-null-string
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-couch-replicator/pull/12.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 #12
----
commit 1050eff30fa7f10dd25d7c54049a801fc6cd3e68
Author: Jay Doane <ja...@gmail.com>
Date: 2015-06-29T05:34:50Z
Handle case when replication param "doc_ids": "null"
BugzID: 48602
----
---
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-replicator pull request: Handle case when replicatio...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-116819764
Yes, it seems like this is a special case for clients who do not understand that "null" != null. Still, returning a 500 doesn't seem very friendly.
---
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-replicator pull request: Handle case when replicatio...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-116816740
Why `null` could be passed as `"null"`? I think the error is on client side that sends such request, isn't 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-replicator pull request: Handle case when replicatio...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-153158819
Erhm, fixed via 437c6571f. You can close this PR.
---
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-replicator pull request: Handle case when replicatio...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-148961332
@jaydoane https://github.com/cloudant/chttpd-local/pull/15 - 404 not found. Anything important there?
---
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-replicator pull request: Handle case when replicatio...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-116834561
AFAICT, there's no specific client that is sending this, but it was apparently discovered in testing.
The internal Cloudant bug that this PR attempts to address claims it was "fixed in couchdb" and refers to
https://git-wip-us.apache.org/repos/asf?p=couchdb.git;a=commitdiff;h=bea76dbf
But of course, that only fixes the "doc_ids":null case, not the specific case this bug report is calling out.
---
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-replicator pull request: Handle case when replicatio...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-118463246
It seems like the fundamental issue is that "null" is a string, but we're expecting an array. My inclination would be to return a response code 400 with the payload {"error":"parameter \`doc_ids\` must be an array"}.
To make this work, we presumably change the function clause to this:
```
convert_options([{<<"doc_ids">>, V} | _R]) when not is_list(V) ->
throw({bad_request, <<"parameter `doc_ids` must be an array">>});
```
but unfortunately, this also requires adding this additional try clause to chttpd_misc:handle_replicate_req:
```
{bad_request, Reason} ->
send_json(Req, 400, {[{error, Reason}]});
```
to avoid sending the client a 500 try_clause error.
Does this seem like the correct 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-replicator pull request: Handle case when replicatio...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-121134650
The follow-up PR: https://github.com/cloudant/chttpd-local/pull/15
---
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-replicator pull request: Handle case when replicatio...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane closed the pull request at:
https://github.com/apache/couchdb-couch-replicator/pull/12
---
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-replicator pull request: Handle case when replicatio...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-116823364
hrm, I don't think we should silently ignore junk input. There's no precedent in this function for sending a 400 bad request, but I think that's the right status code if we're improving this at all.
I note that other params that need numbers use couch_util:to_integer which will also function_clause if given something unexpected.
---
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-replicator pull request: Handle case when replicatio...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-118463333
As an aside, it seems like a more declarative approach to error handling might be desirable.
---
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-replicator pull request: Handle case when replicatio...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-153171910
@kxepal, thank you!
Also, looks like we need to create .travis.yml for this repo?
---
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-replicator pull request: Handle case when replicatio...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-116822338
Right, HTTP 400 looks as more correct one. Technically, such clients cannot implement JSON properly which means that there are more other places where they may hit the same issue (like views). Should we care about them?
P.S. Also wonder what are these clients if it's not a top secret (:
---
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-replicator pull request: Handle case when replicatio...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-149098426
@kxepal, I have closed the obsolete https://github.com/cloudant/chttpd-local/pull/15 and opened https://github.com/apache/couchdb-chttpd/pull/83, which applies the same patch onto the relevant ASF repo
---
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-replicator pull request: Handle case when replicatio...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-116853695
@janl Those who works in such way with HTTP deserves to suffer, that's fine (: That was acceptable in wild-wild 90x, but today is 2015.
---
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-replicator pull request: Handle case when replicatio...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-153158377
@jaydoane done. Thanks! (:
---
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-replicator pull request: Handle case when replicatio...
Posted by janl <gi...@git.apache.org>.
Github user janl commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-116851749
This is a nice “oh they have thought of *that*” kind of thing, kinda like http://blog.couchdb.org/2014/04/08/the-little-things1-do-not-delete/
---
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-replicator pull request: Handle case when replicatio...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/12#issuecomment-153156882
@kxepal, I rebased, and tested against latest chttpd to ensure it still works. Seeing:
```
$ curl http://127.0.0.1:15984/_replicate -u adm:pass -H 'Content-Type: application/json' -X POST -d ' {"source": "foo", "target": "bar", "doc_ids": "null"} '
{"error":"bad_request","reason":"parameter `doc_ids` must be an array"}
```
Can this be merged 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.
---