Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ChangeTracker POST request are incompatible with CouchDB #1139

Closed
robertjpayne opened this issue Feb 24, 2016 · 26 comments

Comments

@robertjpayne
Copy link
Contributor

commented Feb 24, 2016

  • Version: 1.2 Release Branch
  • Client OS: iOS 9.2
  • Server: CouchDB 1.6

The ChangeTracker contains a _usePOST ivar which by default is set to YES. When this is set to YES the changes URL is built as simply _changes and all of the request parameters are pushed into a JSON body for the post request.

This is however invalid as CouchDB still expects the majority of parameters to come through the URL query params not the post body.

You can reproduce this by targeting any CouchDB database and running:

POST /_changes
Content-Type: application/json

{"limit": 5}

You will notice this does not actually limit the result set at all. Where as if you run:

POST /_changes?limit=5

It will correctly capture the parameters. As far as I can tell the only valid parameter to have in the POST body is doc_ids and all others should remain in the query params of the URL

@robertjpayne

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2016

I should say the result of this issue is that against CouchDB 1.6 the replication never ends, it continues to go forever.

@snej

This comment has been minimized.

Copy link
Member

commented Feb 25, 2016

"Incorrect" is the wrong wording. Sync Gateway has supported POST to _changes since 2013. I'm pretty sure that predates CouchDB. I publicly documented it in Feb 2014; in that doc it says we weren't aware of any other implementations at the time.

I don't know if the CouchDB people knew about my spec but didn't follow it, or if they just had the same idea but implemented it differently. Their API docs on it are too vague to be useful. The implementation seems weird; why would you keep most of the properties in the limited-size URL and just put one of them in the body?

Anyway, blame aside, there's an incompatibility. Right now Sync Gateway and CouchDB won't accept the same POST request, so one or the other will have to change to recognize the other's format.

@snej snej changed the title ChangeTracker incorrectly uses HTTP POST ChangeTracker POST request are incompatible with CouchDB Feb 25, 2016

@snej

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

I've filed an issue against SG. My suggestion there is to parse parameters from both the URL and the request body, to handle clients using both styles of request.

In CBL, similarly, we can put the parameters in both locations (except the filter params.) I think that'd be quite easy to implement.

@robertjpayne

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2016

@snej Ah yea this makes a lot more sense. Is there any reason the filter params cannot also be sent in both?

CouchDB is definitely more likely the fault here, it's very strange they don't take all the params from a POST request.

@snej

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

Is there any reason the filter params cannot also be sent in both?

Well, the whole reason for using a POST is to make more room for those filter parameters, because they can get arbitrarily large, too large to reasonably fit in a URL. (There's no explicit maximum length for URLs, but various client libraries, proxies, etc. will barf if they get URLs beyond a certain length, like a few kbytes.)

@snej snej added this to the 1.3 milestone Mar 30, 2016

@snej snej self-assigned this Mar 30, 2016

snej added a commit that referenced this issue Mar 30, 2016
Fixed incompatibility with CouchDB's _changes feed
CouchDB also supports POST to _changes but expects most of the parameters
to still be in the URL, not the request body.

Fixes #1139
@snej

This comment has been minimized.

Copy link
Member

commented Mar 30, 2016

OK, I have a fix. Tested it with a local CouchDB 1.6.1; CBL can pull a 5000-document db from it with no problems. @robertjpayne, it'd be great if you could try this fix with your own app & server. (You should be able to cherry-pick it onto the release/1.2.1 branch, to avoid destabilizing your app with all the other stuff on the dev branch.)

@martypenner

This comment has been minimized.

Copy link

commented Mar 31, 2016

Can confirm I experienced the same issue, and this resolved it.

@snej

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

Thanks!

@snej snej added review and removed in progress labels Mar 31, 2016

@snej

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

Merged to dev branch; closing.

@snej snej closed this Apr 1, 2016

@blairio

This comment has been minimized.

Copy link

commented Apr 14, 2016

AFAICT this fix has not been released yet. Is there an interim workaround for this issue when using the latest CBL (1.2.0) and CouchDB 1.6.1?

Thanks.

@snej

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

I can cherry-pick the fix to a release branch. I believe it's too late to add it to 1.2.1, but I can start a new branch if necessary. I'll try to get to this by EOD.

snej added a commit that referenced this issue Apr 14, 2016
Fixed incompatibility with CouchDB's _changes feed
CouchDB also supports POST to _changes but expects most of the parameters
to still be in the URL, not the request body.

Fixes #1139
@snej

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

OK, it's pushed to a new release/1.2.2 branch as commit 80901c3. (Don't take this as confirmation of a 1.2.2 release; I just needed to start a new branch because 1.2.1 is frozen already.)

@blairio

This comment has been minimized.

Copy link

commented Apr 15, 2016

Thanks for this @snej
I am assuming that I need to build this myself and I can't install couchbase lite as a pod anymore?
ie.
pod "couchbase-lite-ios", :git => "https://github.com/couchbase/couchbase-lite-ios.git", :branch => "release/1.2.2"
will not work.

@snej

This comment has been minimized.

Copy link
Member

commented Apr 16, 2016

I don't know that much about CocoaPods — @pasin is our local expert — but last I heard, our podspec just downloads a binary build from couchbase.com, instead of compiling locally. So that wouldn't work. Carthage, on the other hand, directly checks out a branch and builds it, so that would probably work.

@pasin

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2016

@snej is correct about couchbase-lite-ios CocoaPods andCarthage.

snej added a commit that referenced this issue Apr 17, 2016
Made CBLChangeTracker docIDs filter compatible with CouchDB
CouchDB still requires the filter= parameter in the URL.
Also added a ChangeTracker unit test for docIDs, and found two Sync
Gateway bugs using it :(

For #1139
snej added a commit that referenced this issue Apr 17, 2016
Made CBLChangeTracker docIDs filter compatible with CouchDB
CouchDB still requires the filter= parameter in the URL.
Also added a ChangeTracker unit test for docIDs, and found two Sync
Gateway bugs using it :(

For #1139
@protogrid

This comment has been minimized.

Copy link

commented Jun 23, 2016

Am I right in assuming that this issue is resolved, except for filter params? In other words: Is there a branch/commit which sends all "CBL.filterParams" in the URL query instead of using the POST body?

Update: I'm now able to send filter parametrs in the URL query, after chaning "_usePOST" in "CBLChangeTracker.initWithDatabaseURL" to "NO".

@snej snej added the bug label Jul 3, 2016

@hinesmr

This comment has been minimized.

Copy link

commented Aug 16, 2016

I'm having exactly this problem on Android (not iOS) using couchdb 1.6. I have stored some filter parameters via pull.setFilterParams(params);, but they don't show up in the URL.

Was the android version of couchbase lite never patched? I'm not seeing anything relevant in the ChangeTracker code.

@snej

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

Is there a branch/commit which sends all "CBL.filterParams" in the URL query instead of using the POST body?

I'm confused. The reason for using POST is so there's unlimited room for filter parameters like docIDs. Putting the filter params in the URL would negate that.

@snej

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

Was the android version of couchbase lite never patched?

I'm not sure. You should ask on the forum or mailing list, or file an issue against couchbase-lite-java-core.

@hinesmr

This comment has been minimized.

Copy link

commented Aug 17, 2016

@snej ^^^^ You need to read the rest of the issue above. This has already been discussed.

@hinesmr

This comment has been minimized.

Copy link

commented Aug 17, 2016

@snej I did send a mail to the list ---- but no response so far. I'm willing to open an issue or even submit a patch if someone would actually accept the patch.

I want to make sure it won't get shot down before I even submit it.

@snej

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

You need to read the rest of the issue above. This has already been discussed.

I am familiar with the issue, thanks. Putting the filter parameters in the URL would defeat the purpose of using POST, since it's those parameters that can become too large to fit in the URL. (The original post mentions doc_ids, but this isn't really a hardcoded parameter, just the specific one used by the _doc_ids filter.)

@snej

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

Could someone get some definitive info about how CouchDB interprets POST /db/_changes? They're incompatible with our implementation (which predates theirs), the documentation on their site is too vague to be useful, and I've gone around this issue twice already trying to be compatible.

@rajasaur

This comment has been minimized.

Copy link

commented Oct 6, 2016

We are experiencing the same problem(FilterParams being ignored on CouchDB) on upgrading to CBLite-Android (1.3.x from 1.1.1) and see that CouchDB is not picking up those changes.

@tleyden @hideki : Any chance we can allow for usePOST=false (as currently the only code that can do that is from the unit tests), which will allow the filterParams to go through as GET?

I understand that the main purpose is to avoid long GET urls (for e.g. our case may have a very long filterParams) but unfortunately, if the server isnt budging, then I think the client needs to be adjusted accordingly

@snej

This comment has been minimized.

Copy link
Member

commented Oct 6, 2016

The problem now isn't that CouchDB isn't budging, it's that they haven't documented how it behaves. If someone who knows Erlang wants to dive into the source code and reverse-engineer their exact behavior, I'll implement our client code compatibly. But I don't want to just blindly guess again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.