You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by iilyak <gi...@git.apache.org> on 2016/03/10 21:04:27 UTC
[GitHub] couchdb-chttpd pull request: 2966 before response hook
GitHub user iilyak opened a pull request:
https://github.com/apache/couchdb-chttpd/pull/106
2966 before response hook
This is a companion PR for https://github.com/apache/couchdb-couch/pull/150
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-chttpd 2966-before_response_hook
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-chttpd/pull/106.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #106
----
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196318212
The trick that `?_` macros doesn't involve function execution right now, and delegate it to the time when test will be actually processed by eunit. Otherwise it's easy to make a case when you can execute part of the code in between setup and actual test call.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196325193
Ok, I'll try to explain again. Fixtures, i.e. the test functions that end with _, take a list of _callbacks_. Here we are providing this list of callbacks in form of `fun before_request_match/0`, `fun before_request_no_match/0,` and so on. Fixture does not execute them immediately, but execute `setup/0` first and then starting to call them one-by-one. Each of those callbacks has to be a test object, i.e. has to either return result, anything, really, or throw an exception.
Macros family ?_assert exists to simplify this task, i.e. you can pass into your fixture a list of `[?_assert(true), ?_assertEqual(a, a)]` and as a result get a list `[{?LINE, fun() -> ?assert(true) end}, {?LINE, fun() -> ?assertEqual(a, a)}]` In other words ?_assert is test object generator, it creates the functions that, been called, either return or crash because this is what ?assert.. macros does.
Here we already passing in a list of callbacks in form of `fun function/arity`. Each of those function has to be a test object, i.e. do an assertion when called. But they are instead calling test object generators in form of ?_assert and because this macros is not failing by itself, but just generate tuples of {?LINE, callback}, the tests are passing.
To get this work properly each of the callbacks either has to call ?assert to do the assertion or instead of passing them as callbacks they need to be called in the list, i.e. `[before_request_match(), before_request_no_match()...]`. Then they'll propagate the list with ?_assert... generators _before_ fixture execution and the generated assertions will be executed _during_ the test after setup call.
I hope it makes sense now...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#discussion_r56064788
--- Diff: src/chttpd.erl ---
@@ -1068,3 +1057,18 @@ stack_hash(Stack) ->
%% dedicated chunk.
chunked_response_buffer_size() ->
config:get_integer("httpd", "chunked_response_buffer", 1490).
+
+basic_headers(Req, Headers0) ->
+ Headers = Headers0
+ ++ server_header()
+ ++ couch_httpd_auth:cookie_auth_header(Req, Headers0),
+ chttpd_cors:headers(Req, Headers).
+
+handle_response(Req, Code, Headers, Args, Type) ->
+ couch_stats:increment_counter([couchdb, httpd_status_codes, Code]),
+ respond_(Req, Code, Headers, Args, Type).
+
+respond_(#httpd{mochi_req = MochiReq}, Code, Headers, _Args, start_response) ->
--- End diff --
I stole it from @jaydoane https://github.com/cloudant/couchdb-couch/pull/10/files#diff-667bf0ed30e8a6b8ef281fa934cb9c06R718 :-) . I saw this convention in another project which I cannot recall.
In various erlang projects I saw conventions like:
- respond_
- respond0
- respond_int
- respond_1
- respond_priv
I really dislike things with number. So the choices left are `respond_int`, `respond_priv` and `respond_`. I would choose the last one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196833000
@iilyak: confirmed, it is working now. so, again +1 from me after squash
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196318614
(tbh, I'm not sure now: don't have a chance to check, so my case may be not applicable here. But still nice pitfall with eunit to be catched)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196420327
@eiri: I did push the fixes you've proposed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196317473
@kxepal Well, no. Try to change a match in any of the tests and they'll keep passing.
Fixtures accept either a list of the test objects or an instantinator, that'd take setup's return and return the list of the test objects. A test object is a function that, been executed, either returns any value and then asserted to be true or throws an exception and asserted to be false.
Here we are passing a list of the function, i.e. test objects and each of them executes `?_assert...` macros. That macros just returns tuple `{?LINE, fun() -> ?assert...}` and that treated as a positive assertion, because it is not an exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#discussion_r56066923
--- Diff: src/chttpd.erl ---
@@ -1068,3 +1057,18 @@ stack_hash(Stack) ->
%% dedicated chunk.
chunked_response_buffer_size() ->
config:get_integer("httpd", "chunked_response_buffer", 1490).
+
+basic_headers(Req, Headers0) ->
+ Headers = Headers0
+ ++ server_header()
+ ++ couch_httpd_auth:cookie_auth_header(Req, Headers0),
+ chttpd_cors:headers(Req, Headers).
+
+handle_response(Req, Code, Headers, Args, Type) ->
+ couch_stats:increment_counter([couchdb, httpd_status_codes, Code]),
+ respond_(Req, Code, Headers, Args, Type).
+
+respond_(#httpd{mochi_req = MochiReq}, Code, Headers, _Args, start_response) ->
--- End diff --
Oh, no, as I said I'm indifferent here. I'd say I do share your opinion on naming function with numbers, but think both `respond_` and `do_respond` are acceptable, so it's really up to you to keep the name or rename it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196126266
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#discussion_r56053708
--- Diff: src/chttpd.erl ---
@@ -1068,3 +1057,18 @@ stack_hash(Stack) ->
%% dedicated chunk.
chunked_response_buffer_size() ->
config:get_integer("httpd", "chunked_response_buffer", 1490).
+
+basic_headers(Req, Headers0) ->
+ Headers = Headers0
+ ++ server_header()
+ ++ couch_httpd_auth:cookie_auth_header(Req, Headers0),
+ chttpd_cors:headers(Req, Headers).
+
+handle_response(Req, Code, Headers, Args, Type) ->
+ couch_stats:increment_counter([couchdb, httpd_status_codes, Code]),
+ respond_(Req, Code, Headers, Args, Type).
+
+respond_(#httpd{mochi_req = MochiReq}, Code, Headers, _Args, start_response) ->
--- End diff --
Out of sheer curiosity, where such peculiar naming came from? Is there some erlang convention I'm not aware of? Don't get me wrong, I'm indifferent on the matter, it just looks a bit pythonesque.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196369014
@kxepal welp, erlang's eunits are nutty, no doubt about that, and the fact that I'm (mostly) understand how they work if anything is worrying :)
On a flip side the tests are not inherently broken, [here is tweaked version](https://gist.github.com/eiri/eec8542853746d1050a8) that runs asserts and passes.
Now I'm looking in the code trying to understand if my tweaks that led to this pass is how this functionality actually suppose to work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#discussion_r56066735
--- Diff: src/chttpd.erl ---
@@ -1068,3 +1057,18 @@ stack_hash(Stack) ->
%% dedicated chunk.
chunked_response_buffer_size() ->
config:get_integer("httpd", "chunked_response_buffer", 1490).
+
+basic_headers(Req, Headers0) ->
+ Headers = Headers0
+ ++ server_header()
+ ++ couch_httpd_auth:cookie_auth_header(Req, Headers0),
+ chttpd_cors:headers(Req, Headers).
+
+handle_response(Req, Code, Headers, Args, Type) ->
+ couch_stats:increment_counter([couchdb, httpd_status_codes, Code]),
+ respond_(Req, Code, Headers, Args, Type).
+
+respond_(#httpd{mochi_req = MochiReq}, Code, Headers, _Args, start_response) ->
--- End diff --
I'm neutral on this today, but if you like `do_respond` as well and @eiri don't mind - feel free to (:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196374223
@eiri: Good catch. The tests here https://github.com/apache/couchdb-couch/blob/master/test/couch_db_plugin_tests.erl are broken as well. I am aware that there is a pattern here so I'm planing to create a helper module in couch_epi application eventually for dispatching tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-chttpd/pull/106
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196464009
I take it we are omitting streaming bits as in _all_docs intentionally? Anyway, I'm +1, got a working plugin based on this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#discussion_r56066203
--- Diff: src/chttpd.erl ---
@@ -1068,3 +1057,18 @@ stack_hash(Stack) ->
%% dedicated chunk.
chunked_response_buffer_size() ->
config:get_integer("httpd", "chunked_response_buffer", 1490).
+
+basic_headers(Req, Headers0) ->
+ Headers = Headers0
+ ++ server_header()
+ ++ couch_httpd_auth:cookie_auth_header(Req, Headers0),
+ chttpd_cors:headers(Req, Headers).
+
+handle_response(Req, Code, Headers, Args, Type) ->
+ couch_stats:increment_counter([couchdb, httpd_status_codes, Code]),
+ respond_(Req, Code, Headers, Args, Type).
+
+respond_(#httpd{mochi_req = MochiReq}, Code, Headers, _Args, start_response) ->
--- End diff --
Do you want me to rename it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196336029
@eiri Thanks. So I was on the another case when that was applicable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196397273
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#discussion_r55953701
--- Diff: src/chttpd.erl ---
@@ -1068,3 +1057,18 @@ stack_hash(Stack) ->
%% dedicated chunk.
chunked_response_buffer_size() ->
config:get_integer("httpd", "chunked_response_buffer", 1490).
+
+basic_headers(Req, Headers0) ->
+ Headers = Headers0
+ ++ server_header()
+ ++ couch_httpd_auth:cookie_auth_header(Req, Headers0),
+ chttpd_cors:headers(Req, Headers).
+
+handle_response(Req, Code, Headers, Args, Type) ->
+ couch_stats:increment_counter([couchdb, httpd_status_codes, Code]),
+ respond_(Req, Code, Headers, Args, Type).
+
+respond_(#httpd{mochi_req = MochiReq}, Code, Headers, _Args, start_response) ->
--- End diff --
[Same story here](https://github.com/apache/couchdb-couch/pull/150#discussion_r55953630)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196501448
It looks like we can support streaming as well. I can add a call to chttpd_plugin:before_response [here](https://github.com/cloudant/couchdb-chttpd/blob/2966-before_response_hook/src/chttpd.erl#L784)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196303787
The tests in `test/chttpd_plugin_tests.erl` are over-wrapped. Fixture `callback_test_()` gets a list of callbacks each of which returns callback produced by `?_assert...` macro. Callbacks, been just functions and not results of invoking of those functions, naturally evaluate to true. Switching to actually calling assertions shows that right now 4 tests are passing and 11 are failing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196310500
@eiri that would actually work wrong by evaluating test function body except the last line before actually start process the test. That will easy notice by time spent for test function execution. So it's all correct here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#issuecomment-196544464
@eiri: streaming should work now. I don't know how I missed that last very important commit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: 2966 before response hook
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/106#discussion_r56065059
--- Diff: src/chttpd.erl ---
@@ -1068,3 +1057,18 @@ stack_hash(Stack) ->
%% dedicated chunk.
chunked_response_buffer_size() ->
config:get_integer("httpd", "chunked_response_buffer", 1490).
+
+basic_headers(Req, Headers0) ->
+ Headers = Headers0
+ ++ server_header()
+ ++ couch_httpd_auth:cookie_auth_header(Req, Headers0),
+ chttpd_cors:headers(Req, Headers).
+
+handle_response(Req, Code, Headers, Args, Type) ->
+ couch_stats:increment_counter([couchdb, httpd_status_codes, Code]),
+ respond_(Req, Code, Headers, Args, Type).
+
+respond_(#httpd{mochi_req = MochiReq}, Code, Headers, _Args, start_response) ->
--- End diff --
There is one more
- `do_respond`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---