[GitHub] [couchdb] iilyak commented on a change in pull request #3481: Validate ddoc uses couch_eval

[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