CouchDB
  1. CouchDB
  2. COUCHDB-882

Nonstandard HTTP methods not converted to JSON correctly

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.0.1
    • Fix Version/s: None
    • Component/s: HTTP Interface
    • Labels:
      None
    • Environment:

      Erlang R13, Linux

    • Skill Level:
      Regular Contributors Level (Easy to Medium)

      Description

      Since COUCHDB-815, CouchDB allows nonstandard or unknown HTTP methods in case a _show or similar function may want to implement a response to that method.

      Unfortunately the (my) patch in that ticket used couch_util:to_existing_atom which returns the passed value unmodified if it has no corresponding atom. That is wrong because the HTTP method will be copied into the `req` object in the view server, therefore it must not be an Erlang string (list of integers) because those do not JSONify correctly. Instead, if the atom does not exist, the method should be converted to a binary.

        Activity

        Hide
        Jason Smith added a comment -

        Patch to convert unknown HTTP methods to binary instead of the format Mochi returns--list.

        Show
        Jason Smith added a comment - Patch to convert unknown HTTP methods to binary instead of the format Mochi returns--list.
        Hide
        Filipe Manana added a comment -

        Why not leave it always as a binary?
        That is, avoid at all trying to convert to an atom.

        Show
        Filipe Manana added a comment - Why not leave it always as a binary? That is, avoid at all trying to convert to an atom.
        Hide
        Jason Smith added a comment -

        I did not know why an atom was chosen instead of binary in the first place, so I kept the original functionality, only changing for the newer situation.

        For one thing, the log message may require changing since right now it says 'GET', 'PUT', etc. so I think that is ~p in the format. It will look pretty bad as <<"GET">>, etc. in the logs.

        I will try to use 100% atoms and submit another patch (during/after couchcamp).

        Show
        Jason Smith added a comment - I did not know why an atom was chosen instead of binary in the first place, so I kept the original functionality, only changing for the newer situation. For one thing, the log message may require changing since right now it says 'GET', 'PUT', etc. so I think that is ~p in the format. It will look pretty bad as <<"GET">>, etc. in the logs. I will try to use 100% atoms and submit another patch (during/after couchcamp).
        Hide
        Jason Smith added a comment -

        Sorry, 100% binaries!

        Show
        Jason Smith added a comment - Sorry, 100% binaries!
        Hide
        Filipe Manana added a comment -

        What I meant is:

        Method1 =
        case MochiReq:get(method) of
        % already an atom
        Meth when is_atom(Meth) -> Meth;

        % Non standard HTTP verbs aren't atoms (COPY, MOVE etc) so convert when
        % possible (if any module references the atom, then it's existing).
        Meth -> couch_util:to_binary(Meth)
        end,

        Like this, everywhere in the code you know you have to use an atom to match standard methods, an binaries for others.
        To me it seems like this it simplifies Erlang code that needs to reference non-standard methods.

        Show
        Filipe Manana added a comment - What I meant is: Method1 = case MochiReq:get(method) of % already an atom Meth when is_atom(Meth) -> Meth; % Non standard HTTP verbs aren't atoms (COPY, MOVE etc) so convert when % possible (if any module references the atom, then it's existing). Meth -> couch_util:to_binary(Meth) end, Like this, everywhere in the code you know you have to use an atom to match standard methods, an binaries for others. To me it seems like this it simplifies Erlang code that needs to reference non-standard methods.
        Hide
        Jason Smith added a comment -

        Simplified patch which simply converts any non-atom received from mochi into binary.

        Show
        Jason Smith added a comment - Simplified patch which simply converts any non-atom received from mochi into binary.
        Hide
        Jan Lehnardt added a comment -

        With trunk, the patch makes the following test cases fail:

        copy_doc error 126ms

        1. Run with debuggerAssertion failed: xhr.status == 201
        2. Exception raised:

        {"message":"db.open(\"doc_that_was_copied\") is null","fileName":"http://127.0.0.1:5984/_utils/script/couch_test_runner.js?0.11.0","lineNumber":54,"stack":"((void 0))@http://127.0.0.1:5984/_utils/script/couch_test_runner.js?0.11.0:54\u000arun(-4)@http://127.0.0.1:5984/_utils/script/couch_test_runner.js?0.11.0:91\u000a"}

        stats failure 5627ms

        1. Run with debuggerAssertion 'Copying docs increments doc writes.' failed: expected '21', got '20'

        Show
        Jan Lehnardt added a comment - With trunk, the patch makes the following test cases fail: copy_doc error 126ms 1. Run with debuggerAssertion failed: xhr.status == 201 2. Exception raised: {"message":"db.open(\"doc_that_was_copied\") is null","fileName":"http://127.0.0.1:5984/_utils/script/couch_test_runner.js?0.11.0","lineNumber":54,"stack":"((void 0))@http://127.0.0.1:5984/_utils/script/couch_test_runner.js?0.11.0:54\u000arun(-4)@http://127.0.0.1:5984/_utils/script/couch_test_runner.js?0.11.0:91\u000a"} stats failure 5627ms 1. Run with debuggerAssertion 'Copying docs increments doc writes.' failed: expected '21', got '20'
        Hide
        Jan Lehnardt added a comment -

        Bump to 1.3.x.

        Show
        Jan Lehnardt added a comment - Bump to 1.3.x.

          People

          • Assignee:
            Unassigned
            Reporter:
            Jason Smith
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development