You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "lordgamez (via GitHub)" <gi...@apache.org> on 2025/06/23 08:55:27 UTC
[PR] MINIFICPP-2581 Fix EL evaluation in dynamic properties [nifi-minifi-cpp]
lordgamez opened a new pull request, #1983:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1983
https://issues.apache.org/jira/browse/MINIFICPP-2581
-------------------
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2581 Fix EL evaluation in dynamic properties [nifi-minifi-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1983:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1983#discussion_r2162074296
##########
extensions/expression-language/ProcessContextExpr.cpp:
##########
@@ -70,13 +70,17 @@ nonstd::expected<void, std::error_code> ProcessContextExpr::setDynamicProperty(s
std::map<std::string, std::string> ProcessContextExpr::getDynamicProperties(const FlowFile* flow_file) const {
auto dynamic_props = ProcessContextImpl::getDynamicProperties(flow_file);
- for (auto& [dynamic_property_name, dynamic_property_value] : dynamic_props) {
- if (!cached_dynamic_expressions_.contains(dynamic_property_name)) {
+ const expression::Parameters params{this, flow_file};
+ for (auto& [dynamic_property_name, dynamic_property_value]: dynamic_props) {
+ auto cached_dyn_expr_it = cached_dynamic_expressions_.find(dynamic_property_name);
+ if (cached_dyn_expr_it == cached_dynamic_expressions_.end()) {
auto expression = expression::compile(dynamic_property_value);
- expression::Parameters p(this, flow_file);
- dynamic_property_value = expression(p).asString();
- cached_dynamic_expressions_.emplace(std::string{dynamic_property_name}, std::move(expression));
+ const auto [it, success] = cached_dynamic_expressions_.emplace(dynamic_property_name, expression);
+ gsl_Assert(success && "getDynamicProperties: no element with the key existed, yet insertion failed");
Review Comment:
Using `gsl_Assert` for error handling can be disabled in release builds; consider adding explicit error handling or logging to handle insertion failure in production rather than relying solely on assertions.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2581 Fix EL evaluation in dynamic properties [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1983:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1983#discussion_r2166082933
##########
extensions/expression-language/ProcessContextExpr.cpp:
##########
Review Comment:
Good point, I opened a separate jira ticket for the issue to investigate https://issues.apache.org/jira/browse/MINIFICPP-2585
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2581 Fix EL evaluation in dynamic properties [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1983:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1983#discussion_r2161877485
##########
extensions/expression-language/ProcessContextExpr.cpp:
##########
Review Comment:
I've pushed a few changes that don't change behavior, just restructure the code, lmk if you disagree with anything.
Also, I was wondering if access to the ProcessContext is synchronized at all, or are we doing concurrent unsynchronized accesses (i.e. data race) to non-static data members like cached_dynamic_expressions_. That's a separate bug, but similarly very important.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2581 Fix EL evaluation in dynamic properties [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1983:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1983#discussion_r2162386355
##########
extensions/expression-language/ProcessContextExpr.cpp:
##########
@@ -70,13 +70,17 @@ nonstd::expected<void, std::error_code> ProcessContextExpr::setDynamicProperty(s
std::map<std::string, std::string> ProcessContextExpr::getDynamicProperties(const FlowFile* flow_file) const {
auto dynamic_props = ProcessContextImpl::getDynamicProperties(flow_file);
- for (auto& [dynamic_property_name, dynamic_property_value] : dynamic_props) {
- if (!cached_dynamic_expressions_.contains(dynamic_property_name)) {
+ const expression::Parameters params{this, flow_file};
+ for (auto& [dynamic_property_name, dynamic_property_value]: dynamic_props) {
+ auto cached_dyn_expr_it = cached_dynamic_expressions_.find(dynamic_property_name);
+ if (cached_dyn_expr_it == cached_dynamic_expressions_.end()) {
auto expression = expression::compile(dynamic_property_value);
- expression::Parameters p(this, flow_file);
- dynamic_property_value = expression(p).asString();
- cached_dynamic_expressions_.emplace(std::string{dynamic_property_name}, std::move(expression));
+ const auto [it, success] = cached_dynamic_expressions_.emplace(dynamic_property_name, expression);
+ gsl_Assert(success && "getDynamicProperties: no element with the key existed, yet insertion failed");
Review Comment:
it's not for error handling: due to a previous check within the same function, success==false should be impossible. This assert is just my way of documenting that.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2581 Fix EL evaluation in dynamic properties [nifi-minifi-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1983:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1983#discussion_r2161128990
##########
extensions/expression-language/ProcessContextExpr.cpp:
##########
@@ -73,9 +73,12 @@ std::map<std::string, std::string> ProcessContextExpr::getDynamicProperties(cons
for (auto& [dynamic_property_name, dynamic_property_value] : dynamic_props) {
Review Comment:
The cache for compiled expressions isn’t invalidated if a dynamic property is changed via `setDynamicProperty`. You should clear or update the corresponding entry in `cached_dynamic_expressions_` when properties are reset or removed to avoid stale evaluations.
##########
extensions/expression-language/ProcessContextExpr.cpp:
##########
@@ -73,9 +73,12 @@ std::map<std::string, std::string> ProcessContextExpr::getDynamicProperties(cons
for (auto& [dynamic_property_name, dynamic_property_value] : dynamic_props) {
if (!cached_dynamic_expressions_.contains(dynamic_property_name)) {
auto expression = expression::compile(dynamic_property_value);
- expression::Parameters p(this, flow_file);
- dynamic_property_value = expression(p).asString();
+ expression::Parameters parameters(this, flow_file);
+ dynamic_property_value = expression(parameters).asString();
cached_dynamic_expressions_.emplace(std::string{dynamic_property_name}, std::move(expression));
+ } else {
+ expression::Parameters parameters(this, flow_file);
+ dynamic_property_value = cached_dynamic_expressions_[dynamic_property_name](parameters).asString();
Review Comment:
[nitpick] Avoid two lookups (`contains` + `operator[]`). Consider using `auto it = cached_dynamic_expressions_.find(dynamic_property_name);` to check existence and reuse `it` for lookup or insertion.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org