You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by MikeThomsen <gi...@git.apache.org> on 2018/06/09 19:38:32 UTC
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
GitHub user MikeThomsen opened a pull request:
https://github.com/apache/nifi/pull/2778
NIFI-5288 Quietly convert Java arrays to Lists so the MongoDB API can…
… handle them.
Thank you for submitting a contribution to Apache NiFi.
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 NIFI-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 master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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?
- [ ] 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MikeThomsen/nifi NIFI-5288
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2778.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2778
----
commit 646a52fe8f1f1fb473eef42488c106ddfd825cf1
Author: Mike Thomsen <mi...@...>
Date: 2018-06-09T19:38:02Z
NIFI-5288 Quietly convert Java arrays to Lists so the MongoDB API can handle them.
----
---
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2778#discussion_r194239526
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/PutMongoRecord.java ---
@@ -131,7 +131,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
for (String name : schema.getFieldNames()) {
--- End diff --
Why not do the conversion while inserting?
---
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2778
---
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2778#discussion_r194241356
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/test/java/org/apache/nifi/processors/mongodb/PutMongoRecordIT.java ---
@@ -191,4 +197,33 @@ public void testInsertNestedRecords() throws Exception {
assertEquals(4, collection.count());
//assertEquals(doc, collection.find().first());
}
+
+ @Test
+ public void testArrayConversion() throws Exception {
+ TestRunner runner = init(PutMongoRecord.class);
+ MockSchemaRegistry registry = new MockSchemaRegistry();
+ String rawSchema = "{\"type\":\"record\",\"name\":\"Test\",\"fields\":[{\"name\":\"nom\",\"type\":\"string\"," +
--- End diff --
Let's get rid of the "doc" fields here, I'm sure just pasted from from the example use case that illustrated the bug, but still quite unnecessary and tends to clutter up the test.
---
[GitHub] nifi issue #2778: NIFI-5288 Quietly convert Java arrays to Lists so the Mong...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2778
@mattyb149 I think everything you want is here now.
---
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2778#discussion_r194241336
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/PutMongoRecord.java ---
@@ -131,7 +131,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
for (String name : schema.getFieldNames()) {
document.put(name, contentMap.get(name));
}
- inserts.add(document);
+ inserts.add(convertArrays(document));
--- End diff --
For future-proofing, perhaps this method should be called something like normalizeFields() or something?
---
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2778#discussion_r194241805
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/test/java/org/apache/nifi/processors/mongodb/PutMongoRecordIT.java ---
@@ -191,4 +197,33 @@ public void testInsertNestedRecords() throws Exception {
assertEquals(4, collection.count());
//assertEquals(doc, collection.find().first());
}
+
+ @Test
+ public void testArrayConversion() throws Exception {
+ TestRunner runner = init(PutMongoRecord.class);
+ MockSchemaRegistry registry = new MockSchemaRegistry();
+ String rawSchema = "{\"type\":\"record\",\"name\":\"Test\",\"fields\":[{\"name\":\"nom\",\"type\":\"string\"," +
--- End diff --
Yeah, good idea. I'll change them up.
---
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2778#discussion_r194241017
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/PutMongoRecord.java ---
@@ -131,7 +131,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
for (String name : schema.getFieldNames()) {
--- End diff --
Mainly preference. When went back and looked at it, a single recursive function seemed like the right call. I still think it is. Not much to be saved by mixing it into that loop IMO.
---
[GitHub] nifi issue #2778: NIFI-5288 Quietly convert Java arrays to Lists so the Mong...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2778
+1 LGTM, I couldn't find anything promising about arrays of arrays in Avro, I guess in order to determine types using an Avro schema, it has to have a name, which means it has to be a record or something, so you can have arrays of records of arrays or something...
Anyway ran with integration tests and with a live NiFi, things look good. Thanks for the improvement! Merging to master
---
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2778#discussion_r194241375
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/test/java/org/apache/nifi/processors/mongodb/PutMongoRecordIT.java ---
@@ -191,4 +197,33 @@ public void testInsertNestedRecords() throws Exception {
assertEquals(4, collection.count());
//assertEquals(doc, collection.find().first());
}
+
+ @Test
+ public void testArrayConversion() throws Exception {
+ TestRunner runner = init(PutMongoRecord.class);
+ MockSchemaRegistry registry = new MockSchemaRegistry();
+ String rawSchema = "{\"type\":\"record\",\"name\":\"Test\",\"fields\":[{\"name\":\"nom\",\"type\":\"string\"," +
--- End diff --
Also I think let's change the names of fields and values unless we have permission from the original user to use such test data.
---
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2778#discussion_r194241320
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/PutMongoRecord.java ---
@@ -154,4 +154,28 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
}
session.commit();
}
+
+ private Document convertArrays(Document doc) {
+ Document retVal = new Document();
+ for (Map.Entry<String, Object> entry : doc.entrySet()) {
+ if (entry.getValue() != null && entry.getValue().getClass().isArray()) {
+ List items = new ArrayList();
+ Object[] values = (Object[])entry.getValue();
+ for (int index = 0; index < values.length; index++) {
--- End diff --
I haven't tried this yet, but does this handle arrays of arrays? If not, should it?
---
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2778#discussion_r194242476
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/PutMongoRecord.java ---
@@ -154,4 +154,28 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
}
session.commit();
}
+
+ private Document convertArrays(Document doc) {
+ Document retVal = new Document();
+ for (Map.Entry<String, Object> entry : doc.entrySet()) {
+ if (entry.getValue() != null && entry.getValue().getClass().isArray()) {
+ List items = new ArrayList();
+ Object[] values = (Object[])entry.getValue();
+ for (int index = 0; index < values.length; index++) {
--- End diff --
Saw mostly variations of [this](http://apache-avro.679487.n3.nabble.com/Avro-schema-for-nested-arrays-doesn-t-work-td1455659.html).
---
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2778#discussion_r194241786
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/PutMongoRecord.java ---
@@ -131,7 +131,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
for (String name : schema.getFieldNames()) {
document.put(name, contentMap.get(name));
}
- inserts.add(document);
+ inserts.add(convertArrays(document));
--- End diff --
I prefer convertArrays because it ties back to the ticket and AFAIK this is the only area where the Record API throws Mongo's client for a loop on the fields.
---
[GitHub] nifi issue #2778: NIFI-5288 Quietly convert Java arrays to Lists so the Mong...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2778
Because 5145, the integration tests might not completely run if run as a suite. I have verified that PutMongoRecordIT is not affected by 5145 and can be run by itself with a Docker image of Mongo using defaults:
`docker run -d --name mongo -p 27017:27017 mongo:latest`
---
[GitHub] nifi pull request #2778: NIFI-5288 Quietly convert Java arrays to Lists so t...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2778#discussion_r194242453
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/PutMongoRecord.java ---
@@ -154,4 +154,28 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
}
session.commit();
}
+
+ private Document convertArrays(Document doc) {
+ Document retVal = new Document();
+ for (Map.Entry<String, Object> entry : doc.entrySet()) {
+ if (entry.getValue() != null && entry.getValue().getClass().isArray()) {
+ List items = new ArrayList();
+ Object[] values = (Object[])entry.getValue();
+ for (int index = 0; index < values.length; index++) {
--- End diff --
I just made a change that passes validation with the original schema and that theoretically supports nested arrays. Thing is, from what I can tell you can't even express this in Avro:
```
{
"arrayTest": [
[[0,1], [2,3]]
]
}
```
---