You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/02/21 16:40:23 UTC
[GitHub] [nifi] eduardofontes opened a new pull request #4068: NIFI-6293 -
Add support to Mongo Extended JSON to RunMongoAggregation Query
eduardofontes opened a new pull request #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068
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:
- [ ] 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? _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` folder?
- [ ] 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](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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] eduardofontes commented on a change in pull request #4068:
NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
eduardofontes commented on a change in pull request #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#discussion_r382779031
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/pom.xml
##########
@@ -116,5 +116,10 @@
<version>${nifi.groovy.version}</version>
<scope>test</scope>
</dependency>
- </dependencies>
+ <dependency>
+ <groupId>org.json</groupId>
+ <artifactId>json</artifactId>
+ <version>20180813</version>
Review comment:
@pvillard31 evil json replaced.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] asfgit closed pull request #4068: NIFI-6293 - Add support to
Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] MikeThomsen commented on issue #4068: NIFI-6293 - Add
support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on issue #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#issuecomment-599209372
@eduardofontes FYI, the reason I'm pushing back on adding a new json parser JAR to the nar file is because space is getting to be a premium in our convenience binaries. We're getting a lot of push back as a team from apache infra because our binaries are so big.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] MikeThomsen commented on a change in pull request #4068:
NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on a change in pull request #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#discussion_r392672831
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/test/java/org/apache/nifi/processors/mongodb/RunMongoAggregationIT.java
##########
@@ -124,6 +124,33 @@ public void testAggregation() throws Exception {
}
}
+ @Test
+ public void testExtendedJsonSupport() throws Exception {
Review comment:
You should probably seed the collection with test data that explicitly tests the behavior.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] pvillard31 commented on issue #4068: NIFI-6293 - Add support
to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on issue #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#issuecomment-598734854
@MikeThomsen - do you mind having a look?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] eduardofontes commented on a change in pull request #4068:
NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
eduardofontes commented on a change in pull request #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#discussion_r382738579
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/pom.xml
##########
@@ -116,5 +116,10 @@
<version>${nifi.groovy.version}</version>
<scope>test</scope>
</dependency>
- </dependencies>
+ <dependency>
+ <groupId>org.json</groupId>
+ <artifactId>json</artifactId>
+ <version>20180813</version>
Review comment:
But it's for GOOD 👼
Ok! I'll look for an alternative. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] eduardofontes commented on a change in pull request #4068:
NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
eduardofontes commented on a change in pull request #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#discussion_r392683240
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/RunMongoAggregation.java
##########
@@ -73,11 +78,17 @@
static final List<Bson> buildAggregationQuery(String query) throws IOException {
List<Bson> result = new ArrayList<>();
+ JSONParser jsonParser = new JSONParser();
Review comment:
I don't think so. It was just the first I've found.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] eduardofontes commented on issue #4068: NIFI-6293 - Add
support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
eduardofontes commented on issue #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#issuecomment-599225667
>
>
> @eduardofontes FYI, the reason I'm pushing back on adding a new json parser JAR to the nar file is because space is getting to be a premium in our convenience binaries. We're getting a lot of push back as a team from apache infra because our binaries are so big.
Ok. I understand. It's because my lack of knowledge in Jackson. But I'll try to refactor and not use a new JAR in this NAR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] eduardofontes commented on issue #4068: NIFI-6293 - Add
support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
eduardofontes commented on issue #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#issuecomment-592299212
>
>
> Could you add a unit test or update the integration tests to confirm it fixes https://issues.apache.org/jira/browse/NIFI-6293 ?
Ok. I've create a method "testExtendedJsonSupport" in RunMongoAggregationIT.java where a aggregation query with "$date" extended JSON notation must return values. If return none then extended JSON notation wasn't be interpreted correctly (it was used as simples strings literals).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] pvillard31 edited a comment on issue #4068: NIFI-6293 - Add
support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
pvillard31 edited a comment on issue #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#issuecomment-589823457
Could you add a unit test or update the integration tests to confirm it fixes https://issues.apache.org/jira/browse/NIFI-6293 ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] pvillard31 commented on issue #4068: NIFI-6293 - Add support
to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on issue #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#issuecomment-589823457
Could add a unit test or update the integration tests to confirm it fixes https://issues.apache.org/jira/browse/NIFI-6293 ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] MikeThomsen commented on a change in pull request #4068:
NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on a change in pull request #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#discussion_r392672445
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/RunMongoAggregation.java
##########
@@ -73,11 +78,17 @@
static final List<Bson> buildAggregationQuery(String query) throws IOException {
List<Bson> result = new ArrayList<>();
+ JSONParser jsonParser = new JSONParser();
Review comment:
What's the reason for using this vs Jackson? We try to avoid personal preference in favor of Jackson wherever possible. Is there a deficiency in Jackson?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] eduardofontes commented on a change in pull request #4068:
NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
eduardofontes commented on a change in pull request #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#discussion_r392684776
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/test/java/org/apache/nifi/processors/mongodb/RunMongoAggregationIT.java
##########
@@ -124,6 +124,33 @@ public void testAggregation() throws Exception {
}
}
+ @Test
+ public void testExtendedJsonSupport() throws Exception {
Review comment:
Sorry, I don't understood what did you mean. This test queries by date using Extended JSON "$date" notation. There already has a date field in collection seeded by "setup()". What do you suggest exactaly?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] eduardofontes commented on issue #4068: NIFI-6293 - Add
support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
eduardofontes commented on issue #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#issuecomment-599265743
@MikeThomsen , I've make it work with Jackson and I've fixed a timezone issue in test query.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] pvillard31 commented on a change in pull request #4068:
NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on a change in pull request #4068: NIFI-6293 - Add support to Mongo Extended JSON to RunMongoAggregation Query
URL: https://github.com/apache/nifi/pull/4068#discussion_r382723574
##########
File path: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/pom.xml
##########
@@ -116,5 +116,10 @@
<version>${nifi.groovy.version}</version>
<scope>test</scope>
</dependency>
- </dependencies>
+ <dependency>
+ <groupId>org.json</groupId>
+ <artifactId>json</artifactId>
+ <version>20180813</version>
Review comment:
I believe we don't want to use this library because of the licensing.
https://jira.apache.org/jira/browse/LEGAL-245
to be confirmed though - I believe there are accepted alternatives
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services