Validate ddoc uses couch_eval by garrensmith · Pull Request #3481 · apache/couchdb

garrensmith · 2021-03-30T12:51:21Z

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

Checklist

Code is written and works correctly
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

bessbd

Thank you for this PR, @garrensmith .
The change looks good to me on a first read.
Do you think there's an easy way to add a test for this?

garrensmith · 2021-03-30T15:32:59Z

@bessbd this functionality is used throughout all the fabric2 and couch_views test. I think it's pretty well covered there.

bessbd · 2021-03-30T15:38:18Z

@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.

garrensmith · 2021-03-30T15:43:46Z

@bessbd could you give me an example?

bessbd · 2021-03-30T16:25:44Z

@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.)

iilyak · 2021-03-31T06:01:22Z

src/couch_eval/src/couch_eval.erl

 map_docs({ApiMod, Ctx}, Docs) ->
    ApiMod:map_docs(Ctx, Docs).

+-spec acquire_context(language()) -> {ok, context()} | {error, any()}.


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()}).

Don't forget to update -callback acquire_context().... above as well.

Don't forget to update -callback acquire_context().... above as well.

Never mind it doesn't apply to the callback.

iilyak · 2021-03-31T06:04:13Z

src/couch_mrview/src/couch_mrview.erl

    }} = 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


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?

I don't think so. I'm happy for that error to bubble up.

iilyak · 2021-03-31T06:10:38Z

src/couch_js/src/couch_js.erl

+
+
+try_compile(Proc, FunName, FunSrc) ->
+    couch_query_servers:try_compile(Proc, map, FunName, FunSrc).


Is it always would be a map in a second argument?

I can see that we used to call couch_query_servers:try_compile(Proc, reduce, RedName, RedSrc) here

Looks like we either need to pass it explicitly as try_compile argument or have it as part of a context.

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

reduce on line 206

Can you explain why it is ok to always call map?

iilyak · 2021-03-31T06:15:32Z

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.

iilyak · 2021-03-31T08:55:54Z

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}).

bessbd · 2021-03-31T12:05:17Z

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?

garrensmith · 2021-03-31T12:59:45Z

@bessbd right. I've deleted the function. I think that is the best approach.

rnewson · 2021-03-31T15:34:19Z

src/couch/src/couch_db.erl

 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))


isn't this a layering violation? the index/mrview split was intended to make the m/r view a specific kind of index

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.

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.

iilyak · 2021-03-31T08:33:53Z

src/couch_eval/src/couch_eval.erl

 map_docs({ApiMod, Ctx}, Docs) ->
    ApiMod:map_docs(Ctx, Docs).

+-spec acquire_context(language()) -> {ok, context()} | {error, any()}.


Don't forget to update -callback acquire_context().... above as well.

Never mind it doesn't apply to the callback.

iilyak · 2021-03-31T13:19:00Z

src/couch_js/src/couch_js.erl

+
+
+release_context(Proc) ->
+    couch_js_query_servers:ret_os_process(Proc).


Should it be couch_query_servers:ret_os_process(Proc).?

iilyak · 2021-03-31T13:23:30Z

src/couch_eval/src/couch_eval.erl

+    | error({invalid_eval_api_mod, Language :: binary()})
+    | error({unknown_eval_api_language, Language :: binary()}).
+with_context(#{language := Language}, Fun) ->
+    case acquire_context(Language) of


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

iilyak · 2021-03-31T13:24:16Z

src/couch_js/src/couch_js.erl

+
+
+try_compile(Proc, FunName, FunSrc) ->
+    couch_query_servers:try_compile(Proc, map, FunName, FunSrc).


Can you explain why it is ok to always call map?

iilyak · 2021-03-31T13:25:18Z

src/couch_mrview/src/couch_mrview.erl

-    %% Allow users to save ddocs written in unknown languages
-        ok
-    end.
+    Views =/= [] andalso couch_eval:with_context(#{language => Lang}, fun (Ctx) ->


Do we want to change the behavior and disallow users to save documents with incorrect language?

Because currently the way it is written we wouldn't allow incorrect language.

iilyak

+1 from me.

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.

This makes it that the only way to validate a design doc is through couch_mrview:validate_ddoc so that the correct couch_eval will be used.

garrensmith force-pushed the validate-ddoc branch 3 times, most recently from fbbacc2 to 4c103c9 Compare March 30, 2021 14:37

bessbd reviewed Mar 30, 2021

View reviewed changes

iilyak reviewed Mar 31, 2021

View reviewed changes

garrensmith force-pushed the validate-ddoc branch from 4c103c9 to 820e3b3 Compare March 31, 2021 08:41

garrensmith force-pushed the validate-ddoc branch 2 times, most recently from 97818fc to 9765c09 Compare March 31, 2021 13:32

rnewson reviewed Mar 31, 2021

View reviewed changes

iilyak requested changes Apr 1, 2021

View reviewed changes

iilyak approved these changes Apr 2, 2021

View reviewed changes

garrensmith added 2 commits April 6, 2021 10:29

Validate ddoc uses couch_eval

7b64af6

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.

remove couch_index_server:validate

d568be8

This makes it that the only way to validate a design doc is through couch_mrview:validate_ddoc so that the correct couch_eval will be used.

garrensmith force-pushed the validate-ddoc branch from ec1bf5b to d568be8 Compare April 6, 2021 08:30

garrensmith merged commit 8843083 into main Apr 6, 2021

iilyak mentioned this pull request May 4, 2021

Add clause for unknown_eval_api_language #3540

Merged

4 tasks

nickva deleted the validate-ddoc branch December 17, 2022 05:22



		try_compile(Proc, FunName, FunSrc) ->
		couch_query_servers:try_compile(Proc, map, FunName, FunSrc).



		release_context(Proc) ->
		couch_js_query_servers:ret_os_process(Proc).

Conversation

garrensmith commented Mar 30, 2021

Overview

Testing recommendations

Related Issues or Pull Requests

Checklist

Uh oh!

bessbd left a comment

Choose a reason for hiding this comment

Uh oh!

garrensmith commented Mar 30, 2021

Uh oh!

bessbd commented Mar 30, 2021

Uh oh!

garrensmith commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bessbd commented Mar 30, 2021

Uh oh!

iilyak Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iilyak Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iilyak commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iilyak commented Mar 31, 2021

Uh oh!

bessbd commented Mar 31, 2021

Uh oh!

garrensmith commented Mar 31, 2021

Uh oh!

rnewson Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iilyak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

garrensmith commented Mar 30, 2021 •

edited

Loading

iilyak Mar 31, 2021 •

edited

Loading

iilyak Mar 31, 2021 •

edited

Loading

iilyak commented Mar 31, 2021 •

edited

Loading

rnewson Mar 31, 2021 •

edited

Loading