From 5c2a68076f7263a127e53c58b2fb281a59d1c580 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Mon, 27 May 2024 12:31:19 +0200 Subject: [PATCH 1/5] fix(bridge-v2): report descriptive error on invalid update request Before this commit, generic validation errors were reported as union mismatches of _all_ of the bridge schemas. After this commit, specific schema is chosen before validation. --- apps/emqx_bridge/src/emqx_bridge_v2_api.erl | 19 +++++++++++- .../src/schema/emqx_bridge_v2_schema.erl | 21 ++++++++++++++ .../emqx_bridge_s3_aggreg_upload_SUITE.erl | 29 +++++++++++++++++-- .../src/emqx_dashboard_swagger.erl | 10 +++++-- 4 files changed, 74 insertions(+), 5 deletions(-) diff --git a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl index 56b2cb4ed..b06424ea2 100644 --- a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl @@ -96,7 +96,7 @@ namespace() -> "actions_and_sources". api_spec() -> - emqx_dashboard_swagger:spec(?MODULE, #{check_schema => true}). + emqx_dashboard_swagger:spec(?MODULE, #{check_schema => fun check_api_schema/2}). paths() -> [ @@ -656,6 +656,23 @@ schema("/source_types") -> } }. +%%------------------------------------------------------------------------------ + +check_api_schema(Request, ReqMeta = #{path := Path = "/actions/:id", method := put}) -> + Spec = maps:get(put, schema(Path)), + BridgeId = emqx_utils_maps:deep_get([bindings, id], Request), + try emqx_bridge_resource:parse_bridge_id(BridgeId, #{atom_name => false}) of + {BridgeType, _Name} -> + Schema = emqx_bridge_v2_schema:action_api_schema("put", BridgeType), + SpecRefined = Spec#{'requestBody' => Schema}, + emqx_dashboard_swagger:filter_check_request(Request, ReqMeta#{apispec => SpecRefined}) + catch + throw:#{reason := Reason} -> + ?NOT_FOUND(<<"Invalid bridge ID, ", Reason/binary>>) + end; +check_api_schema(Request, ReqMeta) -> + emqx_dashboard_swagger:filter_check_request(Request, ReqMeta). + %%------------------------------------------------------------------------------ %% Thin Handlers %%------------------------------------------------------------------------------ diff --git a/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl b/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl index caec1f53c..d0b053467 100644 --- a/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl +++ b/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl @@ -31,6 +31,7 @@ actions_get_response/0, actions_put_request/0, actions_post_request/0, + action_api_schema/2, actions_examples/1, action_values/4 ]). @@ -100,6 +101,15 @@ actions_api_schema(Method) -> APISchemas = ?MODULE:registered_actions_api_schemas(Method), hoconsc:union(bridge_api_union(APISchemas)). +action_api_schema(Method, BridgeV2Type) -> + APISchemas = ?MODULE:registered_actions_api_schemas(Method), + case lists:keyfind(atom_to_binary(BridgeV2Type), 1, APISchemas) of + {_, SchemaRef} -> + hoconsc:mk(SchemaRef); + false -> + unknown_bridge_schema(BridgeV2Type) + end. + registered_actions_api_schemas(Method) -> RegisteredSchemas = emqx_action_info:registered_schema_modules_actions(), [ @@ -231,6 +241,17 @@ bridge_api_union(Refs) -> end end. +-dialyzer({nowarn_function, [unknown_bridge_schema/1]}). +unknown_bridge_schema(BridgeV2Type) -> + hoconsc:mk(typerefl:any(), #{ + validator => fun(_) -> + throw(#{ + value => BridgeV2Type, + reason => <<"unknown bridge type">> + }) + end + }). + -spec method_values(action | source, http_method(), atom()) -> schema_example_map(). method_values(Kind, post, Type) -> KindBin = atom_to_binary(Kind), diff --git a/apps/emqx_bridge_s3/test/emqx_bridge_s3_aggreg_upload_SUITE.erl b/apps/emqx_bridge_s3/test/emqx_bridge_s3_aggreg_upload_SUITE.erl index 09cf12329..b7c17bbaa 100644 --- a/apps/emqx_bridge_s3/test/emqx_bridge_s3_aggreg_upload_SUITE.erl +++ b/apps/emqx_bridge_s3/test/emqx_bridge_s3_aggreg_upload_SUITE.erl @@ -156,12 +156,15 @@ t_create_via_http(Config) -> t_on_get_status(Config) -> emqx_bridge_v2_testlib:t_on_get_status(Config, #{}). -t_invalid_config(Config) -> +t_create_invalid_config(Config) -> ?assertMatch( {error, {_Status, _, #{ <<"code">> := <<"BAD_REQUEST">>, - <<"message">> := #{<<"kind">> := <<"validation_error">>} + <<"message">> := #{ + <<"kind">> := <<"validation_error">>, + <<"reason">> := <<"Inconsistent 'min_part_size'", _/bytes>> + } }}}, emqx_bridge_v2_testlib:create_bridge_api( Config, @@ -174,6 +177,28 @@ t_invalid_config(Config) -> ) ). +t_update_invalid_config(Config) -> + ?assertMatch({ok, _Bridge}, emqx_bridge_v2_testlib:create_bridge(Config)), + ?assertMatch( + {error, + {_Status, _, #{ + <<"code">> := <<"BAD_REQUEST">>, + <<"message">> := #{ + <<"kind">> := <<"validation_error">>, + <<"reason">> := <<"Inconsistent 'min_part_size'", _/bytes>> + } + }}}, + emqx_bridge_v2_testlib:update_bridge_api( + Config, + _Overrides = #{ + <<"parameters">> => #{ + <<"min_part_size">> => <<"5GB">>, + <<"max_part_size">> => <<"100MB">> + } + } + ) + ). + t_aggreg_upload(Config) -> Bucket = ?config(s3_bucket, Config), BridgeName = ?config(bridge_name, Config), diff --git a/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl b/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl index 4ada5994c..29623bfba 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl @@ -335,8 +335,8 @@ filter_check_request_and_translate_body(Request, RequestMeta) -> filter_check_request(Request, RequestMeta) -> translate_req(Request, RequestMeta, fun check_only/3). -translate_req(Request, #{module := Module, path := Path, method := Method}, CheckFun) -> - #{Method := Spec} = apply(Module, schema, [Path]), +translate_req(Request, ReqMeta = #{module := Module}, CheckFun) -> + Spec = find_req_apispec(ReqMeta), try Params = maps:get(parameters, Spec, []), Body = maps:get('requestBody', Spec, []), @@ -349,6 +349,12 @@ translate_req(Request, #{module := Module, path := Path, method := Method}, Chec {400, 'BAD_REQUEST', Msg} end. +find_req_apispec(#{apispec := Spec}) -> + Spec; +find_req_apispec(#{module := Module, path := Path, method := Method}) -> + #{Method := Spec} = apply(Module, schema, [Path]), + Spec. + check_and_translate(Schema, Map, Opts) -> hocon_tconf:check_plain(Schema, Map, Opts). From ed7c29ec26d96e5682ab81613b9b97611a6989f3 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Mon, 27 May 2024 16:57:45 +0200 Subject: [PATCH 2/5] fix(api): mention spec override in "request metadata" type --- apps/emqx_dashboard/src/emqx_dashboard_swagger.erl | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl b/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl index 29623bfba..a6005254a 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl @@ -91,7 +91,14 @@ -define(DEFAULT_ROW, 100). -type request() :: #{bindings => map(), query_string => map(), body => map()}. --type request_meta() :: #{module => module(), path => string(), method => atom()}. +-type request_meta() :: #{ + module := module(), + path := string(), + method := atom(), + %% API Operation specification override. + %% Takes precedence over the API specification defined in the module. + apispec => map() +}. %% More exact types are defined in minirest.hrl, but we don't want to include it %% because it defines a lot of types and they may clash with the types declared locally. From 08d88ea814257e19497fb4f0f7033e60f3861d84 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Mon, 27 May 2024 17:14:37 +0200 Subject: [PATCH 3/5] feat(bridge-api): improve error messages for Update Source API --- apps/emqx_bridge/src/emqx_bridge_v2_api.erl | 25 ++++++++++++++++--- .../src/schema/emqx_bridge_v2_schema.erl | 22 +++++++++++++--- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl index b06424ea2..8ba2ef487 100644 --- a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl @@ -658,21 +658,38 @@ schema("/source_types") -> %%------------------------------------------------------------------------------ -check_api_schema(Request, ReqMeta = #{path := Path = "/actions/:id", method := put}) -> - Spec = maps:get(put, schema(Path)), +check_api_schema(Request, ReqMeta = #{path := "/actions/:id", method := put}) -> BridgeId = emqx_utils_maps:deep_get([bindings, id], Request), try emqx_bridge_resource:parse_bridge_id(BridgeId, #{atom_name => false}) of + %% NOTE + %% Bridge type is known, refine the API schema to get more specific error messages. {BridgeType, _Name} -> Schema = emqx_bridge_v2_schema:action_api_schema("put", BridgeType), - SpecRefined = Spec#{'requestBody' => Schema}, - emqx_dashboard_swagger:filter_check_request(Request, ReqMeta#{apispec => SpecRefined}) + emqx_dashboard_swagger:filter_check_request(Request, refine_api_schema(Schema, ReqMeta)) catch throw:#{reason := Reason} -> ?NOT_FOUND(<<"Invalid bridge ID, ", Reason/binary>>) end; +check_api_schema(Request, ReqMeta = #{path := "/sources/:id", method := put}) -> + SourceId = emqx_utils_maps:deep_get([bindings, id], Request), + try emqx_bridge_resource:parse_bridge_id(SourceId, #{atom_name => false}) of + %% NOTE + %% Source type is known, refine the API schema to get more specific error messages. + {BridgeType, _Name} -> + Schema = emqx_bridge_v2_schema:source_api_schema("put", BridgeType), + emqx_dashboard_swagger:filter_check_request(Request, refine_api_schema(Schema, ReqMeta)) + catch + throw:#{reason := Reason} -> + ?NOT_FOUND(<<"Invalid source ID, ", Reason/binary>>) + end; check_api_schema(Request, ReqMeta) -> emqx_dashboard_swagger:filter_check_request(Request, ReqMeta). +refine_api_schema(Schema, ReqMeta = #{path := Path, method := Method}) -> + Spec = maps:get(Method, schema(Path)), + SpecRefined = Spec#{'requestBody' => Schema}, + ReqMeta#{apispec => SpecRefined}. + %%------------------------------------------------------------------------------ %% Thin Handlers %%------------------------------------------------------------------------------ diff --git a/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl b/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl index d0b053467..d7270d206 100644 --- a/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl +++ b/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl @@ -40,6 +40,7 @@ sources_get_response/0, sources_put_request/0, sources_post_request/0, + source_api_schema/2, sources_examples/1, source_values/4 ]). @@ -169,6 +170,15 @@ sources_api_schema(Method) -> APISchemas = ?MODULE:registered_sources_api_schemas(Method), hoconsc:union(bridge_api_union(APISchemas)). +source_api_schema(Method, SourceType) -> + APISchemas = ?MODULE:registered_sources_api_schemas(Method), + case lists:keyfind(atom_to_binary(SourceType), 1, APISchemas) of + {_, SchemaRef} -> + hoconsc:mk(SchemaRef); + false -> + unknown_source_schema(SourceType) + end. + registered_sources_api_schemas(Method) -> RegisteredSchemas = emqx_action_info:registered_schema_modules_sources(), [ @@ -241,13 +251,19 @@ bridge_api_union(Refs) -> end end. --dialyzer({nowarn_function, [unknown_bridge_schema/1]}). unknown_bridge_schema(BridgeV2Type) -> + erroneous_value_schema(BridgeV2Type, <<"unknown bridge type">>). + +unknown_source_schema(SourceType) -> + erroneous_value_schema(SourceType, <<"unknown source type">>). + +-dialyzer({nowarn_function, [erroneous_value_schema/2]}). +erroneous_value_schema(Value, Reason) -> hoconsc:mk(typerefl:any(), #{ validator => fun(_) -> throw(#{ - value => BridgeV2Type, - reason => <<"unknown bridge type">> + value => Value, + reason => Reason }) end }). From 663f4fd39f24dfda814e027e7d320cd24355704e Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Mon, 27 May 2024 17:23:08 +0200 Subject: [PATCH 4/5] chore: bump application versions --- apps/emqx_bridge/src/emqx_bridge.app.src | 2 +- apps/emqx_dashboard/src/emqx_dashboard.app.src | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/emqx_bridge/src/emqx_bridge.app.src b/apps/emqx_bridge/src/emqx_bridge.app.src index a4e2d6fd1..8abfb075e 100644 --- a/apps/emqx_bridge/src/emqx_bridge.app.src +++ b/apps/emqx_bridge/src/emqx_bridge.app.src @@ -1,7 +1,7 @@ %% -*- mode: erlang -*- {application, emqx_bridge, [ {description, "EMQX bridges"}, - {vsn, "0.2.0"}, + {vsn, "0.2.1"}, {registered, [emqx_bridge_sup]}, {mod, {emqx_bridge_app, []}}, {applications, [ diff --git a/apps/emqx_dashboard/src/emqx_dashboard.app.src b/apps/emqx_dashboard/src/emqx_dashboard.app.src index 7feefa04e..427f2958a 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard.app.src +++ b/apps/emqx_dashboard/src/emqx_dashboard.app.src @@ -2,7 +2,7 @@ {application, emqx_dashboard, [ {description, "EMQX Web Dashboard"}, % strict semver, bump manually! - {vsn, "5.1.0"}, + {vsn, "5.1.1"}, {modules, []}, {registered, [emqx_dashboard_sup]}, {applications, [ From c04aaad0a40f38096faa50d6b431f16571bfc1a8 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Tue, 28 May 2024 10:31:14 +0200 Subject: [PATCH 5/5] chore(bridge-v2): leave comment describing need for `nowarn_function` --- apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl b/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl index d7270d206..6dbad456b 100644 --- a/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl +++ b/apps/emqx_bridge/src/schema/emqx_bridge_v2_schema.erl @@ -257,6 +257,8 @@ unknown_bridge_schema(BridgeV2Type) -> unknown_source_schema(SourceType) -> erroneous_value_schema(SourceType, <<"unknown source type">>). +%% @doc Construct a schema that always emits validation error. +%% We need to silence dialyzer because inner anonymous function always throws. -dialyzer({nowarn_function, [erroneous_value_schema/2]}). erroneous_value_schema(Value, Reason) -> hoconsc:mk(typerefl:any(), #{