You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/03/30 12:51:33 UTC
[GitHub] [couchdb] garrensmith opened a new pull request #3481: Validate ddoc uses couch_eval
garrensmith opened a new pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481
## Overview
Change the validate ddoc check to use couch_eval.
Also add in some extra functions in couch_eval so that the try_compile
will work.
## Testing recommendations
All current design doc tests should pass.
## Related Issues or Pull Requests
<!-- If your changes affects multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [x] Code is written and works correctly
- [x] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak edited a comment on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak edited a comment on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-810801703
There are very few places where we need to call `acquire_context/1` and `release_context/1` so I am ok with proposed change. However I wanted to make sure you did consider a different option, which is something like the following:
```
-module(couch_eval).
-export([
...
- map_docs/2
+ map_docs/2,
+ with_context/2,
+ try_compile/3
]).
with_context(#{language := Language}, Fun) ->
try acquire_context(Language) of
{ok, Ctx} -> Fun(Ctx)
after
release_context(Ctx)
end.
%% this would be private function
acquire_context(Language) ->
ApiMod = get_api_mod(Language),
{ok, Ctx} = ApiMod:acquire_context(),
{ok, {ApiMod, Ctx}}.
```
Then the caller would look like:
```
-module(couch_mrview).
validate(DbName, _IsDbPartitioned, DDoc) ->
...
Views =/= [] andalso couch_eval:with_context(#{language => Lang}, fun(Ctx) ->
lists:foreach(fun(V) -> ValidateView(Ctx, V) end, Views)
end),
ok.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-811016853
> > I had no heavy QA in mind, just a single-line eunit meck-y check to assert that couch_mrview:validate is called at the right time. (We could even assert couch_index_server:validate wasn't called, if that's a fundamental part of this change.)
>
> Since `couch_index_server:validate/2` is no longer used we could break it and other tests would fail if we partially revert the PR.
>
> ```
> -module(couch_index_server).
> ...
> validate(_Db, _DDoc) ->
> error({assert, ?MODULE, ?FUNCTION_NAME, ?FUNCTION_ARITY, ?LINE}).
> ```
I didn't know that `couch_index_server:validate` would become dead code after this change. So we might as well remove the `fun` altogether then, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] garrensmith commented on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
garrensmith commented on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-811048201
@bessbd right. I've deleted the function. I think that is the best approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r604619218
##########
File path: src/couch_js/src/couch_js.erl
##########
@@ -49,3 +52,15 @@ map_docs(Proc, Docs) ->
end, Results),
{Doc#doc.id, Tupled}
end, Docs)}.
+
+acquire_context() ->
+ Ctx = couch_query_servers:get_os_process(?JS),
+ {ok, Ctx}.
+
+
+release_context(Proc) ->
+ couch_js_query_servers:ret_os_process(Proc).
+
+
+try_compile(Proc, FunName, FunSrc) ->
+ couch_query_servers:try_compile(Proc, map, FunName, FunSrc).
Review comment:
Is it always would be a `map` in a second argument?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] garrensmith commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
garrensmith commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r604701498
##########
File path: src/couch_mrview/src/couch_mrview.erl
##########
@@ -203,22 +203,23 @@ validate(DbName, _IsDbPartitioned, DDoc) ->
Msg = ["`", Bad, "` is not a supported reduce function."],
throw({invalid_design_doc, Msg});
({RedName, RedSrc}) ->
- couch_query_servers:try_compile(Proc, reduce, RedName, RedSrc)
+ couch_eval:try_compile(Ctx, RedName, RedSrc)
end, Reds)
end,
{ok, #mrst{
language = Lang,
views = Views
}} = couch_mrview_util:ddoc_to_mrst(DbName, DDoc),
- try Views =/= [] andalso couch_query_servers:get_os_process(Lang) of
+ try Views =/= [] andalso couch_eval:acquire_context(Lang) of
Review comment:
I don't think so. I'm happy for that error to bubble up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r605104153
##########
File path: src/couch/src/couch_db.erl
##########
@@ -895,7 +895,7 @@ validate_doc_update(Db, Doc, GetDiskDocFun) ->
validate_ddoc(Db, DDoc) ->
try
- ok = couch_index_server:validate(Db, couch_doc:with_ejson_body(DDoc))
+ ok = couch_mrview:validate(Db, couch_doc:with_ejson_body(DDoc))
Review comment:
ok, I'm onboard with this change and will keep an eye out for the subsequent PR's. There is a lot of cleanup of dead code to do before a 4.0 release can be considered so I hope to see couch_index_server disappear in the near future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] garrensmith merged pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
garrensmith merged pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-810899027
> I had no heavy QA in mind, just a single-line eunit meck-y check to assert that couch_mrview:validate is called at the right time. (We could even assert couch_index_server:validate wasn't called, if that's a fundamental part of this change.)
Since `couch_index_server:validate/2` is no longer used we could break it and other tests would fail if we partially revert the PR.
```
-module(couch_index_server).
...
validate(_Db, _DDoc) ->
error({assert, ?MODULE, ?FUNCTION_NAME, ?FUNCTION_ARITY, ?LINE}).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r604623332
##########
File path: src/couch_js/src/couch_js.erl
##########
@@ -49,3 +52,15 @@ map_docs(Proc, Docs) ->
end, Results),
{Doc#doc.id, Tupled}
end, Docs)}.
+
+acquire_context() ->
+ Ctx = couch_query_servers:get_os_process(?JS),
+ {ok, Ctx}.
+
+
+release_context(Proc) ->
+ couch_js_query_servers:ret_os_process(Proc).
+
+
+try_compile(Proc, FunName, FunSrc) ->
+ couch_query_servers:try_compile(Proc, map, FunName, FunSrc).
Review comment:
Never mind the idea of passing the `map | reduce` in the context. I can see now that we are using both in `couch_mrview:validate/3`
- map on [line 192](https://github.com/apache/couchdb/pull/3481/files#diff-637f0ee4524b44455ad1fcbfa99fdbdc757c0a0c7a1bd462f4e9b5f51a548dc9L192)
- reduce on [line 206](https://github.com/apache/couchdb/pull/3481/files#diff-637f0ee4524b44455ad1fcbfa99fdbdc757c0a0c7a1bd462f4e9b5f51a548dc9L206)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r604619693
##########
File path: src/couch_eval/src/couch_eval.erl
##########
@@ -86,6 +94,26 @@ release_map_context({ApiMod, Ctx}) ->
map_docs({ApiMod, Ctx}, Docs) ->
ApiMod:map_docs(Ctx, Docs).
+-spec acquire_context(language()) -> {ok, context()} | {error, any()}.
Review comment:
Don't forget to update `-callback acquire_context()....` above as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] garrensmith commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
garrensmith commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r605101626
##########
File path: src/couch/src/couch_db.erl
##########
@@ -895,7 +895,7 @@ validate_doc_update(Db, Doc, GetDiskDocFun) ->
validate_ddoc(Db, DDoc) ->
try
- ok = couch_index_server:validate(Db, couch_doc:with_ejson_body(DDoc))
+ ok = couch_mrview:validate(Db, couch_doc:with_ejson_body(DDoc))
Review comment:
This isn't ideal. But it will be temporary. I want to remove couch_index_server but I want to do it in a few smaller PR's rather than one large one. This first step will get the validation_ddoc using couch_eval instead of couch_index_server and the old javascript proc system.
The next one will be fixes to the replication filtering and the vdu. A final one will be where I can remove couch_index_server and the fix this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r604616537
##########
File path: src/couch_mrview/src/couch_mrview.erl
##########
@@ -203,22 +203,23 @@ validate(DbName, _IsDbPartitioned, DDoc) ->
Msg = ["`", Bad, "` is not a supported reduce function."],
throw({invalid_design_doc, Msg});
({RedName, RedSrc}) ->
- couch_query_servers:try_compile(Proc, reduce, RedName, RedSrc)
+ couch_eval:try_compile(Ctx, RedName, RedSrc)
end, Reds)
end,
{ok, #mrst{
language = Lang,
views = Views
}} = couch_mrview_util:ddoc_to_mrst(DbName, DDoc),
- try Views =/= [] andalso couch_query_servers:get_os_process(Lang) of
+ try Views =/= [] andalso couch_eval:acquire_context(Lang) of
Review comment:
`couch_eval:acquire_context/1` can raise `{invalid_eval_api_mod, Language}` or `{unknown_eval_api_language, Language}. Do we need to handle this on line [224](https://github.com/apache/couchdb/pull/3481/files#diff-637f0ee4524b44455ad1fcbfa99fdbdc757c0a0c7a1bd462f4e9b5f51a548dc9R224)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] garrensmith commented on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
garrensmith commented on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-810359018
@bessbd this functionality is used throughout all the fabric2 and couch_views test. I think it's pretty well covered there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-810364766
> @bessbd this functionality is used throughout all the fabric2 and couch_views test. I think it's pretty well covered there.
That I get. I'd just like to make sure we avoid accidentally (partially) reverting this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r605004027
##########
File path: src/couch/src/couch_db.erl
##########
@@ -895,7 +895,7 @@ validate_doc_update(Db, Doc, GetDiskDocFun) ->
validate_ddoc(Db, DDoc) ->
try
- ok = couch_index_server:validate(Db, couch_doc:with_ejson_body(DDoc))
+ ok = couch_mrview:validate(Db, couch_doc:with_ejson_body(DDoc))
Review comment:
isn't this a layering violation? the index/mrview split was intended to make the m/r view a specific kind of index
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r605004027
##########
File path: src/couch/src/couch_db.erl
##########
@@ -895,7 +895,7 @@ validate_doc_update(Db, Doc, GetDiskDocFun) ->
validate_ddoc(Db, DDoc) ->
try
- ok = couch_index_server:validate(Db, couch_doc:with_ejson_body(DDoc))
+ ok = couch_mrview:validate(Db, couch_doc:with_ejson_body(DDoc))
Review comment:
isn't this a layering violation? the index/mrview split was intended to make the m/r view a specific kind of index
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r604620252
##########
File path: src/couch_js/src/couch_js.erl
##########
@@ -49,3 +52,15 @@ map_docs(Proc, Docs) ->
end, Results),
{Doc#doc.id, Tupled}
end, Docs)}.
+
+acquire_context() ->
+ Ctx = couch_query_servers:get_os_process(?JS),
+ {ok, Ctx}.
+
+
+release_context(Proc) ->
+ couch_js_query_servers:ret_os_process(Proc).
+
+
+try_compile(Proc, FunName, FunSrc) ->
+ couch_query_servers:try_compile(Proc, map, FunName, FunSrc).
Review comment:
I can see that we used to call `couch_query_servers:try_compile(Proc, reduce, RedName, RedSrc)` [here](https://github.com/apache/couchdb/pull/3481/files#diff-637f0ee4524b44455ad1fcbfa99fdbdc757c0a0c7a1bd462f4e9b5f51a548dc9L206)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r604615363
##########
File path: src/couch_eval/src/couch_eval.erl
##########
@@ -86,6 +94,26 @@ release_map_context({ApiMod, Ctx}) ->
map_docs({ApiMod, Ctx}, Docs) ->
ApiMod:map_docs(Ctx, Docs).
+-spec acquire_context(language()) -> {ok, context()} | {error, any()}.
Review comment:
The way the function is written it cannot return `{error, any()}`. I think the correct spec would be:
```
-type error(_Error) :: no_return().
....
spec acquire_context(language()) ->
{ok, context()}
| error({invalid_eval_api_mod, Language :: binary()})
| error({unknown_eval_api_language, Language :: binary()}).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak edited a comment on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak edited a comment on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-810801703
There are very few places where we need to call `acquire_context/1` and `release_context/1` so I am ok with proposed change. However I wanted to make sure you did consider a different option, which is something like the following:
```
-module(couch_eval).
-export([
...
- map_docs/2
+ map_docs/2,
+ with_context/2,
+ try_compile/3
]).
with_context(#{language := Language}, Fun) ->
case acquire_context(Language) of
try acquire_context(Language) of
{ok, Ctx} -> Fun(Ctx)
after
release_context(Ctx)
end.
%% this would be private function
acquire_context(Language) ->
ApiMod = get_api_mod(Language),
{ok, Ctx} = ApiMod:acquire_context(),
{ok, {ApiMod, Ctx}}.
```
Then the caller would look like:
```
-module(couch_mrview).
validate(DbName, _IsDbPartitioned, DDoc) ->
...
Views =/= [] andalso couch_eval:with_context(#{language => Lang}, fun(Ctx) ->
lists:foreach(fun(V) -> ValidateView(Ctx, V) end, Views)
end),
ok.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] garrensmith edited a comment on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
garrensmith edited a comment on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-810370749
@bessbd could you give me an example?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r604616537
##########
File path: src/couch_mrview/src/couch_mrview.erl
##########
@@ -203,22 +203,23 @@ validate(DbName, _IsDbPartitioned, DDoc) ->
Msg = ["`", Bad, "` is not a supported reduce function."],
throw({invalid_design_doc, Msg});
({RedName, RedSrc}) ->
- couch_query_servers:try_compile(Proc, reduce, RedName, RedSrc)
+ couch_eval:try_compile(Ctx, RedName, RedSrc)
end, Reds)
end,
{ok, #mrst{
language = Lang,
views = Views
}} = couch_mrview_util:ddoc_to_mrst(DbName, DDoc),
- try Views =/= [] andalso couch_query_servers:get_os_process(Lang) of
+ try Views =/= [] andalso couch_eval:acquire_context(Lang) of
Review comment:
`couch_eval:acqure_context/1` can raise `{invalid_eval_api_mod, Language}` or `{unknown_eval_api_language, Language}. Do we need to handle this on line [224](https://github.com/apache/couchdb/pull/3481/files#diff-637f0ee4524b44455ad1fcbfa99fdbdc757c0a0c7a1bd462f4e9b5f51a548dc9R224)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-810401262
> @bessbd could you give me an example?
If I understand correctly, the essence of this PR is https://github.com/apache/couchdb/pull/3481/files#diff-eb8693c8aa6baf90841ed8bc66c04cbc525af0bb34368e3201ab4891b954190eL2157-R2157 . I'd just like to make sure that we don't inadvertently undo that change.
I had no heavy QA in mind, just a single-line eunit `meck`-y check to assert that `couch_mrview:validate` is called at the right time. (We could even assert `couch_index_server:validate` wasn't called, if that's a fundamental part of this change.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-810801703
There are very few places where we need to call `acquire_context/1` and `release_context/1` so I am ok with proposed change. However I wanted to make sure you did consider a different option, which is something like the following:
```
-module(couch_eval).
-map_docs/2
+map_docs/2,
+with_context/2,
+try_compile/3
with_context(#{language := Language}, Fun) ->
case acquire_context(Language) of
try acquire_context(Language) of
{ok, Ctx} -> Fun(Ctx)
after
release_context(Ctx)
end.
%% this would be private function
-spec acquire_context(language()) -> {ok, context()} | {error, any()}.
acquire_context(Language) ->
ApiMod = get_api_mod(Language),
{ok, Ctx} = ApiMod:acquire_context(),
{ok, {ApiMod, Ctx}}.
```
Then the caller would look like:
```
-module(couch_mrview).
validate(DbName, _IsDbPartitioned, DDoc) ->
...
Views =/= [] andalso couch_eval:with_context(#{language => Lang}, fun(Ctx) ->
lists:foreach(fun(V) -> ValidateView(Ctx, V) end, Views)
end),
ok.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak edited a comment on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak edited a comment on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-810801703
There are very few places where we need to call `acquire_context/1` and `release_context/1` so I am ok with proposed change. However I wanted to make sure you did consider a different option, which is something like the following:
```
-module(couch_eval).
-map_docs/2
+map_docs/2,
+with_context/2,
+try_compile/3
with_context(#{language := Language}, Fun) ->
case acquire_context(Language) of
try acquire_context(Language) of
{ok, Ctx} -> Fun(Ctx)
after
release_context(Ctx)
end.
%% this would be private function
acquire_context(Language) ->
ApiMod = get_api_mod(Language),
{ok, Ctx} = ApiMod:acquire_context(),
{ok, {ApiMod, Ctx}}.
```
Then the caller would look like:
```
-module(couch_mrview).
validate(DbName, _IsDbPartitioned, DDoc) ->
...
Views =/= [] andalso couch_eval:with_context(#{language => Lang}, fun(Ctx) ->
lists:foreach(fun(V) -> ValidateView(Ctx, V) end, Views)
end),
ok.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r604892755
##########
File path: src/couch_eval/src/couch_eval.erl
##########
@@ -87,6 +102,39 @@ map_docs({ApiMod, Ctx}, Docs) ->
ApiMod:map_docs(Ctx, Docs).
+-spec with_context(with_context_opts(), function()) ->
+ any()
+ | error({invalid_eval_api_mod, Language :: binary()})
+ | error({unknown_eval_api_language, Language :: binary()}).
+with_context(#{language := Language}, Fun) ->
+ case acquire_context(Language) of
Review comment:
The case with a single pattern looks weird. In this particular case we can get away with `{ok, Ctx} = acquire_context(Language)`. Because all errors are returned via either of:
- `erlang:error/1` - get_api_mod
- `erlang:throw/1` - `couch_query_servers:get_os_process/1`
##########
File path: src/couch_eval/src/couch_eval.erl
##########
@@ -86,6 +94,26 @@ release_map_context({ApiMod, Ctx}) ->
map_docs({ApiMod, Ctx}, Docs) ->
ApiMod:map_docs(Ctx, Docs).
+-spec acquire_context(language()) -> {ok, context()} | {error, any()}.
Review comment:
> Don't forget to update -callback acquire_context().... above as well.
Never mind it doesn't apply to the callback.
##########
File path: src/couch_js/src/couch_js.erl
##########
@@ -49,3 +52,15 @@ map_docs(Proc, Docs) ->
end, Results),
{Doc#doc.id, Tupled}
end, Docs)}.
+
+acquire_context() ->
+ Ctx = couch_query_servers:get_os_process(?JS),
+ {ok, Ctx}.
+
+
+release_context(Proc) ->
+ couch_js_query_servers:ret_os_process(Proc).
Review comment:
Should it be `couch_query_servers:ret_os_process(Proc).`?
##########
File path: src/couch_mrview/src/couch_mrview.erl
##########
@@ -203,27 +203,18 @@ validate(DbName, _IsDbPartitioned, DDoc) ->
Msg = ["`", Bad, "` is not a supported reduce function."],
throw({invalid_design_doc, Msg});
({RedName, RedSrc}) ->
- couch_query_servers:try_compile(Proc, reduce, RedName, RedSrc)
+ couch_eval:try_compile(Ctx, RedName, RedSrc)
end, Reds)
end,
{ok, #mrst{
language = Lang,
views = Views
}} = couch_mrview_util:ddoc_to_mrst(DbName, DDoc),
- try Views =/= [] andalso couch_query_servers:get_os_process(Lang) of
- false ->
- ok;
- Proc ->
- try
- lists:foreach(fun(V) -> ValidateView(Proc, V) end, Views)
- after
- couch_query_servers:ret_os_process(Proc)
- end
- catch {unknown_query_language, _Lang} ->
- %% Allow users to save ddocs written in unknown languages
- ok
- end.
+ Views =/= [] andalso couch_eval:with_context(#{language => Lang}, fun (Ctx) ->
Review comment:
Because currently the way it is written we wouldn't allow incorrect language.
##########
File path: src/couch_js/src/couch_js.erl
##########
@@ -49,3 +52,15 @@ map_docs(Proc, Docs) ->
end, Results),
{Doc#doc.id, Tupled}
end, Docs)}.
+
+acquire_context() ->
+ Ctx = couch_query_servers:get_os_process(?JS),
+ {ok, Ctx}.
+
+
+release_context(Proc) ->
+ couch_js_query_servers:ret_os_process(Proc).
+
+
+try_compile(Proc, FunName, FunSrc) ->
+ couch_query_servers:try_compile(Proc, map, FunName, FunSrc).
Review comment:
Can you explain why it is ok to always call `map`?
##########
File path: src/couch_mrview/src/couch_mrview.erl
##########
@@ -203,27 +203,18 @@ validate(DbName, _IsDbPartitioned, DDoc) ->
Msg = ["`", Bad, "` is not a supported reduce function."],
throw({invalid_design_doc, Msg});
({RedName, RedSrc}) ->
- couch_query_servers:try_compile(Proc, reduce, RedName, RedSrc)
+ couch_eval:try_compile(Ctx, RedName, RedSrc)
end, Reds)
end,
{ok, #mrst{
language = Lang,
views = Views
}} = couch_mrview_util:ddoc_to_mrst(DbName, DDoc),
- try Views =/= [] andalso couch_query_servers:get_os_process(Lang) of
- false ->
- ok;
- Proc ->
- try
- lists:foreach(fun(V) -> ValidateView(Proc, V) end, Views)
- after
- couch_query_servers:ret_os_process(Proc)
- end
- catch {unknown_query_language, _Lang} ->
- %% Allow users to save ddocs written in unknown languages
- ok
- end.
+ Views =/= [] andalso couch_eval:with_context(#{language => Lang}, fun (Ctx) ->
Review comment:
Do we want to change the behavior and disallow users to save documents with incorrect language?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r604615363
##########
File path: src/couch_eval/src/couch_eval.erl
##########
@@ -86,6 +94,26 @@ release_map_context({ApiMod, Ctx}) ->
map_docs({ApiMod, Ctx}, Docs) ->
ApiMod:map_docs(Ctx, Docs).
+-spec acquire_context(language()) -> {ok, context()} | {error, any()}.
Review comment:
The way function is written it cannot return `{error, any()}`. I think the correct spec would be:
```
-type error(_Error) :: no_return().
....
spec acquire_context(language()) ->
{ok, context()}
| error({invalid_eval_api_mod, Language :: binary()})
| error({unknown_eval_api_language, Language :: binary()}).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] garrensmith commented on pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
garrensmith commented on pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#issuecomment-810370749
@bessbd could you give me an example.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3481: Validate ddoc uses couch_eval
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3481:
URL: https://github.com/apache/couchdb/pull/3481#discussion_r604620772
##########
File path: src/couch_js/src/couch_js.erl
##########
@@ -49,3 +52,15 @@ map_docs(Proc, Docs) ->
end, Results),
{Doc#doc.id, Tupled}
end, Docs)}.
+
+acquire_context() ->
+ Ctx = couch_query_servers:get_os_process(?JS),
+ {ok, Ctx}.
+
+
+release_context(Proc) ->
+ couch_js_query_servers:ret_os_process(Proc).
+
+
+try_compile(Proc, FunName, FunSrc) ->
+ couch_query_servers:try_compile(Proc, map, FunName, FunSrc).
Review comment:
Looks like we either need to pass it explicitly as try_compile argument or have it as part of a context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org