Uploaded image for project: 'Spymemcached Java Client'
  1. Spymemcached Java Client
  2. SPY-57

view setKey and friends should be quoting/escaping Strings passed in

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.8.0-dp, 2.8.0-dp2
    • Fix Version/s: None
    • Component/s: library
    • Security Level: Public
    • Labels:
      None

      Description

      The current API forces the user to quote and encode the values that they're passing as parameters to the view. This should be simpler for the user, but it is not.

      For instance, if a user wanted the string asd"asd as their couch "key" start then they'd have to enter it "\"asd\"asd\"", which is a big burden on the developer.

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

        Activity

        Hide
        mikew Mike Wiederhold added a comment -

        This was filed incorrectly. On String arguments for query parameters I automatically add quotes.

        private String getArg(String key, Object value) {
        // Special case
        if (key.equals(STARTKEYDOCID))

        { return key + "=" + value; }

        if (value instanceof Boolean)

        { return key + "=" + ((Boolean) value).toString(); }

        else if (value instanceof Integer)

        { return key + "=" + ((Integer) value).toString(); }

        else if (value instanceof Stale)

        { return key + "=" + ((Stale) value).toString(); }

        else

        { return key + "=\"" + value + "\""; }

        }

        Show
        mikew Mike Wiederhold added a comment - This was filed incorrectly. On String arguments for query parameters I automatically add quotes. private String getArg(String key, Object value) { // Special case if (key.equals(STARTKEYDOCID)) { return key + "=" + value; } if (value instanceof Boolean) { return key + "=" + ((Boolean) value).toString(); } else if (value instanceof Integer) { return key + "=" + ((Integer) value).toString(); } else if (value instanceof Stale) { return key + "=" + ((Stale) value).toString(); } else { return key + "=\"" + value + "\""; } }
        Hide
        ingenthr Matt Ingenthron added a comment - - edited

        This needs to url encode the string, not just quote it. it will be an API change, but it's the right thing to do. We also need to document that we're expecting the string to be in the platform's default character encoding, just to match what is commonly done in the JDK[1].

        For example, someone could call setKey() with the following valid JSON:
        ["val1","val2"]
        And I think we just need to verify that we're converting it to URL encoded before sending the HTTP GET to the sserver:
        %5B%22val1%22%2C%22val2%22%5D

        The Java method call itself would look like: setKey("[\"val1\", \"val2\"]");

        Or, more likely, they're using a JSON library to generate the string that's getting passed in to setKey(). In fact, our test should do that.

        I think this is the right behavior for setKey(String key) at the moment. We may later want to add a setRawKey() or a setKey(String key, CouchbaseClient.encoding encoding), where that second param is some kind of encoding scheme we come up with if there's not a good standard one to use. That does bring up that we should probably have setKey() that takes a String and a Charset argument, since they may not supply UTF-8.

        [1]: http://download.oracle.com/javase/6/docs/api/java/net/URLEncoder.html

        Show
        ingenthr Matt Ingenthron added a comment - - edited This needs to url encode the string, not just quote it. it will be an API change, but it's the right thing to do. We also need to document that we're expecting the string to be in the platform's default character encoding, just to match what is commonly done in the JDK [1] . For example, someone could call setKey() with the following valid JSON: ["val1","val2"] And I think we just need to verify that we're converting it to URL encoded before sending the HTTP GET to the sserver: %5B%22val1%22%2C%22val2%22%5D The Java method call itself would look like: setKey(" [\"val1\", \"val2\"] "); Or, more likely, they're using a JSON library to generate the string that's getting passed in to setKey(). In fact, our test should do that. I think this is the right behavior for setKey(String key) at the moment. We may later want to add a setRawKey() or a setKey(String key, CouchbaseClient.encoding encoding), where that second param is some kind of encoding scheme we come up with if there's not a good standard one to use. That does bring up that we should probably have setKey() that takes a String and a Charset argument, since they may not supply UTF-8. [1] : http://download.oracle.com/javase/6/docs/api/java/net/URLEncoder.html
        Hide
        mikew Mike Wiederhold added a comment -

        Lowering priority because this isn't the cause of a bug. It might be nice to add this in the future, but at the moment it is not a mandatory fix. Also, URLEncoder is deprecated so if this is fixed we should probably use something else for the encoding.

        Show
        mikew Mike Wiederhold added a comment - Lowering priority because this isn't the cause of a bug. It might be nice to add this in the future, but at the moment it is not a mandatory fix. Also, URLEncoder is deprecated so if this is fixed we should probably use something else for the encoding.
        Hide
        mikew Mike Wiederhold added a comment -

        I'm going to close this issue because it appears that this is taken care of by the underlying http library.

        Show
        mikew Mike Wiederhold added a comment - I'm going to close this issue because it appears that this is taken care of by the underlying http library.

          People

          • Assignee:
            mikew Mike Wiederhold
            Reporter:
            ingenthr Matt Ingenthron
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Gerrit Reviews

              There are no open Gerrit changes