djc / couchdb-python

Python library for working with CouchDB
Other
202 stars 86 forks source link

Not url-encoding doc IDs in update handler call #309

Closed giftig closed 7 years ago

giftig commented 8 years ago

I'm doing a call like this:

            db.update_doc(
                const.RECORD_EXTERNAL_ID_UPDATE_HANDLER,
                docid=doc_id,
                body=json.dumps({
                  'something': 'data needed by my update handler'
                })
            )

...where doc_id is a string like REF-08911/2-0001

This is resulting in couch telling me 'invalid path', because the library isn't urlencoding doc_id before putting it into the URL used to hit couchdb. I've had to manually urllib.quote it, which imo shouldn't be necessary because the library should be transparent about where it's putting this data, and should do it itself since it knows it's putting it into a URL.

Apologies if this is an old / duplicate, I'm using version 1.0 of the library and haven't yet attempted to reproduce it with 1.1.

djc commented 8 years ago

That certainly sounds plausible. Can you rewrite your test case into a unit test like the ones in couchdb.tests, or at least something I can execute stand-alone?

giftig commented 8 years ago

Hmm I've written a unit test but it passes, which is interesting. I'll run it against the version of the library I'm using, and if it fails there then I guess I'm late to the party and I'll upgrade to 1.1. It's probably worth noting that I originally encountered this error while using the library to talk to cloudant, not vanilla couchdb, in which case it's possible it's their fault. I'll submit a pull request with my unit test anyway; can't hurt to have regression testing.

giftig commented 8 years ago

Passes when based onto 75e42dcc7923d597c177023b1ca8fe62712246cd, which I believe is the commit for the 1.0 release, so very possibly this is a problem which only happens with cloudant (which, naturally, I don't expect you to support). I'll try to reproduce with my original code running against couchdb instead of cloudant.

giftig commented 8 years ago

Okay, I've run my original code against couchdb instead of cloudant and it worked. I blame cloudant. I'll leave this open in case you want to look into making it work for cloudant, too, but since one seems to require urlencoding and the other doesn't, there may not be a single solution to suit both, which is a shame.

djc commented 8 years ago

I'd like CouchDB-Python to support Cloudant, but obviously not at any complexity cost. Let me know how you fare with Cloudant, I thought they mostly use the same code base and even if not, they probably want to stay compatible.

giftig commented 8 years ago

They're nearly 100% API-compliant, but occasionally that "nearly" bites me. My guess is couch takes the remainder of the path after it sees an update handler URL prefix and treats the rest as a doc ID, whereas cloudant matches the whole path against its router and expects the doc ID to be a urlencoded component. I'll do some experimentation; it may be that couch will accept it either url-encoded or not, whereas cloudant forces you to urlencode it, in which case the best-of-both would be to urlencode it regardless.

giftig commented 8 years ago

I modified the unit test I wrote to also try with the doc ID urlencoded, but alas it fails because it looks for a doc with a literal %2F in it in regular couchdb if I do that. Doesn't look like there's a good solution to fit both.

djc commented 8 years ago

That's too bad. 😞 Did you get any further feedback from the Cloudant team about this stuff?

giftig commented 8 years ago

Haven't spoken to them about it yet; I'll raise a ticket with them when I get chance. It'd be nice to know why they decided to change this, at least.

elistevens commented 8 years ago

I'd also check and see what the behavior is with couchdb 2.0; that's supposed to be much closer to cloudant's stuff.

Eli

On Tue, Nov 1, 2016 at 8:34 AM, Rob Moore notifications@github.com wrote:

Haven't spoken to them about it yet; I'll raise a ticket with them when I get chance. It'd be nice to know why they decided to change this, at least.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/djc/couchdb-python/issues/309#issuecomment-257599059, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIbIJiCCOm9aZS0om4OF3au_I-Og793ks5q51wQgaJpZM4KjOw6 .

giftig commented 8 years ago

Actually I did run it against 2.0 earlier but most of the tests failed because it refused to allow temporary views:

...
======================================================================
ERROR: test_multiple_rows (__main__.CouchTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "couchdb/tests/couch_tests.py", line 127, in test_multiple_rows
    results = list(self.db.query(query))
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/client.py", line 1260, in __iter__
    return iter(self.rows)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/client.py", line 1280, in rows
    self._fetch()
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/client.py", line 1266, in _fetch
    data = self.view._exec(self.options)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/client.py", line 1166, in _exec
    }, **_encode_view_options(options))
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/http.py", line 574, in post_json
    **params)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/http.py", line 593, in _request_json
    headers=headers, **params)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/http.py", line 589, in _request
    credentials=self.credentials)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/http.py", line 421, in request
    raise Forbidden(error)
Forbidden: (u'forbidden', u'Temporary views are not supported in CouchDB')

======================================================================
ERROR: test_update_with_unsafe_doc_ids (__main__.CouchTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "couchdb/tests/couch_tests.py", line 223, in test_update_with_unsafe_doc_ids
    _test_response(urllib.quote(doc_id, safe=''))
  File "couchdb/tests/couch_tests.py", line 209, in _test_response
    docid=doc_id
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/client.py", line 1024, in update_doc
    _, headers, body = func(**options)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/http.py", line 564, in put
    return self._request('PUT', path, body=body, headers=headers, **params)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/http.py", line 589, in _request
    credentials=self.credentials)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/http.py", line 429, in request
    raise ServerError((status, error))
ServerError: (500, (u'render_error', u'function raised error: (new TypeError("doc is null", "updates.test", 3)) \nstacktrace: (null,[object Object])@updates.test:3\nrunUpdate(function (doc, req) {doc.test = "passed";return [doc, "ok"];},[object Object],[object Array])@./share/server/main.js:970\n(function (doc, req) {doc.test = "passed";return [doc, "ok"];},[object Object],[object Array])@./share/server/main.js:1061\n("_design/test_slashes_in_doc_ids",[object Array],[object Array])@./share/server/main.js:1526\n()@./share/server/main.js:1571\n()@./share/server/main.js:1592\n@./share/server/main.js:1\n'))

======================================================================
ERROR: test_utf8_encoding (__main__.CouchTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "couchdb/tests/couch_tests.py", line 192, in test_utf8_encoding
    for idx, row in enumerate(self.db.query(query)):
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/client.py", line 1260, in __iter__
    return iter(self.rows)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/client.py", line 1280, in rows
    self._fetch()
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/client.py", line 1266, in _fetch
    data = self.view._exec(self.options)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/client.py", line 1166, in _exec
    }, **_encode_view_options(options))
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/http.py", line 574, in post_json
    **params)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/http.py", line 593, in _request_json
    headers=headers, **params)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/http.py", line 589, in _request
    credentials=self.credentials)
  File "/Users/robmo/Code/third-party/couchdb-python/couchdb/http.py", line 421, in request
    raise Forbidden(error)
Forbidden: (u'forbidden', u'Temporary views are not supported in CouchDB')

----------------------------------------------------------------------
Ran 9 tests in 3.585s

FAILED (errors=7)
giftig commented 8 years ago

Actually I've just noticed in the output that my test didn't fail for the same reason, though, it failed because it couldn't find the doc (because the encoding was wrong). So it seems encoding the slash still breaks on couch 2.0

giftig commented 8 years ago

I've just run into this again (in a completely different language) so I'm raising a support ticket with Cloudant this time; will let you know what they have to say. I've actually discovered it doesn't work in cloudant regardless of whether it's url-encoded; it always says "Invalid path" as if my slash is part of the url structure, so it seems it must be a bug.

giftig commented 7 years ago

FTR it took a while but I've heard back from Cloudant engineers and it seems the issue may in fact be in the nginx proxy my company uses to talk to Cloudant, as they say they can't reproduce the issue their end. So it seems the egg may be on my (or my colleague's) face after all.

djc commented 7 years ago

Ah, those proxies... Always manage to be less transparent than you thought.