CouchDB
  1. CouchDB
  2. COUCHDB-1852

Last-Event-ID header should be honoured in eventsource _changes feed

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4.0
    • Component/s: HTTP Interface
    • Labels:
      None

      Description

      When using the EventSource _changes feed support, the browser API will automatically reconnect and send the last sequence it received in the Last-Event-ID header.

      The server side needs to use the Last-Event-ID instead of the 'since' query string as a starting point for the changes feed.

      You can see the issue by simply creating a database and adding documents and watching what happens to the eventsource _changes feed.

      To see the feed with curl use:
      curl -i -H 'Last-Event-ID: 2' 'http://localhost:5984/testdb/_changes?feed=eventsource'
      the header should also have priority over the since query string:
      curl -i -H 'Last-Event-ID: 2' 'http://localhost:5984/testdb/_changes?feed=eventsource?since=1'

      1. 0002-Fix-for-COUCHDB-1852.patch
        3 kB
        Damjan Georgievski
      2. 0001-add-test-for-COUCHDB-1852.patch
        2 kB
        Damjan Georgievski

        Activity

        Hide
        ASF GitHub Bot added a comment -

        Github user sebastianrothbucher commented on a diff in the pull request:

        https://github.com/apache/couchdb/pull/354#discussion_r43657064

        — Diff: test/javascript/tests/changes.js —
        @@ -638,29 +664,36 @@ couchTests.changes = function(debug) {
        req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs");
        resp = JSON.parse(req.responseText);

        • TEquals(3, resp.last_seq);
          + // (seq as before)
          + //TEquals(3, resp.last_seq);
          TEquals(2, resp.results.length);
        • req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs&since=2");
          + // we can no longer pass a number into 'since' - but we have the 2nd last above - so we can use it (puh!)
          + req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs&since=" + encodeURIComponent(JSON.stringify(resp.results[0].seq)));
          resp = JSON.parse(req.responseText);
        • TEquals(3, resp.last_seq);
          + // (seq as before)
          + //TEquals(3, resp.last_seq);
          TEquals(1, resp.results.length);
          TEquals(2, resp.results[0].changes.length);

        // COUCHDB-1852

        • T(db.deleteDb());
          + // test w/ new temp DB
          + db_name = get_random_db_name();
          + db = new CouchDB(db_name, {"X-Couch-Full-Commit":"true"}

          );
          T(db.createDb());

        • // create 4 documents... this assumes the update sequnce will start from 0 and get to 4
          + // create 4 documents... this assumes the update sequnce will start from 0 and then do sth in the cluster
          db.save( {"bop" : "foom"}

          );
          db.save(

          {"bop" : "foom"}

          );
          db.save(

          {"bop" : "foom"}

          );
          db.save(

          {"bop" : "foom"}

          );
          + // because of clustering, we need the 2nd entry as since value
          + req = CouchDB.request("GET", "/" + db_name + "/_changes");

        // simulate an EventSource request with a Last-Event-ID header

        • req = CouchDB.request("GET", "/test_suite_db/_changes?feed=eventsource&timeout=0&since=0",
        • {"headers": {"Accept": "text/event-stream", "Last-Event-ID": "2"}});
          + req = CouchDB.request("GET", "/" + db_name + "/_changes?feed=eventsource&timeout=0&since=0",
          + {"headers": {"Accept": "text/event-stream", "Last-Event-ID": JSON.stringify(JSON.parse(req.responseText).results[1].seq)}});
            • End diff –

        did a PR: https://github.com/janl/couchdb/pull/4

        Show
        ASF GitHub Bot added a comment - Github user sebastianrothbucher commented on a diff in the pull request: https://github.com/apache/couchdb/pull/354#discussion_r43657064 — Diff: test/javascript/tests/changes.js — @@ -638,29 +664,36 @@ couchTests.changes = function(debug) { req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs"); resp = JSON.parse(req.responseText); TEquals(3, resp.last_seq); + // (seq as before) + //TEquals(3, resp.last_seq); TEquals(2, resp.results.length); req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs&since=2"); + // we can no longer pass a number into 'since' - but we have the 2nd last above - so we can use it (puh!) + req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs&since=" + encodeURIComponent(JSON.stringify(resp.results [0] .seq))); resp = JSON.parse(req.responseText); TEquals(3, resp.last_seq); + // (seq as before) + //TEquals(3, resp.last_seq); TEquals(1, resp.results.length); TEquals(2, resp.results [0] .changes.length); // COUCHDB-1852 T(db.deleteDb()); + // test w/ new temp DB + db_name = get_random_db_name(); + db = new CouchDB(db_name, {"X-Couch-Full-Commit":"true"} ); T(db.createDb()); // create 4 documents... this assumes the update sequnce will start from 0 and get to 4 + // create 4 documents... this assumes the update sequnce will start from 0 and then do sth in the cluster db.save( {"bop" : "foom"} ); db.save( {"bop" : "foom"} ); db.save( {"bop" : "foom"} ); db.save( {"bop" : "foom"} ); + // because of clustering, we need the 2nd entry as since value + req = CouchDB.request("GET", "/" + db_name + "/_changes"); // simulate an EventSource request with a Last-Event-ID header req = CouchDB.request("GET", "/test_suite_db/_changes?feed=eventsource&timeout=0&since=0", {"headers": {"Accept": "text/event-stream", "Last-Event-ID": "2"}}); + req = CouchDB.request("GET", "/" + db_name + "/_changes?feed=eventsource&timeout=0&since=0", + {"headers": {"Accept": "text/event-stream", "Last-Event-ID": JSON.stringify(JSON.parse(req.responseText).results [1] .seq)}}); End diff – did a PR: https://github.com/janl/couchdb/pull/4
        Hide
        ASF GitHub Bot added a comment -

        Github user kxepal commented on a diff in the pull request:

        https://github.com/apache/couchdb/pull/354#discussion_r43337892

        — Diff: test/javascript/tests/changes.js —
        @@ -638,29 +664,36 @@ couchTests.changes = function(debug) {
        req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs");
        resp = JSON.parse(req.responseText);

        • TEquals(3, resp.last_seq);
          + // (seq as before)
          + //TEquals(3, resp.last_seq);
          TEquals(2, resp.results.length);
        • req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs&since=2");
          + // we can no longer pass a number into 'since' - but we have the 2nd last above - so we can use it (puh!)
          + req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs&since=" + encodeURIComponent(JSON.stringify(resp.results[0].seq)));
          resp = JSON.parse(req.responseText);
        • TEquals(3, resp.last_seq);
          + // (seq as before)
          + //TEquals(3, resp.last_seq);
          TEquals(1, resp.results.length);
          TEquals(2, resp.results[0].changes.length);

        // COUCHDB-1852

        • T(db.deleteDb());
          + // test w/ new temp DB
          + db_name = get_random_db_name();
          + db = new CouchDB(db_name, {"X-Couch-Full-Commit":"true"}

          );
          T(db.createDb());

        • // create 4 documents... this assumes the update sequnce will start from 0 and get to 4
          + // create 4 documents... this assumes the update sequnce will start from 0 and then do sth in the cluster
          db.save( {"bop" : "foom"}

          );
          db.save(

          {"bop" : "foom"}

          );
          db.save(

          {"bop" : "foom"}

          );
          db.save(

          {"bop" : "foom"}

          );
          + // because of clustering, we need the 2nd entry as since value
          + req = CouchDB.request("GET", "/" + db_name + "/_changes");

        // simulate an EventSource request with a Last-Event-ID header

        • req = CouchDB.request("GET", "/test_suite_db/_changes?feed=eventsource&timeout=0&since=0",
        • {"headers": {"Accept": "text/event-stream", "Last-Event-ID": "2"}});
          + req = CouchDB.request("GET", "/" + db_name + "/_changes?feed=eventsource&timeout=0&since=0",
          + {"headers": {"Accept": "text/event-stream", "Last-Event-ID": JSON.stringify(JSON.parse(req.responseText).results[1].seq)}});
            • End diff –

        See https://github.com/apache/couchdb-chttpd/pull/89 . I made this value completely opaque as it was before, so there is no need to JSON encode seq for Last-Event-ID header. However, we can discuss this moment.

        Show
        ASF GitHub Bot added a comment - Github user kxepal commented on a diff in the pull request: https://github.com/apache/couchdb/pull/354#discussion_r43337892 — Diff: test/javascript/tests/changes.js — @@ -638,29 +664,36 @@ couchTests.changes = function(debug) { req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs"); resp = JSON.parse(req.responseText); TEquals(3, resp.last_seq); + // (seq as before) + //TEquals(3, resp.last_seq); TEquals(2, resp.results.length); req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs&since=2"); + // we can no longer pass a number into 'since' - but we have the 2nd last above - so we can use it (puh!) + req = CouchDB.request("GET", "/" + db.name + "/_changes?style=all_docs&since=" + encodeURIComponent(JSON.stringify(resp.results [0] .seq))); resp = JSON.parse(req.responseText); TEquals(3, resp.last_seq); + // (seq as before) + //TEquals(3, resp.last_seq); TEquals(1, resp.results.length); TEquals(2, resp.results [0] .changes.length); // COUCHDB-1852 T(db.deleteDb()); + // test w/ new temp DB + db_name = get_random_db_name(); + db = new CouchDB(db_name, {"X-Couch-Full-Commit":"true"} ); T(db.createDb()); // create 4 documents... this assumes the update sequnce will start from 0 and get to 4 + // create 4 documents... this assumes the update sequnce will start from 0 and then do sth in the cluster db.save( {"bop" : "foom"} ); db.save( {"bop" : "foom"} ); db.save( {"bop" : "foom"} ); db.save( {"bop" : "foom"} ); + // because of clustering, we need the 2nd entry as since value + req = CouchDB.request("GET", "/" + db_name + "/_changes"); // simulate an EventSource request with a Last-Event-ID header req = CouchDB.request("GET", "/test_suite_db/_changes?feed=eventsource&timeout=0&since=0", {"headers": {"Accept": "text/event-stream", "Last-Event-ID": "2"}}); + req = CouchDB.request("GET", "/" + db_name + "/_changes?feed=eventsource&timeout=0&since=0", + {"headers": {"Accept": "text/event-stream", "Last-Event-ID": JSON.stringify(JSON.parse(req.responseText).results [1] .seq)}}); End diff – See https://github.com/apache/couchdb-chttpd/pull/89 . I made this value completely opaque as it was before, so there is no need to JSON encode seq for Last-Event-ID header. However, we can discuss this moment.
        Hide
        ASF subversion and git services added a comment -

        Commit 002a908132bfe6a74e4a886fc539a3b7987d4c63 in couchdb-chttpd's branch refs/heads/master from Damjan Georgievski
        [ https://git-wip-us.apache.org/repos/asf?p=couchdb-chttpd.git;h=002a908 ]

        Support Last-Event-ID header for eventsource changes feeds

        COUCHDB-1852

        Show
        ASF subversion and git services added a comment - Commit 002a908132bfe6a74e4a886fc539a3b7987d4c63 in couchdb-chttpd's branch refs/heads/master from Damjan Georgievski [ https://git-wip-us.apache.org/repos/asf?p=couchdb-chttpd.git;h=002a908 ] Support Last-Event-ID header for eventsource changes feeds COUCHDB-1852
        Hide
        ASF subversion and git services added a comment -

        Commit 002a908132bfe6a74e4a886fc539a3b7987d4c63 in couchdb-chttpd's branch refs/heads/1843-feature-bigcouch-clustered-eventsource-changes-feed from Damjan Georgievski
        [ https://git-wip-us.apache.org/repos/asf?p=couchdb-chttpd.git;h=002a908 ]

        Support Last-Event-ID header for eventsource changes feeds

        COUCHDB-1852

        Show
        ASF subversion and git services added a comment - Commit 002a908132bfe6a74e4a886fc539a3b7987d4c63 in couchdb-chttpd's branch refs/heads/1843-feature-bigcouch-clustered-eventsource-changes-feed from Damjan Georgievski [ https://git-wip-us.apache.org/repos/asf?p=couchdb-chttpd.git;h=002a908 ] Support Last-Event-ID header for eventsource changes feeds COUCHDB-1852
        Hide
        ASF subversion and git services added a comment -

        Commit be1643b43106e30ae17e7aac92680c57be7018de in couchdb-chttpd's branch refs/heads/1843-feature-bigcouch-clustered-eventsource-changes-feed from Damjan Georgievski
        [ https://git-wip-us.apache.org/repos/asf?p=couchdb-chttpd.git;h=be1643b ]

        Support Last-Event-ID header for eventsource changes feeds

        COUCHDB-1852

        Show
        ASF subversion and git services added a comment - Commit be1643b43106e30ae17e7aac92680c57be7018de in couchdb-chttpd's branch refs/heads/1843-feature-bigcouch-clustered-eventsource-changes-feed from Damjan Georgievski [ https://git-wip-us.apache.org/repos/asf?p=couchdb-chttpd.git;h=be1643b ] Support Last-Event-ID header for eventsource changes feeds COUCHDB-1852
        Hide
        ASF subversion and git services added a comment -

        Commit dfd2199a4c6f67b76018eccfdd00b4fd4190e392 in branch refs/heads/master from Damjan Georgievski
        [ https://git-wip-us.apache.org/repos/asf?p=couchdb.git;h=dfd2199 ]

        Support Last-Event-ID header for eventsource changes feeds

        COUCHDB-1852

        Show
        ASF subversion and git services added a comment - Commit dfd2199a4c6f67b76018eccfdd00b4fd4190e392 in branch refs/heads/master from Damjan Georgievski [ https://git-wip-us.apache.org/repos/asf?p=couchdb.git;h=dfd2199 ] Support Last-Event-ID header for eventsource changes feeds COUCHDB-1852
        Hide
        Damjan Georgievski added a comment -

        New take on the test

        Show
        Damjan Georgievski added a comment - New take on the test
        Hide
        Damjan Georgievski added a comment -

        Here's the latest patches. I checked that the test fails for non-patched couch_httpd_db and succeeds on a patched one.

        I don't know if I can make the tests fail with a more reasonable error message?

        Show
        Damjan Georgievski added a comment - Here's the latest patches. I checked that the test fails for non-patched couch_httpd_db and succeeds on a patched one. I don't know if I can make the tests fail with a more reasonable error message?
        Hide
        Robert Newson added a comment -

        That looks better. A couple of comments but we're almost there;

        1) 'since it's probably the browser makes a reconnect' to 'since it's probably the browser reconnecting'
        2) A test to confirm the header overrides the since value in changes.js
        3) Add a sentence to ./share/doc/src/changes.rst to state that couchdb honors the Last-Event-ID header
        4) Add 'eventsource feeds now supports the Last-Event-ID header' to CHANGES and NEWS.

        Show
        Robert Newson added a comment - That looks better. A couple of comments but we're almost there; 1) 'since it's probably the browser makes a reconnect' to 'since it's probably the browser reconnecting' 2) A test to confirm the header overrides the since value in changes.js 3) Add a sentence to ./share/doc/src/changes.rst to state that couchdb honors the Last-Event-ID header 4) Add 'eventsource feeds now supports the Last-Event-ID header' to CHANGES and NEWS.
        Hide
        Damjan Georgievski added a comment -

        It seems I deleted the wrong one.

        Show
        Damjan Georgievski added a comment - It seems I deleted the wrong one.
        Hide
        Robert Newson added a comment -

        hm, I still see the original patch.

        Show
        Robert Newson added a comment - hm, I still see the original patch.
        Hide
        Damjan Georgievski added a comment -

        Updated the patch to add the header check in parse_changes_query, after all the query parameters are processed. That way the Last-Event-ID header overrides any "since" query params.

        Show
        Damjan Georgievski added a comment - Updated the patch to add the header check in parse_changes_query, after all the query parameters are processed. That way the Last-Event-ID header overrides any "since" query params.
        Hide
        Damjan Georgievski added a comment - - edited

        Here's a patch fixing this issue. This one works, but I could make it better if I move the line

        ChangesFun = couch_changes:handle_changes

        under the whole case ChangesArgs#changes_args.feed of block

        Show
        Damjan Georgievski added a comment - - edited Here's a patch fixing this issue. This one works, but I could make it better if I move the line ChangesFun = couch_changes:handle_changes under the whole case ChangesArgs#changes_args.feed of block

          People

          • Assignee:
            Unassigned
            Reporter:
            Damjan Georgievski
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development