Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1121 rule bridge properties #9398

Merged
merged 2 commits into from Nov 23, 2022
Merged

1121 rule bridge properties #9398

merged 2 commits into from Nov 23, 2022

Conversation

zmstone
Copy link
Member

@zmstone zmstone commented Nov 21, 2022

this is a follow up for #9240

@zmstone zmstone marked this pull request as draft November 21, 2022 20:33
@@ -110,8 +110,9 @@ republish(
Payload = format_msg(PayloadTks, Selected),
QoS = replace_simple_var(QoSTks, Selected, 0),
Retain = replace_simple_var(RetainTks, Selected, false),
PubProps = maps:get(pub_props, OriginalMsg, #{}),
Copy link
Member Author

@zmstone zmstone Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was originally (#9240) taken from Selected's pub_props atom filed,
which seemed to be wrong because selected element keys are of binary format. (i.e. <<"pub_props">>).

I tried to change to take from the original message, it seemed to work, however it also affects
other events like message.dropped, message.acked, in those events, a user may not necessary
want to keep the properties.

For example, if someone just want to republish a message.dropped event, but only interested in the topic name.

Fixed to pick <<"pub_props">> value from Selected, but try to convert properties to atom keys,
this requires user to explicitly SELECT pub_props (see test case examples).

The "proper" approach is perhaps to treat message properties like qos, retain flag, and payload,
meaning to add another argument here:

fields("republish_args") ->
[
{topic,
?HOCON(
binary(),
#{
desc => ?DESC("republish_args_topic"),
required => true,
example => <<"a/1">>
}
)},
{qos,
?HOCON(
qos(),
#{
desc => ?DESC("republish_args_qos"),
default => <<"${qos}">>,
example => <<"${qos}">>
}
)},
{retain,
?HOCON(
hoconsc:union([boolean(), binary()]),
#{
desc => ?DESC("republish_args_retain"),
default => <<"${retain}">>,
example => <<"${retain}">>
}
)},
{payload,
?HOCON(
binary(),
#{
desc => ?DESC("republish_args_payload"),
default => <<"${payload}">>,
example => <<"${payload}">>
}
)}
].

@terry-xiaoyu @kjellwinblad @thalesmg wdyt ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need a 'properties' config in this case.

  • If the 'properties' is not configured and it's a PUBLISH message, we republish the message using the original properties.
  • If the 'properties' is not configured and it's an event like $events/client_connected, we republish the message without any property.
  • If the 'properties' is configured, then we republish using the configured properties.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "proper" approach is perhaps to treat message properties like qos, retain flag, and payload,
meaning to add another argument here:

Based on my limited understanding I also think this would be the the proper approach

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my very limited knowledge about this, it seems like the props should indeed be a new config; otherwise, it's not obvious that a property should be dropped or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your suggestions.
I have added a user_properties republish action argument with default template ${user_properties}.

Decided NOT to fill user_properties with original message's pub_props.'User-Property',
because the code is shared for both message republish and for message events such as message.acked and message.dropped.

In order to republish message with User-Property, a user will have to explicitly select a variable,
or configure with the special value ${pub_props.'User-Property'}.

@zmstone zmstone force-pushed the 1121-rule-bridge-properties branch 2 times, most recently from 4945bd4 to 9e45ad1 Compare November 21, 2022 21:34
Copy link
Contributor

@kjellwinblad kjellwinblad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me (just a few minor comments about sleeps in test cases).

apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl Outdated Show resolved Hide resolved
apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl Outdated Show resolved Hide resolved
apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl Outdated Show resolved Hide resolved
@zmstone zmstone force-pushed the 1121-rule-bridge-properties branch 2 times, most recently from 5f9b7f0 to f70f18d Compare November 22, 2022 18:49
Comment on lines -128 to -131
Topic = emqx_plugin_libs_rule:proc_tmpl(TopicTks, Selected),
Payload = format_msg(PayloadTks, Selected),
QoS = replace_simple_var(QoSTks, Selected, 0),
Retain = replace_simple_var(RetainTks, Selected, false),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these three lines duplicates line 113-116, so i merged the two function clauses.
the only special part is the flags field.

@zmstone zmstone force-pushed the 1121-rule-bridge-properties branch 2 times, most recently from 37d187e to 7a309c8 Compare November 22, 2022 18:56
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 3526362825

  • 20 of 24 (83.33%) changed or added relevant lines in 3 files are covered.
  • 67 unchanged lines in 16 files lost coverage.
  • Overall coverage decreased (-0.1%) to 78.132%

Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/emqx/src/emqx_misc.erl 5 7 71.43%
apps/emqx_rule_engine/src/emqx_rule_actions.erl 12 14 85.71%
Files with Coverage Reduction New Missed Lines %
apps/emqx/src/emqx_olp.erl 1 77.78%
apps/emqx/src/emqx_message.erl 1 94.83%
apps/emqx/src/emqx_authentication_config.erl 1 83.62%
apps/emqx_management/src/emqx_mgmt_api_trace.erl 1 80.41%
apps/emqx/src/emqx_connection.erl 1 87.64%
apps/emqx/src/emqx_router_helper.erl 1 86.27%
apps/emqx_conf/src/emqx_conf_schema.erl 1 92.31%
apps/emqx_gateway/src/mqttsn/emqx_sn_channel.erl 1 72.92%
apps/emqx/src/emqx_channel.erl 2 87.38%
apps/emqx_authn/src/emqx_authn.erl 2 75.0%
Totals Coverage Status
Change from base Build 3523358423: -0.1%
Covered Lines: 20955
Relevant Lines: 26820

💛 - Coveralls

@zmstone zmstone marked this pull request as ready for review November 23, 2022 08:51
@zmstone zmstone merged commit cded5fc into master Nov 23, 2022
@zmstone zmstone deleted the 1121-rule-bridge-properties branch November 23, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants