Skip to content

MINIFICPP-2885 Allow explictily unsetting properties with default value - #2241

Open
martinzink wants to merge 5 commits into
apache:mainfrom
martinzink:MINIFICPP-2885
Open

MINIFICPP-2885 Allow explictily unsetting properties with default value#2241
martinzink wants to merge 5 commits into
apache:mainfrom
martinzink:MINIFICPP-2885

Conversation

@martinzink

Copy link
Copy Markdown
Member

This also fixes MINIFICPP-2884 Omitting Properties entierly skips component validation


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?
  • 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.

Comment on lines +384 to +385
} else {
validateComponentProperties(*processor, procCfg.name, "");

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment on lines +938 to +949
auto my_prop = component.getSupportedProperty(property_name);
if (!my_prop.has_value()) {
// Dynamic property fallback for previous workflow
return;
}
if (my_prop->getRequired()) {
raiseComponentError(component.getName(), "", "Can't explicitly unset required property");
}
const auto prop_def_cleared = component.clearPropertyDefaultValue(property_name);
if (!prop_def_cleared) {
raiseComponentError(component.getName(), "", prop_def_cleared.error().message());
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds explicit unsetting of optional property defaults and validates processors whose Properties section is omitted.

Changes:

  • Adds APIs for clearing property defaults.
  • Handles null YAML/JSON property values.
  • Adds a test processor and configuration tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
minifi-api/include/minifi-cpp/core/Property.h Adds default-value clearing.
minifi-api/include/minifi-cpp/core/ConfigurableComponent.h Exposes the clearing API.
core-framework/include/core/ConfigurableComponentImpl.h Declares the implementation.
core-framework/src/core/ConfigurableComponentImpl.cpp Implements default clearing.
libminifi/src/core/flow/StructuredConfiguration.cpp Handles null values and omitted properties.
extensions/stable-api-testing/PropertyTester.h Defines the test processor.
extensions/stable-api-testing/ExtensionInitializer.cpp Registers the test processor.
extensions/stable-api-testing/tests/ConfigTests.cpp Tests validation and unsetting behavior.
Suppressed comments (2)

extensions/stable-api-testing/tests/ConfigTests.cpp:153

  • These trailing commas make the payload invalid JSON; AdaptiveConfiguration therefore takes its YAML fallback path. This means the purported JSON success case does not cover the JSON parser/node implementation. Remove both trailing commas.
      },
    }

extensions/stable-api-testing/tests/ConfigTests.cpp:205

  • These trailing commas cause RapidJSON parsing to fail and the adaptive parser to treat the payload as YAML, so this test never verifies JSON null unsetting. Remove both trailing commas to exercise the intended JSON path.
      },
    }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread libminifi/src/core/flow/StructuredConfiguration.cpp
Comment thread libminifi/src/core/flow/StructuredConfiguration.cpp Outdated
Comment thread extensions/stable-api-testing/tests/ConfigTests.cpp Outdated
@martinzink
martinzink requested review from fgerlits and szaszm August 11, 2026 09:09
@lordgamez lordgamez assigned lordgamez and unassigned lordgamez Aug 11, 2026
@lordgamez
lordgamez self-requested a review August 11, 2026 09:24
Comment thread extensions/stable-api-testing/PropertyTester.h Outdated
Comment thread extensions/stable-api-testing/tests/PropertyTesterTests.cpp
Comment thread extensions/stable-api-testing/PropertyTester.h
martinzink and others added 2 commits August 11, 2026 14:33
Co-authored-by: Ferenc Gerlits <fgerlits@users.noreply.github.com>

@szaszm szaszm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's not include this in 1.0, but +1 afterwards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix priority Review these first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants