NIFIREG-395 - Implemented the ability to import and export versioned flows through the UI #319

mtien-apache · 2021-04-19T18:50:39Z

Thank you for submitting a contribution to Apache NiFi Registry.

Please provide a short description of the PR here:

Description of PR

[WIP] This PR adds the ability for a user to take a previously saved versionedFlowSnapshot JSON file and import it as a new flow definition to an existing bucket or import it as the next new version of an existing flow through the UI.
This PR also adds the ability to export a versionedFlowSnapshot as a JSON file, which removes snapshot metadata associated with the given Registry.
New dialogs have been created to handle import and export.
The REST endpoints added in BucketFlowResource are -
- importFlow - /buckets/{bucketId}/flows/import
- importVersionedFlow - /buckets/{bucketId}/flows/{flowId}/versions/import
- exportVersionedFlow- /buckets/{bucketId}/flows/{flowId}/versions/{versionNumber: \d+}/export

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 NIFIREG-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? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi-registry folder?
Have you written or updated unit tests to verify your changes?
Have you verified that the full build is successful on JDK 8?
Have you verified that the full build is successful on JDK 11?
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, including the main LICENSE file under nifi-registry-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-registry-assembly?

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 for build issues and submit an update to your PR as soon as possible.

…ioned flows through the UI. - Added REST endpoints in BucketFlowResource for importFlow, importVersionedFlow, exportVersionedFlow. - Added import and export dialogs.

mtien-apache · 2021-04-19T19:01:32Z

This PR is still WIP, but is open for initial review. I still need to add integration tests and resolve some styling issues, specifically with text alignment in the dialogs.

@exceptionfactory @thenatog @bbende @pvillard31 Would you please review?

@scottyaslan @sardell @akocsis @andras Would you please review the frontend code?

Thank you!

bbende

@mtien-apache nice work, this is going to be very helpful!

I left a few inline comments, but here are some overall results from testing with a secured registry where I authenticated with LDAP (i.e. using JWTs)...

Using JSON obtained from NiFi "Download flow definition"

Initial import worked correctly
Trying to import a second version of the flow resulted in an error "Version must be greater than zero, or use -1 to indicate latest version"
Trying to download a version of the flow popped up the file download dialog with the name "versionedFlow", after clicking save Chrome showed a download for "versionedFlow.txt" and said "Failed - Needs authorization"

Using JSON obtained from another NiFi Registry (i.e. it had the snapshot metadata populated from the other registry):

Import New Flow produced an error "No applicable policies could be found. Contact the system administrator.", I think because it was trying to store it based on the previous metadata that was never cleared out

nifi-registry-core/nifi-registry-web-api/pom.xml

...nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/BucketFlowResource.java

exceptionfactory

Thanks for the work on this feature @mtien-apache! Noted a few questions, will run through another review when it is no longer in draft status.

nifi-registry-core/nifi-registry-web-api/pom.xml

...nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/BucketFlowResource.java

exceptionfactory · 2021-04-20T15:24:54Z

...ts/explorer/grid-list/dialogs/download-versioned-flow/nf-registry-download-versioned-flow.js

+        var version = this.selectedVersion;
+
+        this.nfRegistryApi.exportDropletVersionedSnapshot(this.droplet.link.href, version).subscribe(function (response) {
+            if (!response.status || response.status === 200) {


Does a false for response.status also indicate a success? Not quite following the logic.

@exceptionfactory A successful response has typically been just the response body returned. So, when the body is returned (a success) there will not be a status property. I can see how this could be confusing at first glance and without much context. This if-statement is used throughout the codebase, so I decided to follow the conventions.

.../webapp/components/explorer/grid-list/dialogs/import-new-flow/nf-registry-import-new-flow.js

nifi-registry-core/nifi-registry-web-ui/src/main/webapp/services/nf-registry.api.js

- Revised import REST endpoints that sets a new snapshot metadata to ensure previous Registry data is not used. - Refactored some logic from the export REST endpoint into the ServiceFacade. - Fixed the frontend download API endpoint to use the appropriate href data url.

mtien-apache · 2021-04-21T04:45:26Z

I have pushed a commit that resolves the importing and exporting issues found by @bbende, as well as addressing other feedback found above this comment.

Thanks for the first reviews, @bbende and @exceptionfactory !

andrewmlim · 2021-04-23T16:02:18Z

This new functionality is great! It is going to be extremely useful.

Here are some thoughts after some UI/UX testing:

I think it would be an improvement if the "Delete data flow" action simply said "Delete flow". We refer to that type of asset in the Registry as "flow" and the documentation also refers to it as a "flow". The dialog that appears after selecting the action also says "Delete flow" so this change would help with consistency.
The title for the "Delete flow" dialog should be changed to "Delete Flow" for case consistency with the "Import New Version" and "Download Version" dialogs.
When a flow version is downloaded the default name of the file is always flow-version-#.json (where # is the version #). I think it would be helpful if the name would default to the flow name. So if I had a flow named "Kafka2HDFS" and wanted to download version 3 of it, the default name would be "Kafka2HDFS-version-3.json".
I noticed that the color of the "DELETE" button in the "Delete Flow" dialog is a different color (more red) than the other dialog buttons (IMPORT and DOWNLOAD). This appears to be intentional. The "DELETE" button for deleting a bucket is the same red, so understandable if the color difference is to make it stand out since it is a unrecoverable action.

andrewmlim · 2021-04-23T16:29:00Z

Additional thoughts:

In the "Import New Flow" and "Import New Version" dialogs, change "Data Flow Name" to "Flow Name" for same reasons stated earlier.
In the "Import New Version" dialog, consider changing "Change Log Comment" to "Version Comments" which is what is used in NiFi when saving a flow version.

andrewmlim · 2021-04-23T17:50:39Z

Testing how the feature does error handling and noticed the following:

When importing new flow, if I select a JSON file that is valid JSON, but not a valid flow definition file, after selecting import I get the error message "Cannot create versioned flow snapshot - flowContents must not be null". After closing the dialog, it seems like nothing was imported, but if you refresh the view a new flow is imported with version 0. (As an aside, if you select "Download version" on this corrupt flow, the UI seems to open a dialog on the left side of the window.)

andrewmlim · 2021-04-23T18:20:11Z

Related to my last comment, here are the messages in the error dialogs I have found so far:

Non flow definition JSON file: "Cannot create versioned flow snapshot - flowContents must not be null"
Incomplete flow definition JSON file: "Deserialization of uploaded JSON failed"

It would be great if we could make these error messages more user friendly. But that could definitely be done as a separate Jira/PR. I wouldn't block this PR to address this.

mtien-apache · 2021-04-23T21:30:48Z

@andrewmlim Thanks for reviewing!

The UI changes make sense to me, especially if the names will match what's in the documentation and NiFi.
@moranr Do you have any additional thoughts before I proceed?
For the "DELETE" flow button color in the 'Delete Flow' dialog is what's currently in Registry:

I noticed that the color of the "DELETE" button in the "Delete Flow" dialog is a different color (more red) than the other dialog buttons (IMPORT and DOWNLOAD). This appears to be intentional. The "DELETE" button for deleting a bucket is the same red, so understandable if the color difference is to make it stand out since it is a unrecoverable action.

I'm looking into the invalid flow definition file error now.

moranr · 2021-04-26T13:52:31Z

Hey @mtien-apache - thanks for all your work on this. @andrewmlim 's comments all make sense to me and I would proceed with his recommendations. RE the Delete button color, yes the vibrant red is intentional as Drew observed. I believe this color is also used in other "delete" scenarios, so perhaps double-check to make sure color usage is consistent.

mtien-apache · 2021-04-26T17:49:41Z

Hey @mtien-apache - thanks for all your work on this. @andrewmlim 's comments all make sense to me and I would proceed with his recommendations. RE the Delete button color, yes the vibrant red is intentional as Drew observed. I believe this color is also used in other "delete" scenarios, so perhaps double-check to make sure color usage is consistent.

@moranr Thanks for confirming - will proceed with the UI changes. @andrewmlim Confirmed the Delete button color is the same as other Delete buttons used throughout Registry (eg, Delete Buckets).

…he Import dialogs. - Fixed the flow definition file upload input fields to handle text overflow. NIFIREG-395 - Address PR feedback. - Adjusted import dialog label names. - Updated exported file name and format. - Added messages to inform the user why the Import New Flow button is disabled.

…uploaded. - Refactored client side import file methods to directly pass the file to the server as an API param. - Refactored client side uploadFlow method to re-use BucketFlowResource.createFlow. - Removed BucketFlowResource.importFlow method. - Refactored logic in BucketFlowResource.importVersionedFlow to the service facade.

mtien-apache · 2021-04-29T02:09:13Z

Pushed 2 commits:

Handled the scenario when an invalid snapshot file is uploaded during Import New Flow @andrewmlim
Refactored client side API import methods
- In the upload file methods, I removed formData and now passing the file as an API param. Also using HTTP headers to pass additional data
- In uploadFlow, I'm now making multiple calls to the server that re-uses the existing BucketFlowResource.createFlow method
Refactored server side import methods
- Removed BucketFlowResource.importFlow
- Refactored logic from BucketFlowResource.importVersionedFlow to the service facade

UI changes:

Fixed remaining UI style issues
In the 'Import New Flow' dialog, the initial value for the Bucket dropdown menu is set to the Bucket the user is currently viewing. Otherwise, no value is set when viewing all buckets.
Added messages to inform the user why the 'Import New Flow' button is displayed
- One displays a message in the center of the page when there are no existing buckets
- Another message is displayed under the button when buckets exist but the user does not have any write permissions

andrewmlim · 2021-04-30T16:45:22Z

Testing the latest changes and noticed that whenever I select "Import new version", the version is always shown as "v.2" in the "Import New Version" dialog even if is it the 3rd, 4th, 5th, etc. version that I am importing.

andrewmlim · 2021-04-30T17:42:57Z

I've seen the change where the name of the flow download is no longer always flow-version-#.json. But the name of the download defaults to the name of the process group defined in the flow definition not the name of the flow as I suggested in my earlier comments. For example, I import a new flow and give it the flow name "Kafka2HDFS" and select a flow definition file that has the name "ASDF" for the process group. This new flow is imported successfully. Within the NiFi Registry UI, there is nothing that references/displays "ASDF".

But if I choose to download this flow version from Registry, the name of the file is "ASDF-version-1.json". I expected it to be "Kafka2HDFS-version-1.json".

andrewmlim · 2021-04-30T17:45:55Z

In the Actions drop-down, "Import new version" is always highlighted even when another selection has the focus.

andrewmlim · 2021-04-30T20:34:14Z

Related to my last comment, here are the messages in the error dialogs I have found so far:

Non flow definition JSON file: "Cannot create versioned flow snapshot - flowContents must not be null"

Incomplete flow definition JSON file: "Deserialization of uploaded JSON failed"

It would be great if we could make these error messages more user friendly. But that could definitely be done as a separate Jira/PR. I wouldn't block this PR to address this.

For the incomplete flow definition JSON file, the error I see now when importing is: "Unexpected end-of-input: expected close marker for Object (start marker at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 1, column: 1]) at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 2, column: 1]"

which is more specific to the issue (that there is no closing } in the JSON), but less readable.

mtien-apache · 2021-05-03T23:09:28Z

Related to my last comment, here are the messages in the error dialogs I have found so far:

Non flow definition JSON file: "Cannot create versioned flow snapshot - flowContents must not be null"

Incomplete flow definition JSON file: "Deserialization of uploaded JSON failed"

It would be great if we could make these error messages more user friendly. But that could definitely be done as a separate Jira/PR. I wouldn't block this PR to address this.

@andrewmlim I refactored the resource method to let the Spring framework to automatically handle the deserialization and removed the "Deserialization of uploaded JSON failed" message.

Agree to improve the error message in a separate PR for the "Cannot create versioned flow snapshot - flowContents must not be null" message.

For the incomplete flow definition JSON file, the error I see now when importing is: "Unexpected end-of-input: expected close marker for Object (start marker at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 1, column: 1]) at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 2, column: 1]"

which is more specific to the issue (that there is no closing } in the JSON), but less readable.

After investigating this exception message, it seems the exception is being caught by an ExceptionMapper and translating it to a BadRequest. It may be from jackson-databind in the Spring framework, which is not something we have much control over. It's possible to write a custom ExceptionMapper for a BadRequest, but the custom message could end up applying across the app for any method throwing that type of Exception, which can be addressed in a separate PR.

Although I agree the error message could be better, it at least points to which line the error is found. I believe this is sufficient for the time being. Thoughts?

andrewmlim · 2021-05-04T02:36:13Z

Although I agree the error message could be better, it at least points to which line the error is found. I believe this is sufficient for the time being. Thoughts?

I agree that it is sufficient for now. All of the error messages can be handled outside of this PR. Thanks!

- Fixed Import New Version dialog to show the next version being imported. - Added frontend and backend integration tests. - Created ExportedVersionedFlowSnapshot object. - Added test resource file and updated rat configuration to skip the file during contrib-check. - Refactored frontend and server side code.

mtien-apache · 2021-05-04T19:57:13Z

Pushed a commit:

Added a new ExportedVersionedFlowSnapshot object
Fixed Import New Version dialog to show the next version to be imported
Refactored frontend and server side code.
Added frontend and backend integration tests.

Will add more frontend tests.

mtien-apache · 2021-05-04T20:21:18Z

In the Actions drop-down, "Import new version" is always highlighted even when another selection has the focus.

@andrewmlim (cc: @moranr ) It seems like the initial focus behavior is based on keyboard actions and from an accessibility perspective, it's the expected behavior. When using the arrow keys, the focus moves with the new selection. I think what is making it confusing is that the keyboard and mouseover selections have the same red highlighting styles. I believe this highlighting behavior was always present but probably wasn't an issue since there had only been 1 menu item (Delete).

One proposed fix is I can dig in to change the styles between a keyboard selection and mouseover selection, something like in other dropdown menus in Registry:

Thoughts?

exceptionfactory

The recent updates look good @mtien-apache! I noted a few additional minor recommendations.

...nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/BucketFlowResource.java

...gistry-web-api/src/main/java/org/apache/nifi/registry/web/service/StandardServiceFacade.java

...istry-core/nifi-registry-web-api/src/test/java/org/apache/nifi/registry/web/api/FlowsIT.java

.../webapp/components/explorer/grid-list/dialogs/import-new-flow/nf-registry-import-new-flow.js

exceptionfactory · 2021-05-04T21:29:25Z

nifi-registry-core/nifi-registry-web-ui/src/main/webapp/services/nf-registry.api.js

+        return this.http.get(url, options).pipe(
+            map(function (response) {
+                // export the VersionedFlowSnapshot by creating a hidden anchor element
+                var stringSnapshot = encodeURIComponent(JSON.stringify(response.body, null, 2));


Is it necessary to call JSON.stringify() or is it possible to just get the text content of response.body? If response.body is already JSON, that means it went through an unnecessary serialization step, which would be great to avoid if possible.

@exceptionfactory Good catch! I can return a responseType: 'text' and omit calling JSON.stringify(). The file content will not be in pretty print format but that seems ok to me.

...nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/BucketFlowResource.java

bbende · 2021-05-10T14:10:12Z

Latest changes look good!

One minor comment... when downloading a version, the window that pops up has the latest version labeled as "Latest Version (v.#)" but then the other items in the list are just "Version #", seems a little inconsistent that one has "v." and the others don't. Should the latest one just say "Latest Version (#)" or maybe "Version # (latest)" ?

Also, while writing this, I started thinking about how we have generally been referring to this as import/export, and the buttons for import use the word "Import", but the for export we are calling it "Download Version". I'm not too hung up on this, but started wondering if that should be called "Export Version" to better correspond with "Import"?

andrewmlim · 2021-05-10T21:51:02Z

One proposed fix is I can dig in to change the styles between a keyboard selection and mouseover selection, something like in other dropdown menus in Registry:

Thoughts?

That would be nice if you could change the styles. That would definitely help distinguish the two difference selections.

andrewmlim · 2021-05-10T21:51:48Z

I tested a secure Registry with different authorizations given to users (on the users themselves and also on bucket policies). Auth looks good!

andrewmlim · 2021-05-10T21:56:48Z

One minor comment... when downloading a version, the window that pops up has the latest version labeled as "Latest Version (v.#)" but then the other items in the list are just "Version #", seems a little inconsistent that one has "v." and the others don't. Should the latest one just say "Latest Version (#)" or maybe "Version # (latest)" ?

Also, while writing this, I started thinking about how we have generally been referring to this as import/export, and the buttons for import use the word "Import", but the for export we are calling it "Download Version". I'm not too hung up on this, but started wondering if that should be called "Export Version" to better correspond with "Import"?

I think the most consistent label would be "Latest (Version #)".

"Export Version" is a good suggestion. Like Bryan, I don't think that change is critical.

moranr · 2021-05-11T13:27:56Z

I think the most consistent label would be "Latest (Version #)".

@mtien-apache I like that suggestion.

"Export Version" is a good suggestion. Like Bryan, I don't think that change is critical.

Also agree here, but not critical. Typically you see Upload/Download or Import/Export used in together.

- Updated Flow Actions menu hover and focus styles. - Changed dialog title to 'Export Version', the latest menu item name, and renamed the component. - Disabled frontend export flows depending on read permissions. - Added and updated tests

mtien-apache · 2021-05-13T00:11:49Z

Pushed commits that addresses all remaining changes and PR feedback. The PR is ready for a full review.
@sardell Would you review the frontend changes?

Recent changes include:

Changed the dialog title from 'Download Version' to 'Export Version' and refactored the component. @bbende @andrewmlim @moranr Thanks for the feedback and agreed 'Export' seems more consistent
Changed the 'Export Version' dialog dropdown menu item to "Latest (Version #)"
Changed the Actions menu hover style to differentiate from the focus style
Added frontend tests. Note - With the way Angular and Javascript is currently configured in Registry, I had issues writing certain tests for the new components. It's been captured in a JIRA ticket: NIFIREG-454. Once resolved, more tests can be added.

exceptionfactory

Thanks for working through the feedback details @mtien-apache! This looks just about good to go, I noted one more detail around the HTTP 201 Created response.

...nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/BucketFlowResource.java

andrewmlim · 2021-05-14T18:00:16Z

Recent changes include:

Changed the dialog title from 'Download Version' to 'Export Version' and refactored the component. @bbende @andrewmlim @moranr Thanks for the feedback and agreed 'Export' seems more consistent

Changed the 'Export Version' dialog dropdown menu item to "Latest (Version #)"

Changed the Actions menu hover style to differentiate from the focus style

These look good! Thanks for making all of these improvements @mtien-apache !

- Updated comments header

sardell · 2021-05-14T21:04:05Z

@mtien-apache I just finished looking at the UI code and testing your changes in the UI. Nice work. I'm a +1 (non-binding).

mtien-apache · 2021-05-27T18:59:04Z

Closed in favor of apache/nifi#5107

NIFIREG-395 - [WIP] Implemented the ability to import and export vers…

14b8bbe

…ioned flows through the UI. - Added REST endpoints in BucketFlowResource for importFlow, importVersionedFlow, exportVersionedFlow. - Added import and export dialogs.

bbende reviewed Apr 20, 2021

View reviewed changes

exceptionfactory reviewed Apr 20, 2021

View reviewed changes

NIFIREG-395 - Fix styling in dialogs.

78109ae

mtien-apache added 2 commits April 27, 2021 11:04

exceptionfactory reviewed May 4, 2021

View reviewed changes

NIFIREG-395 - Address PR feedback.

bdb4d06

- Updated Flow Actions menu hover and focus styles. - Changed dialog title to 'Export Version', the latest menu item name, and renamed the component. - Disabled frontend export flows depending on read permissions. - Added and updated tests

mtien-apache marked this pull request as ready for review May 12, 2021 20:16

mtien-apache added 2 commits May 12, 2021 14:27

NIFIREG-395 - Removed JSON.stringify and return a responseType of text.

62a69b8

NIFIREG-395 - Updated the 'Export Version' latest menu item name

24f4d05

mtien-apache requested a review from bbende May 13, 2021 15:33

exceptionfactory reviewed May 14, 2021

View reviewed changes

...nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/BucketFlowResource.java Show resolved Hide resolved

...nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/BucketFlowResource.java Outdated Show resolved Hide resolved

NIFIREG-395 - Added a Location header for a created Response

6d9901a

- Updated comments header

exceptionfactory approved these changes May 14, 2021

View reviewed changes

mtien-apache closed this May 27, 2021

andrewmlim mentioned this pull request Jun 3, 2021

NIFIREG-395 - Implemented the ability to import and export versioned … apache/nifi#5107

Merged

13 tasks

NIFIREG-395 - Implemented the ability to import and export versioned flows through the UI #319

NIFIREG-395 - Implemented the ability to import and export versioned flows through the UI #319

Conversation

mtien-apache commented Apr 19, 2021

Description of PR

For all changes:

For code changes:

For documentation related changes:

Note:

mtien-apache commented Apr 19, 2021

bbende left a comment

Choose a reason for hiding this comment

exceptionfactory left a comment

Choose a reason for hiding this comment

exceptionfactory Apr 20, 2021

Choose a reason for hiding this comment

mtien-apache May 12, 2021

Choose a reason for hiding this comment

mtien-apache commented Apr 21, 2021

andrewmlim commented Apr 23, 2021 • edited Loading

andrewmlim commented Apr 23, 2021

andrewmlim commented Apr 23, 2021

andrewmlim commented Apr 23, 2021

mtien-apache commented Apr 23, 2021

moranr commented Apr 26, 2021

mtien-apache commented Apr 26, 2021

mtien-apache commented Apr 29, 2021 • edited Loading

andrewmlim commented Apr 30, 2021

andrewmlim commented Apr 30, 2021

andrewmlim commented Apr 30, 2021

andrewmlim commented Apr 30, 2021

mtien-apache commented May 3, 2021

andrewmlim commented May 4, 2021

mtien-apache commented May 4, 2021

mtien-apache commented May 4, 2021

exceptionfactory left a comment

Choose a reason for hiding this comment

exceptionfactory May 4, 2021

Choose a reason for hiding this comment

mtien-apache May 12, 2021

Choose a reason for hiding this comment

bbende commented May 10, 2021

andrewmlim commented May 10, 2021

andrewmlim commented May 10, 2021

andrewmlim commented May 10, 2021

moranr commented May 11, 2021

mtien-apache commented May 13, 2021

exceptionfactory left a comment

Choose a reason for hiding this comment

andrewmlim commented May 14, 2021

sardell commented May 14, 2021

mtien-apache commented May 27, 2021

andrewmlim commented Apr 23, 2021 •

edited

Loading

mtien-apache commented Apr 29, 2021 •

edited

Loading