NIFI-12017 add ability to choose to output to single line for base32 base64 contents #8417

knguyen1 · 2024-02-15T19:47:10Z

Summary

NIFI-12017

Tracking

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

Issue Tracking

Apache NiFi Jira issue created

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

knguyen1 · 2024-02-15T19:59:14Z

FYI: @exceptionfactory @dan-s1

...dard-processors/src/main/java/org/apache/nifi/processors/standard/encoding/EncodingMode.java

Update nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/encoding/EncodingMode.java Co-authored-by: dan-s1 <dstieg1@gmail.com> Simplify if logic, simplify enum expression

dan-s1 · 2024-02-15T23:25:52Z

@knguyen1 It looks like there are Checkstyle violations that must be corrected.

knguyen1 · 2024-02-15T23:57:13Z

@knguyen1 It looks like there are Checkstyle violations that must be corrected.

Fixed. 95f6c0a

knguyen1 · 2024-02-16T12:06:47Z

@dan-s1 I just realised our change to enum introduces a breaking change.

case "BASE64_ENCODING":
case "BASE32_ENCODING":

It's because we're using name() from the enum. Thoughts on overriding getValue() and return the old string values instead?

dan-s1

The suggested changes addresses your latest question

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java

Fixes backward-compatibility issue, earlier proc uses `base32`, `base64`, `hex`, etc. as string. Co-authored-by: dan-s1 <dstieg1@gmail.com>

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java

…/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Co-authored-by: dan-s1 <dstieg1@gmail.com>

dan-s1

Hopefully we are getting closer. I still need to look over the unit tests but here are some formatting suggestions. Now with Junit5 none of the test methods need to be public so the public modifier should be removed from the tests similar to the ones you added. In addition the standard in the code base is to have the @Test annotation above the test method signature so most of the current suggestions are to align with that . Thanks!

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java

...standard-processors/src/test/java/org/apache/nifi/processors/standard/TestEncodeContent.java

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java

...standard-processors/src/test/java/org/apache/nifi/processors/standard/TestEncodeContent.java

dan-s1

Further suggestions to refactor the multiple unit test into single parameterized tests.

...standard-processors/src/test/java/org/apache/nifi/processors/standard/TestEncodeContent.java

…undTrip` to `testRoundTrip`

dan-s1

A few more things which should be changed.

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java

...standard-processors/src/test/java/org/apache/nifi/processors/standard/TestEncodeContent.java

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java

Co-authored-by: dan-s1 <dstieg1@gmail.com>

dan-s1 · 2024-02-27T21:05:33Z

@exceptionfactory When you get a chance can you please check things over to see if all necessary changes were made? Thanks!

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java

Co-authored-by: dan-s1 <dstieg1@gmail.com>

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java

Co-authored-by: dan-s1 <dstieg1@gmail.com>

exceptionfactory

Thanks for the work on these new properties @knguyen1, and thanks for the feedback thus far @dan-s1.

The general capability is useful, but there are still several refinements needed to maintain backward compatibility. I also recommended a few documentation changes. One particular note is the Line Separator property, which seems unnecessary in general. Unless there is a strong reason to support either LF or CRLF, just using LF across the board seems simpler. Sometimes less is more when it comes to configuration, but feel free to provide more details.

...dard-processors/src/main/java/org/apache/nifi/processors/standard/encoding/EncodingMode.java

...dard-processors/src/main/java/org/apache/nifi/processors/standard/encoding/EncodingType.java

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java

Co-authored-by: David Handermann <exceptionfactory@apache.org>

…le-line-for-base32-base64-contents' of github.com:knguyen1/nifi into feat/NIFI-12017/add-ability-to-choose-to-output-to-single-line-for-base32-base64-contents

knguyen1 · 2024-03-03T23:44:49Z

I fixed failing tests and build failures from the latest code review. Everything's passing now. @exceptionfactory

I had to add this method: EncodingType getEncodingTypeFromString(final String encodingTypeValue) because previous implementation used hex instead of hexadecimal as in @exceptionfactory's suggestion. So we can't easily use EncodingType.valueOf anymore.

Latest is pending the resolution of comments regarding.

Is this property necessary? It seems best to hard-code the line-feed character \n, rather than making this platform-dependent or configurable.

This description needs some rewording and clarification. It is not clear why it is rounded to a multiple of 4, and again, the conditional nature of the property does not need to be stated.

I have left those threads unresolved.

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java

Co-authored-by: dan-s1 <dstieg1@gmail.com>

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java

Co-authored-by: dan-s1 <dstieg1@gmail.com>

exceptionfactory

Thanks for updating the property values @knguyen1.

The last remaining item to change is the Encoded Content Line Separator property. I recommend removing the property and using a hard-coded \n value so that the behavior is the same regardless of the operating system on which NiFi is running.

...dard-processors/src/main/java/org/apache/nifi/processors/standard/encoding/EncodingMode.java

- Set positive integer validator for Encoded Line Length

exceptionfactory

Thanks for working through the feedback @knguyen1. After one more runtime review, I adjusted the new properties to be required, since the default values align with current behavior. I also set the Encoded Line Length property to use the Positive Integer Validator, ensuring that the value will never be zero or lower.

With these changes, I plan to merge soon, pending one more successful build.

exceptionfactory

Thanks again for taking the time to work through all the feedback @knguyen1! +1 merging

exceptionfactory · 2024-03-28T01:49:28Z

Thanks for the reviews as well @dan-s1!

knguyen1 mentioned this pull request Feb 15, 2024

NIFI-12017 add ability to choose to output to single line for base32 base64 contents #8038

Closed

9 tasks

dan-s1 reviewed Feb 15, 2024

View reviewed changes

...dard-processors/src/main/java/org/apache/nifi/processors/standard/encoding/EncodingMode.java Outdated Show resolved Hide resolved

knguyen1 force-pushed the feat/NIFI-12017/add-ability-to-choose-to-output-to-single-line-for-base32-base64-contents branch from 7f97614 to f07f292 Compare February 15, 2024 21:12

Remove trailing space

95f6c0a

dan-s1 reviewed Feb 16, 2024

View reviewed changes

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Outdated Show resolved Hide resolved

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Outdated Show resolved Hide resolved

Apply suggestions from code review

a7786ae

Fixes backward-compatibility issue, earlier proc uses `base32`, `base64`, `hex`, etc. as string. Co-authored-by: dan-s1 <dstieg1@gmail.com>

dan-s1 reviewed Feb 16, 2024

View reviewed changes

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Outdated Show resolved Hide resolved

Update nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors…

d804d39

…/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Co-authored-by: dan-s1 <dstieg1@gmail.com>

dan-s1 reviewed Feb 16, 2024

View reviewed changes

...standard-processors/src/test/java/org/apache/nifi/processors/standard/TestEncodeContent.java Show resolved Hide resolved

dan-s1 reviewed Feb 16, 2024

View reviewed changes

...standard-processors/src/test/java/org/apache/nifi/processors/standard/TestEncodeContent.java Outdated Show resolved Hide resolved

knguyen1 added 7 commits February 16, 2024 18:43

Remove getValue() from defaultValue defs

c5e0e4d

Remove public from tests; break line for @Test and void

8c16ea8

Use org.junit.jupiter.api.Assertions for assertEquals

4e7732f

Refactor testBasicEncodeBase32 to use ParameterizedTest

9fd11b7

Parameterize testBasicEncodeHex and testBasicDecodeBase32

ed7bd5a

Refactor testBasicDecodeHex to use ParameterizedTest

743d55d

Refactor testBasicDecodeBase64 to use ParameterizedTest

a4ee842

dan-s1 reviewed Feb 16, 2024

View reviewed changes

...standard-processors/src/test/java/org/apache/nifi/processors/standard/TestEncodeContent.java Outdated Show resolved Hide resolved

knguyen1 added 2 commits February 16, 2024 20:33

Collapse testBase64RoundTrip, testBase32RoundTrip, and `testHexRo…

e994e3c

…undTrip` to `testRoundTrip`

Collapse "not" hex, base32, base64 tests to testDecodeFailure

1a15112

dan-s1 reviewed Feb 26, 2024

View reviewed changes

Apply suggestions from code review

164e7c2

Co-authored-by: dan-s1 <dstieg1@gmail.com>

dan-s1 reviewed Feb 27, 2024

View reviewed changes

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Outdated Show resolved Hide resolved

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Outdated Show resolved Hide resolved

Apply suggestions from code review

2c554eb

Co-authored-by: dan-s1 <dstieg1@gmail.com>

dan-s1 reviewed Feb 27, 2024

View reviewed changes

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Outdated Show resolved Hide resolved

From CR: Refactor lineLength var for clarity

229dc0c

Co-authored-by: dan-s1 <dstieg1@gmail.com>

exceptionfactory requested changes Mar 3, 2024

View reviewed changes

knguyen1 and others added 2 commits March 3, 2024 07:36

Apply suggestions from code review by @exceptionfactory

0f2624e

Co-authored-by: David Handermann <exceptionfactory@apache.org>

Add value for EncodingType enum; fix value for encoding mode

ecc3263

knguyen1 force-pushed the feat/NIFI-12017/add-ability-to-choose-to-output-to-single-line-for-base32-base64-contents branch from ecc3263 to 229dc0c Compare March 3, 2024 13:15

knguyen1 and others added 3 commits March 3, 2024 08:26

Apply suggestions from code review

46a4053

Co-authored-by: David Handermann <exceptionfactory@apache.org>

Merge branch 'feat/NIFI-12017/add-ability-to-choose-to-output-to-sing…

e6c4cc6

…le-line-for-base32-base64-contents' of github.com:knguyen1/nifi into feat/NIFI-12017/add-ability-to-choose-to-output-to-single-line-for-base32-base64-contents

Fix build and test failures from code review suggs.

c062801

Remove colloquial we from comments

74fd762

dan-s1 reviewed Mar 4, 2024

View reviewed changes

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Outdated Show resolved Hide resolved

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Outdated Show resolved Hide resolved

knguyen1 and others added 2 commits March 4, 2024 09:54

Apply suggestions from code review

8c254df

Co-authored-by: dan-s1 <dstieg1@gmail.com>

Fix getEncodingType call

1f1e982

dan-s1 reviewed Mar 4, 2024

View reviewed changes

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Outdated Show resolved Hide resolved

...ifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/EncodeContent.java Outdated Show resolved Hide resolved

Apply suggestions from code review

f094026

Co-authored-by: dan-s1 <dstieg1@gmail.com>

exceptionfactory reviewed Mar 12, 2024

View reviewed changes

...dard-processors/src/main/java/org/apache/nifi/processors/standard/encoding/EncodingMode.java Outdated Show resolved Hide resolved

knguyen1 and others added 6 commits March 12, 2024 14:32

Fix indentations

b8df280

Remove ENCODED_LINE_SEPARATOR property and tests

ce32323

Actually fix spacings

cb4ce48

Fix license has extra space

84c64bd

Remove un-used import

a8d0498

NIFI-12017 Set new properties as required

623dd93

- Set positive integer validator for Encoded Line Length

exceptionfactory reviewed Mar 26, 2024

View reviewed changes

exceptionfactory approved these changes Mar 28, 2024

View reviewed changes

exceptionfactory closed this in 5cc857b Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-12017 add ability to choose to output to single line for base32 base64 contents #8417

NIFI-12017 add ability to choose to output to single line for base32 base64 contents #8417

knguyen1 commented Feb 15, 2024 •

edited

knguyen1 commented Feb 15, 2024

dan-s1 commented Feb 15, 2024

knguyen1 commented Feb 15, 2024

knguyen1 commented Feb 16, 2024

dan-s1 left a comment

dan-s1 left a comment •

edited

dan-s1 left a comment

dan-s1 left a comment

dan-s1 commented Feb 27, 2024

exceptionfactory left a comment

knguyen1 commented Mar 3, 2024

exceptionfactory left a comment

exceptionfactory left a comment •

edited

exceptionfactory left a comment

exceptionfactory commented Mar 28, 2024

NIFI-12017 add ability to choose to output to single line for base32 base64 contents #8417

NIFI-12017 add ability to choose to output to single line for base32 base64 contents #8417

Conversation

knguyen1 commented Feb 15, 2024 • edited

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

knguyen1 commented Feb 15, 2024

dan-s1 commented Feb 15, 2024

knguyen1 commented Feb 15, 2024

knguyen1 commented Feb 16, 2024

dan-s1 left a comment

Choose a reason for hiding this comment

dan-s1 left a comment • edited

Choose a reason for hiding this comment

dan-s1 left a comment

Choose a reason for hiding this comment

dan-s1 left a comment

Choose a reason for hiding this comment

dan-s1 commented Feb 27, 2024

exceptionfactory left a comment

Choose a reason for hiding this comment

knguyen1 commented Mar 3, 2024

exceptionfactory left a comment

Choose a reason for hiding this comment

exceptionfactory left a comment • edited

Choose a reason for hiding this comment

exceptionfactory left a comment

Choose a reason for hiding this comment

exceptionfactory commented Mar 28, 2024

knguyen1 commented Feb 15, 2024 •

edited

dan-s1 left a comment •

edited

exceptionfactory left a comment •

edited