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

NIFI-12411: Update PublishAMQP to read AMQP headers value from FlowFile attributes and amq$headers string #8105

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

umarhussain15
Copy link
Contributor

Summary

NIFI-12411

Processor can now also create message headers using attributes matching the given Regex property. The name of the attribute is used as header key. In addition to that it can read values from two sources i.e. attributes matching Regex and the headers present in amq$headers string. In case of key duplication in both sources, the precedence property decide which value will be used.

AMQP attributes keys are now set as constants strings and reused where needed.

Added onScheduled to PublishAMQP and ConsumeAMQP to read all properties which don't change during running state.

Test cases added for PublishAMQP headers source changes.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 21

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@umarhussain15 umarhussain15 force-pushed the NIFI-12411-PublishAMQP-Put-header-values-from-attributes-by-matching-Regex branch from a84db6e to 8081da9 Compare December 15, 2023 19:01
Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for the proposed improvement @umarhussain15.

The general focus on the change makes sense, but there are some other stylistic recommendations I noted. It is helpful to improve the overall Processor implementation through the definition and reuse of static values for things like FlowFile attributes, so thanks for including those changes.

@umarhussain15 umarhussain15 force-pushed the NIFI-12411-PublishAMQP-Put-header-values-from-attributes-by-matching-Regex branch from 8081da9 to 0f18099 Compare April 7, 2024 19:50
@joewitt
Copy link
Contributor

joewitt commented May 4, 2024

@umarhussain15 Can you please rebase and consider the feedback shared from @exceptionfactory ?

Thanks

@umarhussain15 umarhussain15 force-pushed the NIFI-12411-PublishAMQP-Put-header-values-from-attributes-by-matching-Regex branch from 0f18099 to aad49a6 Compare May 12, 2024 11:39
@umarhussain15
Copy link
Contributor Author

Hi @joewitt @exceptionfactory,
I have reverted the changes of making class level variables based on processor properties.
Also, the branch is rebased on latest changes from main

Copy link
Contributor Author

@umarhussain15 umarhussain15 left a comment

Choose a reason for hiding this comment

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

Changes applied in new commits

@umarhussain15
Copy link
Contributor Author

@exceptionfactory @joewitt

I have added one more change, which will allow users to set amqp processor's password via env variables.
I think this will be helpful in cases where we use AMQP processors in multiple NiFI instances. Instead of copying and pasting different passwords in processor properties, we can reference the env variable via its name.
Let me know your thoughts on it.
Thanks.

@exceptionfactory
Copy link
Contributor

@exceptionfactory @joewitt

I have added one more change, which will allow users to set amqp processor's password via env variables. I think this will be helpful in cases where we use AMQP processors in multiple NiFI instances. Instead of copying and pasting different passwords in processor properties, we can reference the env variable via its name. Let me know your thoughts on it. Thanks.

Thanks for calling out this particular change @umarhussain15. Given the scope of the current pull request, I recommend considering that change separately.

In general, sensitive properties such as passwords do not support reading from environment variables. The reason is that environment variables are also accessible through Expression Language, so they are not necessarily secure. The use case is still supported through Parameter Contexts and Parameter Providers, where the Environment Variable Parameter Provider can be used. This is an additional layer of indirection, but it follows the general convention for other sensitive properties.

@umarhussain15
Copy link
Contributor Author

Thanks for the explanation, @exceptionfactory. I will revert the change here then.

…le attributes and `amq$headers` string

Processor can now also create message headers using attributes matching the given Regex property. The name of attribute is used as header key.
In addition to that it can read values from two sources i.e. attributes matching Regex and the headers present in `amq$headers` string. In case
of key duplication in both sources the precedence property decide which value will be used.

Signed-off-by: Umar Hussain <[email protected]>
@umarhussain15 umarhussain15 force-pushed the NIFI-12411-PublishAMQP-Put-header-values-from-attributes-by-matching-Regex branch from 8452c49 to b8e6ab6 Compare May 17, 2024 14:02
@umarhussain15
Copy link
Contributor Author

Reverted the AbstractAMQPProcessor change to support expression language in password field.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for following up @umarhussain15. Please note, the Checkstyle rules were recently updated to include consistency checking for whitespace, so the latest static analysis build noted these issues:

Warning:  src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java:[115,38] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
Warning:  src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java:[307,76] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
Warning:  src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java:[314,76] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
Warning:  src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java:[316,76] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
Warning:  src/main/java/org/apache/nifi/amqp/processors/PublishAMQP.java:[380,68] (whitespace) WhitespaceAround: '{' is not preceded with whitespace.
Warning:  src/main/java/org/apache/nifi/amqp/processors/ConsumeAMQP.java:[349,69] (whitespace) WhitespaceAround: '{' is not preceded with whitespace.

@umarhussain15
Copy link
Contributor Author

Checkstyle reported issues fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants