You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by zenfenan <gi...@git.apache.org> on 2018/06/04 17:27:53 UTC
[GitHub] nifi pull request #2758: NIFI-5261: Added JSON_VALIDATOR to StandardValidato...
GitHub user zenfenan opened a pull request:
https://github.com/apache/nifi/pull/2758
NIFI-5261: Added JSON_VALIDATOR to StandardValidators
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:
- [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?
### 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?
- [x] 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/zenfenan/nifi NIFI-5261
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2758.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 #2758
----
commit 8fca7bca325d1bcabfdeb1a3903ed893f6dfcb50
Author: zenfenan <si...@...>
Date: 2018-06-04T17:18:38Z
NIFI-5261: Added JSON_VALIDATOR to StandardValidators
----
---
[GitHub] nifi pull request #2758: NIFI-5261: Added JSON_VALIDATOR to StandardValidato...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2758#discussion_r193373467
--- Diff: nifi-commons/nifi-utils/pom.xml ---
@@ -40,5 +40,10 @@
<artifactId>nifi-api</artifactId>
<version>1.7.0-SNAPSHOT</version>
</dependency>
+ <dependency>
--- End diff --
Yeah, NP and it'll make my life easier with the L&N.
---
[GitHub] nifi pull request #2758: NIFI-5261: Added JSON_VALIDATOR to StandardValidato...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2758
---
[GitHub] nifi pull request #2758: NIFI-5261: Added JSON_VALIDATOR to StandardValidato...
Posted by zenfenan <gi...@git.apache.org>.
Github user zenfenan commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2758#discussion_r193272550
--- Diff: nifi-commons/nifi-utils/pom.xml ---
@@ -40,5 +40,10 @@
<artifactId>nifi-api</artifactId>
<version>1.7.0-SNAPSHOT</version>
</dependency>
+ <dependency>
--- End diff --
@mattyb149 I agree.
@MikeThomsen Can you cover this in NIFI-5271 i.e. scrapping GSON and making use of Jackson?
---
[GitHub] nifi pull request #2758: NIFI-5261: Added JSON_VALIDATOR to StandardValidato...
Posted by zenfenan <gi...@git.apache.org>.
Github user zenfenan commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2758#discussion_r193081777
--- Diff: nifi-commons/nifi-utils/src/test/java/org/apache/nifi/util/validator/TestStandardValidators.java ---
@@ -288,4 +288,42 @@ public void testiso8061InstantValidator() {
vr = val.validate("foo", "2016-01-01T01:01:01.000Z", vc);
assertTrue(vr.isValid());
}
+
--- End diff --
Looks interesting. I'll try to make use of this in my future contributions.
---
[GitHub] nifi pull request #2758: NIFI-5261: Added JSON_VALIDATOR to StandardValidato...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2758#discussion_r192826557
--- Diff: nifi-commons/nifi-utils/pom.xml ---
@@ -40,5 +40,10 @@
<artifactId>nifi-api</artifactId>
<version>1.7.0-SNAPSHOT</version>
</dependency>
+ <dependency>
--- End diff --
Jackson is used in more packages than Gson, so I think you should switch over to that unless you have a compelling reason.
---
[GitHub] nifi pull request #2758: NIFI-5261: Added JSON_VALIDATOR to StandardValidato...
Posted by zenfenan <gi...@git.apache.org>.
Github user zenfenan commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2758#discussion_r193082747
--- Diff: nifi-commons/nifi-utils/pom.xml ---
@@ -40,5 +40,10 @@
<artifactId>nifi-api</artifactId>
<version>1.7.0-SNAPSHOT</version>
</dependency>
+ <dependency>
--- End diff --
I feel more convenient using Gson. I think it should be fine in this case since we are just trying to find if there are JSON syntax issues and not using some core serialization/deserialization that JSON libraries (Jackson, etc.,) offer.
---
[GitHub] nifi pull request #2758: NIFI-5261: Added JSON_VALIDATOR to StandardValidato...
Posted by zenfenan <gi...@git.apache.org>.
Github user zenfenan commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2758#discussion_r192820713
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/GetMongo.java ---
@@ -60,23 +59,6 @@
@InputRequirement(Requirement.INPUT_ALLOWED)
@CapabilityDescription("Creates FlowFiles from documents in MongoDB")
public class GetMongo extends AbstractMongoProcessor {
- public static final Validator DOCUMENT_VALIDATOR = (subject, value, context) -> {
--- End diff --
@MikeThomsen I have replaced this one as well as `AGG_VALIDATOR` with the new `JSON_VALIDATOR`. I have ensured that the tests run fine. Also run a sample flow on a live MongoDB instance. Everything worked fine. Let me know, if you find anything odd.
---
[GitHub] nifi pull request #2758: NIFI-5261: Added JSON_VALIDATOR to StandardValidato...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2758#discussion_r192825698
--- Diff: nifi-commons/nifi-utils/src/test/java/org/apache/nifi/util/validator/TestStandardValidators.java ---
@@ -288,4 +288,42 @@ public void testiso8061InstantValidator() {
vr = val.validate("foo", "2016-01-01T01:01:01.000Z", vc);
assertTrue(vr.isValid());
}
+
--- End diff --
This would be a great use case for Groovy instead of Java. I've started doing that with my unit tests because you can specify the JSON like this:
```
import static groovy.json.JsonOutput.*
def json = prettyPrint(toJson([
Name: "Crockford, Douglas"
]))
```
Not required, but worth thinking about because it's a lot cleaner and Groovy is allowed in tests.
---
[GitHub] nifi pull request #2758: NIFI-5261: Added JSON_VALIDATOR to StandardValidato...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2758#discussion_r193272119
--- Diff: nifi-commons/nifi-utils/pom.xml ---
@@ -40,5 +40,10 @@
<artifactId>nifi-api</artifactId>
<version>1.7.0-SNAPSHOT</version>
</dependency>
+ <dependency>
--- End diff --
There are some things to consider when using Gson over Jackson and in general for this PR, this is related to the point I mentioned on a different PR about where to put a "well-formed JSON" validator:
1) Performance (https://blog.takipi.com/the-ultimate-json-library-json-simple-vs-gson-vs-jackson-vs-json/)
2) The bundle already has Jackson (such as the Mongo bundle)
3) Increased NAR size (Gson is only ~23K so minor but 84 current bundles use nifi-utils and only 1-2 were updated to use the validator)
@zenfenan I agree that on the surface, Gson takes care of this issue conveniently and with a lightweight footprint. However because of the reasons above, it seems this could be problematic, and we should consider a different approach regardless of convenience.
---