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.

Sign up for GitHub

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

Jump to bottom

NIFI-11059 PutBoxFile processor #6892

Closed

krisztina-zsihovszki wants to merge 6 commits into apache:main from krisztina-zsihovszki:NIFI-11059

Contributor

krisztina-zsihovszki commented Jan 26, 2023

Summary

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-11059
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-11059

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 8
- JDK 11
- JDK 17

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

turcsanyip self-requested a review

January 26, 2023 09:47

exceptionfactory reviewed

View reviewed changes

Contributor

exceptionfactory left a comment

Thanks for working on this new Processor @krisztina-zsihovszki! I will defer to others for a more thorough code and functionality review, but I noted a handful of minor stylistic recommendations.

...undle/nifi-box-processors/src/test/java/org/apache/nifi/processors/box/FetchBoxFileTest.java Outdated

+                      final BoxFile.Info fetchedFileInfo = createFileInfo(TEST_PATH,  MODIFIED_TIME);
+                      doReturn(fetchedFileInfo).when(mockBoxFile).getInfo();
+                      // WHEN

Contributor

exceptionfactory Jan 26, 2023

The use of WHEN and THEN comments should be avoided.

...ndle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/BoxFileAttributes.java Outdated

+                  public static final String ID = "box.id";
+                  public static final String ID_DESC = "The id of the file";
+                  public static final String FILENAME = CoreAttributes.FILENAME.key();

Contributor

exceptionfactory Jan 26, 2023

I recommend avoiding reproducing the FILENAME and PATH attributes and using the CoreAttributes enum directly.

...ox-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/BoxFileUtils.java Outdated

+                  }
+                  public static Map<String, String> createAttributeMap(Info fileInfo) {
+                      final Map<String, String> attributes = new HashMap<>();

Contributor

exceptionfactory Jan 26, 2023

Recommend using LinkedHashMap instead of HashMap for deterministic ordering of attributes added.

...ox-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/FetchBoxFile.java Outdated

-                      outFlowFile = session.putAttribute(outFlowFile, ERROR_MESSAGE_ATTRIBUTE, e.getMessage());
-                      session.transfer(outFlowFile, REL_FAILURE);
+                      flowFile = session.putAttribute(flowFile, ERROR_CODE, "" + e.getResponseCode());

Contributor

exceptionfactory Jan 26, 2023

This is a good opportunity to adjust this approach. String.valueOf(e.getResponseCode()) should be used instead of concatenating an empty string to the response code.

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated

+                          REL_FAILURE
+                  )));
+                  public static final int CONFLICT_RESPONSE_CODE = 409;

Contributor

exceptionfactory Jan 26, 2023

Can this variable be protected or private?

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated

+                          getLogger().error("Exception occurred while uploading file '{}' to Box folder '{}'", filename, fullPath, e);
+                          handleExpectedError(session, flowFile, e);
+                      } catch (Exception e) {
+                          getLogger().error("Unexpected exception occurred while uploading file '{}' to Box folder '{}'", filename, fullPath, e);

Contributor

exceptionfactory Jan 26, 2023

Suggested change

      
                        getLogger().error("Unexpected exception occurred while uploading file '{}' to Box folder '{}'", filename, fullPath, e);
          
                        getLogger().error("Upload failed: File [{}] folder [{}]", filename, fullPath, e);

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated

+                      }
+                      if (createFolder) {
+                          getLogger().debug("Create folder " + folderName + " parent id: " + parentFolderId);

Contributor

exceptionfactory Jan 26, 2023

Log statements should use placeholders:

Suggested change

      
                        getLogger().debug("Create folder " + folderName + " parent id: " + parentFolderId);
          
                        getLogger().debug("Creating Folder [{}] Parent [{}]", folderName, parentFolderId);

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated

+                          getLogger().info("File with the same name '{}' already exists in '%s'. Remote file is not modified due to {} being set to '{}'.",
+                                  filename, path, CONFLICT_RESOLUTION.getDisplayName(), conflictResolution);
+                      } else if (FAIL_RESOLUTION.equals(conflictResolution)) {
+                          throw new ProcessException(format("File with the same name '%s' already exists in '%s'.", filename, path), e);

Contributor

exceptionfactory Jan 26, 2023

Recommend avoiding periods at the end of messages:

Suggested change

      
                        throw new ProcessException(format("File with the same name '%s' already exists in '%s'.", filename, path), e);
          
                        throw new ProcessException(format("File with the same name [%s] already exists in [%s]", filename, path), e);

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated

+                      final String path = BoxFileUtils.getPath(folder.getInfo());
+                      if (IGNORE_RESOLUTION.equals(conflictResolution)) {
+                          getLogger().info("File with the same name '{}' already exists in '%s'. Remote file is not modified due to {} being set to '{}'.",

Contributor

exceptionfactory Jan 26, 2023

Suggested change

      
                        getLogger().info("File with the same name '{}' already exists in '%s'. Remote file is not modified due to {} being set to '{}'.",
          
                        getLogger().info("File with the same name [{}] already exists in [%s] Remote file is not modified due to [{}] being set to [{}]",

exceptionfactory reviewed

View reviewed changes

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated

Comment on lines 92 to 94

+                  public static final String IGNORE_RESOLUTION = "ignore";
+                  public static final String REPLACE_RESOLUTION = "replace";
+                  public static final String FAIL_RESOLUTION = "fail";

Contributor

exceptionfactory Jan 26, 2023

Based on your work for NIFI-11068 in PR #6861, it looks like these variables can be replaced with the new ConflictResolutionStrategy.

turcsanyip requested changes

View reviewed changes

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...s/src/main/resources/docs/org.apache.nifi.processors.box.FetchBoxFile/additionalDetails.html Outdated Show resolved Hide resolved

...s/src/main/resources/docs/org.apache.nifi.processors.box.FetchBoxFile/additionalDetails.html Outdated Show resolved Hide resolved

...ors/src/main/resources/docs/org.apache.nifi.processors.box.PutBoxFile/additionalDetails.html Outdated Show resolved Hide resolved

...ox-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/FetchBoxFile.java Outdated Show resolved Hide resolved

...ox-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/FetchBoxFile.java Outdated Show resolved Hide resolved

...ndle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/BoxFileAttributes.java Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

krisztina-zsihovszki added 3 commits

January 27, 2023 12:55


          NIFI-11059 PutBoxFile processor

f3f5646


          NIFI-11059 Review comments

35d491c


          NIFI-11059 Review comments

0867d0a

krisztina-zsihovszki force-pushed the NIFI-11059 branch from ebca5e4 to 0867d0a Compare

January 27, 2023 17:51


          NIFI-11059 Additional unit tests added

f8b00eb

turcsanyip requested changes

View reviewed changes

...ox-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/FetchBoxFile.java Outdated Show resolved Hide resolved

...ors/src/main/resources/docs/org.apache.nifi.processors.box.PutBoxFile/additionalDetails.html Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved


          NIFI-11059 Review comments

db226ea

turcsanyip requested changes

View reviewed changes

Contributor

turcsanyip left a comment

@krisztina-zsihovszki Thanks for the latest changes!
I found only some minor issues / improvements. Pls. see my comments below.

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...-box-bundle/nifi-box-processors/src/main/java/org/apache/nifi/processors/box/PutBoxFile.java Outdated Show resolved Hide resolved

...-bundle/nifi-box-processors/src/test/java/org/apache/nifi/processors/box/PutBoxFileTest.java Outdated Show resolved Hide resolved


          NIFI-11059 Review comments

99e313f

turcsanyip approved these changes

View reviewed changes

Contributor

turcsanyip left a comment

Thanks @krisztina-zsihovszki!

+1 LGTM

@exceptionfactory Do you have any further comments or can we merge it?

exceptionfactory approved these changes

View reviewed changes

Contributor

exceptionfactory left a comment

Thanks for working on this @krisztina-zsihovszki, looks good! No further comments @turcsanyip, feel free to merge!

Contributor

turcsanyip commented Feb 2, 2023

@exceptionfactory Thanks for your review!
Merging to main...

asfgit closed this in

653631c

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