Uploaded image for project: 'Couchbase PHP client library'
  1. Couchbase PHP client library
  2. 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

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.1.0-dp5
    • Fix Version/s: 1.1.0
    • Component/s: None
    • Security Level: Public
    • Labels:
      None

      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".

      No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

        Hide
        daschl Michael Nitschinger added a comment -

        For design review.

        Show
        daschl Michael Nitschinger added a comment - For design review.
        Hide
        daschl Michael Nitschinger added a comment -

        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

        Show
        daschl Michael Nitschinger added a comment - 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
        Hide
        ingenthr Matt Ingenthron added a comment -

        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.

        Show
        ingenthr Matt Ingenthron added a comment - 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.
        Hide
        ingenthr Matt Ingenthron added a comment -

        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.

        Show
        ingenthr Matt Ingenthron added a comment - 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.
        Hide
        michael8robinson Michael Robinson added a comment -

        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?

        Show
        michael8robinson Michael Robinson added a comment - 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?
        Hide
        mnunberg Mark Nunberg added a comment - - edited

        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

        Show
        mnunberg Mark Nunberg added a comment - - edited 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
        Hide
        michael8robinson Michael Robinson added a comment -

        Would logic equivalent to this not solve the problem?

        if (is_string($params["key"]))

        { $paramString += 'key="'.$params["key"].'"'; }

        else

        { $paramString += 'key='.$params["key"]; }
        Show
        michael8robinson Michael Robinson added a comment - Would logic equivalent to this not solve the problem? if (is_string($params ["key"] )) { $paramString += 'key="'.$params["key"].'"'; } else { $paramString += 'key='.$params["key"]; }
        Hide
        mnunberg Mark Nunberg added a comment -

        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

        Show
        mnunberg Mark Nunberg added a comment - 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
        Hide
        ingenthr Matt Ingenthron added a comment -

        Mark: I saw you mention this earlier. Can you provide an assessment on this?

        Show
        ingenthr Matt Ingenthron added a comment - Mark: I saw you mention this earlier. Can you provide an assessment on this?

          People

          • Assignee:
            ingenthr Matt Ingenthron
            Reporter:
            michael8robinson Michael Robinson
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Gerrit Reviews

              There are no open Gerrit changes