NIFI-7037 Split off username and password fields for GetMongo processor by karthik-kadajji · Pull Request #4231 · apache/nifi

karthik-kadajji · 2020-04-24T08:59:26Z

Thank you for submitting a contribution to Apache NiFi.

Please provide a short description of the PR here:

Description of PR

Enables X functionality; fixes bug NIFI-YYYY.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

[X ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
[X ] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
[X ] Has your PR been rebased against the latest commit within the target branch (typically master)?
[ X] 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:

[ X] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
[ X] Have you written or updated unit tests to verify your changes?
Have you verified that the full build is successful on both JDK 8 and 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-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
[ X] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

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

HorizonNet · 2020-04-24T16:03:17Z

+        runner.removeProperty(AbstractMongoProcessor.URI);
+        runner.setVariable("uri", "mongodb://localhost:27017/?authSource="+DB_NAME);
+        runner.setProperty(AbstractMongoProcessor.URI, "${uri}");
+        runner.setProperty(GetMongo.PASSWORD,password);


To stay consistent, add a space between the arguments. There are also other similar places in this class.

I added the spacing . Thank you

HorizonNet · 2020-04-24T16:04:22Z

-            new Document("_id", "doc_1").append("a", 1).append("b", 2).append("c", 3),
-            new Document("_id", "doc_2").append("a", 1).append("b", 2).append("c", 4).append("date_field", CAL.getTime()),
-            new Document("_id", "doc_3").append("a", 1).append("b", 3)
+                new Document("_id", "doc_1").append("a", 1).append("b", 2).append("c", 3),


Was this done by IDE auto-indentation or because the earlier version did not match the general indentation? There are also similar places in the other classes.

Yes, it is due to earlier version mismatch, for continuous indentation it is 8 spaces in some places and 4 at some. However, I have indented as before.

HorizonNet · 2020-04-24T16:04:45Z

+        ArrayList users = (ArrayList) result.get("users");
+        if (!users.isEmpty()) {
+            db.runCommand(dropUserCommand);
+            System.out.println("dropping user");


Better would be to use a logger.

I have dropped the entire line, since it is a test case, didnt need the "dropping user" message or log

HorizonNet

Left three minor NITs. LGTM overall.

HorizonNet · 2020-04-27T10:31:14Z


                    outgoingFlowFile = session.putAllAttributes(outgoingFlowFile, attributes);
-                    session.getProvenanceReporter().receive(outgoingFlowFile, getURI(context));
+                    String uriPass="";


Please add spaces around the equals sing.

HorizonNet · 2020-04-27T10:32:07Z

+
+        Assert.assertTrue(parsed.get("date_field").getClass() == String.class);
+        Assert.assertTrue(((String) parsed.get("date_field")).startsWith(format.format(CAL.getTime())));
+


Remove empty line.

HorizonNet · 2020-04-27T10:32:15Z

+        Assert.assertTrue(((String) parsed.get("date_field")).startsWith(format.format(CAL.getTime())));
+
+    }
+


alopresto · 2020-04-28T00:41:16Z

            .build();
+    static final PropertyDescriptor USER_NAME = new PropertyDescriptor.Builder()
+            .name("User Name")
+            .displayName("username")


I think the name and displayName values are switched.

Thank you for pointing it out. I will add it

alopresto · 2020-04-28T00:41:57Z

+            .build();
+
+    static final PropertyDescriptor PASSWORD = new PropertyDescriptor.Builder()
+            .name("Password")


The name value should match the formatting of the username field and a displayName should also be present.

alopresto · 2020-04-28T00:49:49Z

-                    session.getProvenanceReporter().receive(outgoingFlowFile, getURI(context));
+                    String uriPass = "";
+                    if (context.getProperty(USER_NAME).getValue() != null) {
+                        uriPass = "mongodb://" + context.getProperty(USER_NAME).getValue() + ":" + context.getProperty(PASSWORD).getValue() + "@" + getURI(context).substring(10);


I don't believe the URI is validated anywhere to ensure it starts with mongodb://, so arbitrarily starting the index at 10 isn't guaranteed to produce the expected outcome.

Also, there is a note in the MongoDB docs regarding usernames and passwords that contain special characters:

If the username or password includes the at sign @, colon :, slash /, or the percent sign % character, use percent encoding.

I will add in the validation for the starting 10 characters for the URI.

I did see the MongoDB doc, for percent encoding which would be better:
a) forcing the user to handle the percent encoding and printing it in the description as a note for user.
b) explicitly handling of it by the processor.

current implementation is goes the first way. But if you think It is better for processor to handle, I will look into it.

This is not something I would expect the user to do manually. It should be done automatically by the processor.

alopresto · 2020-04-28T00:53:17Z

    }
+
+    @OnScheduled
+    public void createClient(ProcessContext context) throws IOException {


Most of this method appears to duplicate AbstractMongoProcessor#createClient(). I suggest the duplicated code should be refactored.

It is true I could have done the changes in AbstractMongoProcessor, but it would end up affecting the components of PutMongo. Also Since request was for just getMongo I did it this way.

I think all 6 of the MongoDB processors need this change if one does, so it should be made in a consistent manner. It appears the Jira is incomplete as it only indicates the GetMongo processor, but that's what this review process is intended to mitigate. Requirements gathering is notoriously difficult and a single user's request has to be balanced against the ongoing needs of the project as a whole. Any code changes introduced need to be well-understood, complete, tested, sustainable, and maintainable. I use the analogy from camping of "leave it better than you found it."

@alopresto We started moving toward using a controller service to manage the client connection I think ~3 releases ago. The best place for these changes might be there, so we can start moving toward deprecating per-processor connection management altogether.

alopresto · 2020-04-28T00:59:11Z

It looks like the PutMongo and DeleteMongo processors (as well as GetMongoRecord, PutMongoRecord, and DeleteMongoRecord) could also take advantage of these new property descriptors, and therefore the better location for them is in AbstractMongoProcessor.

MikeThomsen · 2020-05-21T00:03:04Z

@karthik-kadajji It would be easier to apply this change to the Mongo controller service. If you look at all of the processors, you'll see that they have "client service" as an option. In the long run, I'm planning to start deprecating the current configuration options and have the Mongo processors use that because it matches our design patterns better.

MikeThomsen

@karthik-kadajji I'm -1 on merging because this needs to be in the controller service, as the goal is to deprecate the configuration fields in each processor in favor of using a controller service to act as the connection pool.

github-actions · 2021-04-25T00:15:53Z

We're marking this PR as stale due to lack of updates in the past few months. If after another couple of weeks the stale label has not been removed this PR will be closed. This stale marker and eventual auto close does not indicate a judgement of the PR just lack of reviewer bandwidth and helps us keep the PR queue more manageable. If you would like this PR re-opened you can do so and a committer can remove the stale tag. Or you can open a new PR. Try to help review other PRs to increase PR review bandwidth which in turn helps yours.

NIFI-7037 Split off username and password fields for GetMongo processor

f88dcdc

HorizonNet reviewed Apr 24, 2020

View reviewed changes

karthik-kadajji added 2 commits April 27, 2020 10:22

NIFI-7037 Split off username and password fields for GetMongo processor

a7edf6a

NIFI-7037 Split off username and password fields for GetMongo processor

546450f

HorizonNet reviewed Apr 27, 2020

View reviewed changes

karthik-kadajji added 2 commits April 27, 2020 13:52

NIFI-7037 Split off username and password fields for GetMongo processor

ae4d4d7

NIFI-7037 Split off username and password fields for GetMongo processor

1db5d20

HorizonNet approved these changes Apr 27, 2020

View reviewed changes

alopresto reviewed Apr 28, 2020

View reviewed changes

MikeThomsen requested changes Jul 6, 2020

View reviewed changes

github-actions Bot added the Stale label Apr 25, 2021

github-actions Bot closed this May 11, 2021


		Assert.assertTrue(parsed.get("date_field").getClass() == String.class);
		Assert.assertTrue(((String) parsed.get("date_field")).startsWith(format.format(CAL.getTime())));

Conversation

karthik-kadajji commented Apr 24, 2020

Description of PR

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HorizonNet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karthik-kadajji Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alopresto commented Apr 28, 2020

Uh oh!

MikeThomsen commented May 21, 2020

Uh oh!

MikeThomsen left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

karthik-kadajji Apr 28, 2020 •

edited

Loading