nickva commented on code in PR #5858:
URL: https://github.com/apache/couchdb/pull/5858#discussion_r3159169928
##########
src/mango/src/mango_selector.erl:
##########
@@ -548,22 +672,136 @@ match({[{<<"$", _/binary>> = Op, _}]}, _, _) ->
% We need to traverse value to find field. The call to
% mango_doc:get_field/2 may return either not_found or
% bad_path in which case matching fails.
-match({[{Field, Cond}]}, Value, Cmp) ->
+match({[{Field, Cond}]}, Value, #ctx{verbose = Verb, path = Path} = Ctx) ->
+ InnerPath = extend_path(Field, Path),
+ InnerCtx = Ctx#ctx{path = InnerPath},
case mango_doc:get_field(Value, Field) of
not_found when Cond == {[{<<"$exists">>, false}]} ->
- true;
+ case Verb of
+ true -> [];
+ false -> true
+ end;
not_found ->
- false;
+ case Verb of
+ true -> [#failure{op = field, type = not_found, ctx =
InnerCtx}];
+ false -> false
+ end;
bad_path ->
- false;
+ case Verb of
+ true -> [#failure{op = field, type = bad_path, ctx =
InnerCtx}];
+ false -> false
+ end;
SubValue when Field == <<"_id">> ->
- match(Cond, SubValue, fun mango_json:cmp_raw/2);
+ match(Cond, SubValue, InnerCtx#ctx{cmp = fun
mango_json:cmp_raw/2});
SubValue ->
- match(Cond, SubValue, Cmp)
+ match(Cond, SubValue, InnerCtx)
end;
-match({[_, _ | _] = _Props} = Sel, _Value, _Cmp) ->
+match({[_, _ | _] = _Props} = Sel, _Value, _Ctx) ->
error({unnormalized_selector, Sel}).
+extend_path(Field, Path) when is_binary(Field) ->
+ [Field | Path];
+extend_path(Field, Path) when is_list(Field) ->
+ lists:foldl(fun(F, Acc) -> [F | Acc] end, Path, Field).
+
+match_with_failure(Expr, Value, Op, Params, #ctx{negate = Neg} = Ctx) ->
+ case not match(Expr, Value, Ctx#ctx{verbose = false}) of
+ Neg -> [];
+ _ -> [#failure{op = Op, params = Params, ctx = Ctx}]
+ end.
+
+compare(_, _, #ctx{verbose = false}, Cond) ->
+ Cond;
+compare(Op, Arg, #ctx{negate = Neg} = Ctx, Cond) ->
+ case not Cond of
+ Neg -> [];
+ _ -> [#failure{op = Op, params = [Arg], ctx = Ctx}]
+ end.
+
+format_failure(#failure{op = Op, type = Type, params = Params, ctx = Ctx}) ->
+ Path = format_path(Ctx#ctx.path),
+ Msg = format_op(Op, Ctx#ctx.negate, Type, Params),
+ {[{<<"path">>, Path}, {<<"message">>, list_to_binary(Msg)}]}.
+
+format_op(Op, _, empty_list, _) ->
+ io_lib:format("operator $~p was invoked with an empty list", [Op]);
+format_op(Op, _, bad_value, [Value]) ->
+ io_lib:format("operator $~p was invoked with a bad value: ~s", [Op,
jiffy:encode(Value)]);
+format_op(_, _, not_found, []) ->
+ io_lib:format("must be present", []);
+format_op(_, _, bad_path, []) ->
+ io_lib:format("used an invalid path", []);
+format_op(eq, false, mismatch, [X]) ->
+ io_lib:format("must be equal to ~s", [jiffy:encode(X)]);
+format_op(eq, true, Type, Params) ->
+ format_op(ne, false, Type, Params);
+format_op(ne, false, mismatch, [X]) ->
+ io_lib:format("must not be equal to ~s", [jiffy:encode(X)]);
+format_op(ne, true, Type, Params) ->
+ format_op(eq, false, Type, Params);
+format_op(lt, false, mismatch, [X]) ->
+ io_lib:format("must be less than ~s", [jiffy:encode(X)]);
+format_op(lt, true, Type, Params) ->
+ format_op(gte, false, Type, Params);
+format_op(lte, false, mismatch, [X]) ->
+ io_lib:format("must be less than or equal to ~s", [jiffy:encode(X)]);
+format_op(lte, true, Type, Params) ->
+ format_op(gt, false, Type, Params);
+format_op(gt, false, mismatch, [X]) ->
+ io_lib:format("must be greater than ~s", [jiffy:encode(X)]);
+format_op(gt, true, Type, Params) ->
+ format_op(lte, false, Type, Params);
+format_op(gte, false, mismatch, [X]) ->
+ io_lib:format("must be greater than or equal to ~s", [jiffy:encode(X)]);
+format_op(gte, true, Type, Params) ->
+ format_op(le, false, Type, Params);
Review Comment:
There is no `le` operator it looks like?
Make sure to add tests for all the cases and check coverage.
##########
src/couch/src/couch_query_servers.erl:
##########
@@ -490,6 +490,8 @@ validate_doc_update(Db, DDoc, EditDoc, DiskDoc, Ctx,
SecObj) ->
case Resp of
ok ->
ok;
+ {[{<<"forbidden">>, Message}, {<<"failures">>, Failures}]} ->
+ throw({forbidden, Message, Failures});
Review Comment:
We're changing the forbidden tuple shape. Make sure to check all the place
which handle `{forbidden, _}` and now they may have to handle the triple-arg
version. I noticed fabric_doc_update needs to handle this
src/fabric/src/fabric_doc_update.erl
We should also call out and see if this will affect online cluster upgrades
(new worker nodes throwing it and old coordinator nodes getting function clause
errors). It maybe be fine, just needs an extra careful look at it.
##########
src/mango/src/mango_selector.erl:
##########
@@ -548,22 +672,136 @@ match({[{<<"$", _/binary>> = Op, _}]}, _, _) ->
% We need to traverse value to find field. The call to
% mango_doc:get_field/2 may return either not_found or
% bad_path in which case matching fails.
-match({[{Field, Cond}]}, Value, Cmp) ->
+match({[{Field, Cond}]}, Value, #ctx{verbose = Verb, path = Path} = Ctx) ->
+ InnerPath = extend_path(Field, Path),
+ InnerCtx = Ctx#ctx{path = InnerPath},
case mango_doc:get_field(Value, Field) of
not_found when Cond == {[{<<"$exists">>, false}]} ->
- true;
+ case Verb of
+ true -> [];
+ false -> true
+ end;
not_found ->
- false;
+ case Verb of
+ true -> [#failure{op = field, type = not_found, ctx =
InnerCtx}];
+ false -> false
+ end;
bad_path ->
- false;
+ case Verb of
+ true -> [#failure{op = field, type = bad_path, ctx =
InnerCtx}];
+ false -> false
+ end;
SubValue when Field == <<"_id">> ->
- match(Cond, SubValue, fun mango_json:cmp_raw/2);
+ match(Cond, SubValue, InnerCtx#ctx{cmp = fun
mango_json:cmp_raw/2});
SubValue ->
- match(Cond, SubValue, Cmp)
+ match(Cond, SubValue, InnerCtx)
end;
-match({[_, _ | _] = _Props} = Sel, _Value, _Cmp) ->
+match({[_, _ | _] = _Props} = Sel, _Value, _Ctx) ->
error({unnormalized_selector, Sel}).
+extend_path(Field, Path) when is_binary(Field) ->
+ [Field | Path];
+extend_path(Field, Path) when is_list(Field) ->
+ lists:foldl(fun(F, Acc) -> [F | Acc] end, Path, Field).
+
+match_with_failure(Expr, Value, Op, Params, #ctx{negate = Neg} = Ctx) ->
+ case not match(Expr, Value, Ctx#ctx{verbose = false}) of
+ Neg -> [];
+ _ -> [#failure{op = Op, params = Params, ctx = Ctx}]
+ end.
+
+compare(_, _, #ctx{verbose = false}, Cond) ->
+ Cond;
+compare(Op, Arg, #ctx{negate = Neg} = Ctx, Cond) ->
+ case not Cond of
+ Neg -> [];
+ _ -> [#failure{op = Op, params = [Arg], ctx = Ctx}]
+ end.
+
+format_failure(#failure{op = Op, type = Type, params = Params, ctx = Ctx}) ->
+ Path = format_path(Ctx#ctx.path),
+ Msg = format_op(Op, Ctx#ctx.negate, Type, Params),
+ {[{<<"path">>, Path}, {<<"message">>, list_to_binary(Msg)}]}.
Review Comment:
Should that be iolist_to_binary/1?
##########
src/mango/src/mango_native_proc.erl:
##########
@@ -112,14 +112,30 @@ handle_call({prompt, [<<"ddoc">>, DDocId,
[<<"validate_doc_update">>], Args]}, _
Msg = [<<"validate_doc_update">>, DDocId],
{stop, {invalid_call, Msg}, {invalid_call, Msg}, St};
Selector ->
- [NewDoc, OldDoc, _Ctx, _SecObj] = Args,
- Struct = {[{<<"newDoc">>, NewDoc}, {<<"oldDoc">>, OldDoc}]},
- Reply =
- case mango_selector:match(Selector, Struct) of
- true -> true;
- _ -> {[{<<"forbidden">>, <<"document is not valid">>}]}
- end,
- {reply, Reply, St}
+ case mango_selector:has_allowed_fields(Selector, [<<"newDoc">>,
<<"oldDoc">>]) of
+ false ->
+ Msg =
+ <<"'validate_doc_update' may only contain 'newDoc' and
'oldDoc' as top-level fields">>,
+ {stop, {invalid_call, Msg}, {invalid_call, Msg}, St};
+ true ->
+ [NewDoc, OldDoc, _Ctx, _SecObj] = Args,
+ Struct =
+ case OldDoc of
+ null -> {[{<<"newDoc">>, NewDoc}]};
+ Doc -> {[{<<"newDoc">>, NewDoc}, {<<"oldDoc">>,
Doc}]}
+ end,
+ Reply =
+ case mango_selector:match_failures(Selector, Struct) of
+ [] ->
+ true;
+ Failures ->
+ {[
+ {<<"forbidden">>, <<"forbidden">>},
Review Comment:
This would change the message to `<<"forbidden">>` from `<<"document is not
valid">>`. It does seems a bit redundant as the error is already `forbidden`
##########
src/mango/src/mango_native_proc.erl:
##########
@@ -112,14 +112,30 @@ handle_call({prompt, [<<"ddoc">>, DDocId,
[<<"validate_doc_update">>], Args]}, _
Msg = [<<"validate_doc_update">>, DDocId],
{stop, {invalid_call, Msg}, {invalid_call, Msg}, St};
Selector ->
- [NewDoc, OldDoc, _Ctx, _SecObj] = Args,
- Struct = {[{<<"newDoc">>, NewDoc}, {<<"oldDoc">>, OldDoc}]},
- Reply =
- case mango_selector:match(Selector, Struct) of
- true -> true;
- _ -> {[{<<"forbidden">>, <<"document is not valid">>}]}
- end,
- {reply, Reply, St}
+ case mango_selector:has_allowed_fields(Selector, [<<"newDoc">>,
<<"oldDoc">>]) of
+ false ->
+ Msg =
+ <<"'validate_doc_update' may only contain 'newDoc' and
'oldDoc' as top-level fields">>,
+ {stop, {invalid_call, Msg}, {invalid_call, Msg}, St};
+ true ->
+ [NewDoc, OldDoc, _Ctx, _SecObj] = Args,
+ Struct =
+ case OldDoc of
+ null -> {[{<<"newDoc">>, NewDoc}]};
Review Comment:
Does this mean before we allowed OldDoc = null and now we skip it when it's
null? I guess we haven't made a release so it won't break anything.
##########
src/mango/src/mango_native_proc.erl:
##########
@@ -112,14 +112,30 @@ handle_call({prompt, [<<"ddoc">>, DDocId,
[<<"validate_doc_update">>], Args]}, _
Msg = [<<"validate_doc_update">>, DDocId],
{stop, {invalid_call, Msg}, {invalid_call, Msg}, St};
Selector ->
- [NewDoc, OldDoc, _Ctx, _SecObj] = Args,
- Struct = {[{<<"newDoc">>, NewDoc}, {<<"oldDoc">>, OldDoc}]},
- Reply =
- case mango_selector:match(Selector, Struct) of
- true -> true;
- _ -> {[{<<"forbidden">>, <<"document is not valid">>}]}
- end,
- {reply, Reply, St}
+ case mango_selector:has_allowed_fields(Selector, [<<"newDoc">>,
<<"oldDoc">>]) of
+ false ->
+ Msg =
+ <<"'validate_doc_update' may only contain 'newDoc' and
'oldDoc' as top-level fields">>,
Review Comment:
I wonder when this would fire, would be be on every time we attempt to
insert a doc we'd crash the prompt?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
