Patch for #165: For ViewQuery.keys(), use GET instead of POST

[ceefour]

For ViewQuery.keys(), use GET instead of POST. Fixed #165.

[paramaw]

İ'd like to suggest that this patch be accepted, it has saved me a significant cost saving with cloudant

[helun]

the build is failing.
have you run all tests locally?

[ceefour]

Ok I'll try to fix it.

[ceefour]

Ok I've found the issue, since the current tests actually expects the behavior, I'll need to add more tests

[YannRobert]

How is it working with GET request size limit ? this would depend a combination of the key size and key count.

I would like to point out that this particular feature can be implemented using method override
or the method could be extracted to a class, instance of which would be set as a dependency, and then dependency could be injected an alternative implementation by those who need to switch implementation.

This hopefully would avoid any change on ViewQuery class.
This would also avoid changing the existing behavior.

[ceefour]

The caller determines this:

• .keys() uses GET
• .keysUsingPost() uses POST
  On Apr 8, 2014 1:09 AM, "Yann ROBERT" notifications@github.com wrote:

How is it working with GET request size limit ? this would depend a
combination of the key size and key count.

I would like to point out that this particular feature can be implemented
using method override
or the method could be extracted to a class, instance of which would be
set as a dependency, and then dependency could be injected an alternative
implementation by those who need to switch implementation.

This hopefully would avoid any change on ViewQuery class.
This would also avoid changing the existing behavior.

Reply to this email directly or view it on GitHubhttps://github.com//pull/170#issuecomment-39763505
.

[helun]

The behavior should not change for current users. GET should only be used if you explicitly request that.

I think it should be controlled though a separate method:
.keys() should set the keys for all cases and
.usePostForMultipleKeys(boolean b)

[ceefour]

@helun, alright I'll change it to that.

I would like to propose that default uses GET though, the reason is this will be expected and most reasonable behavior for new users. And for Cloudant users, they'd be (unpleasantly) surprised seeing the keys() default to the expensive POST.

While the behavior changes, it should just work for all but the most extreme cases, in which case one should be able to easily fix it by using the proper method.

Do you approve?

[YannRobert]

Do you mind if I propose an alternative implementation that would fix #165 ?
I care because I do not wish for a new method in a already very long StdCouchDbConnector class.
Alternative implementation should use either method override or dependency injection.

[helun]

If @ceefour makes the changes i proposed there will just be one new method in ViewQuery:
usePostForMultipleKeys(boolean b)

or have I missed something?

[YannRobert]

The point is that the logic is too complex to be implemented in the same StdCouchDbConnector method

What if I want to use an automatic threshold, that, if the HTTP request can be done using GET because the query String is shorter than x Kb, then decides to use GET, otherwise fallback to POST ?

What if I want to use a constant or variable threshold, that, if the keys count it higher than X, uses POST, otherwise used GET ?

Some users would expect the framework to handle this, and propose yet another patch to the existing method, with another boolean that would drive the decision to use GET or POST ?

What if some other user one day ask for a feature to transparently split the query of 1000 IDs into 10 GET queries of 100 IDs ?

Then a user may ask for an automatic retry on failure, etc ...

I propose that method
private T executeQuery(final ViewQuery query, ResponseCallback rh)
should just delegate to
queryExecutor.executeQuery(final ViewQuery query, ResponseCallback rh)

with queryExecutor beeing a member of StdCouchDbConnector, of type QueryExecutor (new interface)
and a new class DefaultQueryExecutor than implements existing code

so that @ceefour can inject an alternative QueryExecutor into the StdCouchDbConnector instance he wants to change behavior of.

@ceefour could then contribute an AlwaysUseGetQueryExecutor implementation.
I could then contribute my FixThresholdQueryExecutor and AutomaticThresoldQueryExecutor.

This would not even require any change on the ViewQuery class
(excepting maybe an accessor on java.util.Map<java.lang.String,java.lang.String> queryParams)

As for encouraging new user to use AlwaysUseGetQueryExecutor, and existing user to switch, we can do this using documentation.

@ceefour would this fit your need ?

[ceefour]

Hi @helun, my latest commit soluvas@adc3cd2 implements your request, with a "twist" (as a way of persuading you to use GET as default):

GET is default as long as JSON array of keys is ≤ 3000 chars. Beyond that, it switches to POST. After some research, it seems that most 32-bit webservers defaulted to a 4K GET limit, while 64-bit webservers defaults to 8K. So I picked 3000 just as a compromise, with a bit of safe margin.

.usePostForMultipleKeys(true) is available when user wants to use POST even with small keys.

I hope this is acceptable. Please :)

[ceefour]

@YannRobert I have two opinions about it, but this is just my opinion.

I feel that Ektorp is a low-level CouchDB access library. It's a bit higher when compared to dealing with HttpClient, but feels the same level as JDBC. The features you described seems like it belongs to a higher abstraction, like what Hibernate does.

If these features (auto-retry, auto-split, etc.) will be included into Ektorp (which is nice), I believe it is a separate feature than what I'm trying to achieve here (and should be discussed separately), which is simply a low-level way to tell Ektorp what HTTP method to use.

[YannRobert]

http://en.wikipedia.org/wiki/Strategy_pattern

Do you need a StdCouchDbConnector to always use the same "strategy", when querying ? point 1
Or do you need your user to control the HTTP method to use on a per-method-invocation manner ? point 2

Point 1 would not require a change in ViewQuery. Just wire-in the strategy you want into StdCouchDbConnector

Point 2 may be done with a change in ViewQuery (as you did), but may be done using several instance of StdCouchDbConnector, each configured with the appropriate stategy.

As I understand your need, you are more concerned by always using the GET method, so always using an alternative implementation to the existing one.
So why not use a strategy pattern, and wire-in the strategy you need at application startup ?

Please let me do the refactoring so that you can wire-in the strategy you like. I may come up with a PR real soon.

[YannRobert]

Please see my PR #174

[helun]

We'll go with @YannRobert s solution, so I'll close this one

[ceefour]

@helun Thanks :) Now I can wait for 1.4.2 release :)