[PCBC-137] view querying needs to be more straightforward; e.g. character strings of decimal digits are serialised over the REST API as integers in view requests Created: 25/Oct/12  Updated: 11/Dec/12  Resolved: 23/Nov/12

Status: Resolved
Project: Couchbase PHP client library
Component/s: None
Affects Version/s: 1.1.0-dp5
Fix Version/s: 1.1.0
Security Level: Public

Type: Bug Priority: Blocker
Reporter: Michael Robinson Assignee: Matt Ingenthron
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   
Given this code:

 $key = "1";
 $result = $cb->view("design", "view", array("key"=>$key));

The PHP library will serialize this to a REST request as follows:

GET /bucket/_design/design/_view/view?key=1

This will be deserialized by couchbase as the integer value 1, not as the string value "1". Consequently, any key that is a string of decimal digits cannot be retrieved from a view with the PHP library.

This workaround can be used:

$key = "1";
$result = $cb->view("design", "view", array("key"=>'"'.$key.'"'));

The library will serialize this to a REST request as follows:

GET /bucket/_design/design/_view/view?key="1"

And couchbase will deserialize this as the string value "1".






 Comments   
Comment by Matt Ingenthron [ 25/Oct/12 ]
Mark: I saw you mention this earlier. Can you provide an assessment on this?
Comment by Mark Nunberg [ 25/Oct/12 ]
This is a more generalized bug or task of making php aware of the view parameters it passes.

Currently it only serializes the key-value pairs in the array as is. Having php know about where to place quotes would mean making the client aware abotu the variations in the view parameters, or in other words, implementing all of the view logic in the client library.

For now the user should be aware of this, and if something clearly mandates "quotes" then it should be quoted already when passed to the array.

While it's not the most elegant solution the fix to this is by far not surgical, and thus i recommend changing the name of this bug
Comment by Michael Robinson [ 25/Oct/12 ]
Would logic equivalent to this not solve the problem?

if (is_string($params["key"])) {
  $paramString += 'key="'.$params["key"].'"';
}
else {
  $paramString += 'key='.$params["key"];
}
Comment by Mark Nunberg [ 25/Oct/12 ]
No, because there are some parameters which do need actual integers.

Maybe the server can tolerate numeric parameters even fi they are enclosed in quotes, but I am not sure about that.

See

http://www.couchbase.com/docs/couchbase-manual-2.0/couchbase-views-querying-rest-api.html - and for example, group_level

Basically, the php client needs more logic to know the appropriate type for each parameter and perform the necessary coercion. i.e. numeric values for keys are not the same as numeric values for pagination
Comment by Michael Robinson [ 26/Oct/12 ]
Ok, it seems there are two problems at issue:

Problem 1: As a general case, type coercions across all parameters in the REST query string are not being handled as intelligently as they could be by the PHP library.

Problem 2: As a special case of problem 1, the PHP library will silently fail to retrieve records from a view if the key happens to be a string of decimal digits.

Without addressing the technical or architectural challenges of problem 1, it seems that, with respect to problem 2, as a general principle, silently failing to retrieve valid records for which a valid request was submitted is generally recognized as undesirable behavior for a database system.

This undesirable behavior could be rectified by the following (and as far as I can see, trivial) change to the serialization logic:

  If the parameter is in the set ("key", "startkey", "startkey_docid", "endkey", "endkey_docid"),
     and if the PHP type of the corresponding parameter value is "string",
       then serialize the parameter value in quotation marks in the REST query string,
     otherwise serialize the parameter value according to current implementation.

Yes?



    
Comment by Matt Ingenthron [ 26/Oct/12 ]
That does seem to make sense to me Michael. That's roughly what we evolved to on the other clients (Java && .NET), but there things are a bit different because of the types.
Comment by Matt Ingenthron [ 12/Nov/12 ]
Michael: can you take the lead on documenting a reasonable view query API. You have been through this once before, so I think you'd be the best person here. Simply document it here in this issue and then let's schedule a quick design review.
Comment by Michael Nitschinger [ 13/Nov/12 ]
Here is my proposal.

I've been thinking through and working around this while hacking on Basement (https://github.com/daschl/Basement) and also while extending the Java API. You may see similarities because of this (and you'll hopefully see why and how it makes sense).

At the lowest level, I think we should provide two ways of passing in queries in there.

1) A plain array as it is now. This way users can work around possible bugs and also have the flexibility if they know what they're doing. We should clearly state though that they are "on their own" when going down this route.

2) A CouchbaseViewQuery object (sorry, since we don't support namespaces in the extension). This works nearly similar to the Java-one, with the main difference that we need to care about working out the appropriate result type on our own (since you can pass in everything as a variable).

All further discussion (except noted) refers to the new object and how it should behave.

The construct should either take no params or an array to be initialized with. If it gets an array, it doesn't set the params directly but calls each setter for the given key to make sure that all rules are properly enforced.

Here is a simplified example. It assumes that all setter methods have the name as the param (like key() when setting/getting the key). If we want bean-style setters the method names need to be changed of course.

class CouchbaseViewQuery {

// holds the params to be exported
private $params = array();

// iterate over the given object an call setters.
public function __construct($params = array()) {
foreach($params as $key => $value) {
if($value !== null) {
$this->{$key}($value);
}
}
}

public function reduce($reduce = null) {
// getter/setter functionality in each method makes it condensed.
if($reduce == null) {
return $this->params['reduce'];
}

// do appropriate type checking for each param depending on whats
// supported.
if($reduce === 'true' || $reduce === true) {
$this->params['reduce'] = true;
}

// return object to let chain setters.
return $this;
}

}

Depending on how much we want to change the underlying interface, this query object can have either a toArray() or toString() export method that handles either only exporting it to an array, a string or also handle JSON serialization or encoding (don't know what of this parts is handled by libcouchbase itself).

Here is a possible usage example:

$query = new CouchbaseViewQuery();
$query->setReduce(true); // equally to $query->setReduce('true'); then
$cb->view('design', 'view', $query);

Also, all checks and conversions can then be handled transparently and securely by their appropriate setter methods. PHP also provides is_string() and so on to further determine what is passed into the variable.

Also, the export method is then in the position to do semantical checks like throw a warning/exception when reduce is false (or not set) and a group param is passed on. This could also be done in the setter but I'd like to see it done on exporting to reduce the coupling between setters.

The query handling object could look similar to this: https://github.com/daschl/Basement/blob/master/README.md#working-with-views (of course the method signature for view querying is differently, but the Query object would be nearly the same).

Let me know what you think guys,
Michael
Comment by Michael Nitschinger [ 13/Nov/12 ]
For design review.
Generated at Fri Aug 29 06:35:22 CDT 2014 using JIRA 5.2.4#845-sha1:c9f4cc41abe72fb236945343a1f485c2c844dac9.