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 2019/12/30 00:06:47 UTC
[GitHub] [nifi] MikeThomsen opened a new pull request #3953: NIFI-5901 Added
JSON/JSONB support to PutDatabaseRecord
MikeThomsen opened a new pull request #3953: NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
URL: https://github.com/apache/nifi/pull/3953
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] mattyb149 commented on a change in pull request #3953:
NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #3953: NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
URL: https://github.com/apache/nifi/pull/3953#discussion_r363446188
##########
File path: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/pattern/Put.java
##########
@@ -89,7 +89,7 @@ public void onTrigger(ProcessContext context, ProcessSession session, FC functio
session.transfer(routedFlowFiles, relationship)));
if (flowFiles == null || flowFiles.isEmpty()) {
- logger.debug("No incoming FlowFiles.");
+ //logger.debug("No incoming FlowFiles.");
Review comment:
This should probably be restored as it appears to be useful for debugging live flows
----------------------------------------------------------------
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] mattyb149 commented on a change in pull request #3953:
NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #3953: NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
URL: https://github.com/apache/nifi/pull/3953#discussion_r363445186
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
##########
@@ -252,6 +258,16 @@
.defaultValue("false")
.build();
+ static final PropertyDescriptor MAP_RECORD_TO_JSON = new PropertyDescriptor.Builder()
+ .name("put-db-record-map-record-to-json")
+ .displayName("Map \"record\" types to JSON")
Review comment:
What if we made this a dropdown with allowable values vs booleans? It appears that setting the JDBC type to `OTHER` supports both JSON and JSONB (at least for PostgreSQL), but in the future we may want to support the SQLXML type for inserting into XML columns and so on. The description of the `JSON` option should probably explicitly say that the JDBC type is set to `OTHER`, as that might be helpful information for other databases that do things differently.
----------------------------------------------------------------
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 #3953:
NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on a change in pull request #3953: NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
URL: https://github.com/apache/nifi/pull/3953#discussion_r363913788
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
##########
@@ -252,6 +258,16 @@
.defaultValue("false")
.build();
+ static final PropertyDescriptor MAP_RECORD_TO_JSON = new PropertyDescriptor.Builder()
+ .name("put-db-record-map-record-to-json")
+ .displayName("Map \"record\" types to JSON")
Review comment:
The JSON support is implemented using the MapRecord API that converts the Record to JSON without having to use a record writer. Since nothing like that exists for XML, how do you want to handle it? I suppose we could allow a writer to be specified and have the writer simply take the schema from the Record field.
----------------------------------------------------------------
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] mattyb149 commented on a change in pull request #3953:
NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #3953: NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
URL: https://github.com/apache/nifi/pull/3953#discussion_r364540127
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
##########
@@ -252,6 +258,16 @@
.defaultValue("false")
.build();
+ static final PropertyDescriptor MAP_RECORD_TO_JSON = new PropertyDescriptor.Builder()
+ .name("put-db-record-map-record-to-json")
+ .displayName("Map \"record\" types to JSON")
Review comment:
I don't see where MapRecord converts Record to JSON, I thought you were using Jackson's ObjectMapper for that? For XML I'm leery of JAXB as I guess that has Java 9+ and/or classpath issues, but I wonder if XStream is an option. Also I don't know if external systems take YAML these days but the idea was to offer the capability to support various structured data formats. If everything but JSON is a non-starter then I'm fine with leaving it a boolean.
----------------------------------------------------------------
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 #3953:
NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on a change in pull request #3953: NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
URL: https://github.com/apache/nifi/pull/3953#discussion_r364748167
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
##########
@@ -252,6 +258,16 @@
.defaultValue("false")
.build();
+ static final PropertyDescriptor MAP_RECORD_TO_JSON = new PropertyDescriptor.Builder()
+ .name("put-db-record-map-record-to-json")
+ .displayName("Map \"record\" types to JSON")
Review comment:
What I meant was, I use `toMap` to convert to a vanilla Java `Map` and then use Jackson for that. Come to think of it, I probably could support XML that way...
----------------------------------------------------------------
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 #3953: NIFI-5901 Added
JSON/JSONB support to PutDatabaseRecord
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on issue #3953: NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
URL: https://github.com/apache/nifi/pull/3953#issuecomment-574383067
@mattyb149 did some experimentation with the jackson mapper, and I don't think it's a very good solution for some of the XML use cases that might arise here. Not sure what the right solution for XML is, but I think sticking with JSON is the right call for now.
Do you know of any cases where not automatically treating `Types.OTHER` from JDBC as JSON would be the right call?
----------------------------------------------------------------
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] mattyb149 commented on a change in pull request #3953:
NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #3953: NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
URL: https://github.com/apache/nifi/pull/3953#discussion_r363445914
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
##########
@@ -1042,7 +1072,22 @@ private static String normalizeColumnName(final String colName, final boolean tr
return colName == null ? null : (translateColumnNames ? colName.toUpperCase().replace("_", "") : colName);
}
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+
+ private static String convertMapRecord(Record record) {
Review comment:
If we use a set of options for what to convert the record to (see my other comment), this method should be named accordingly for converting to a JSON string, as we would need other similar methods for other representations such as XML.
----------------------------------------------------------------
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 #3953:
NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on a change in pull request #3953: NIFI-5901 Added JSON/JSONB support to PutDatabaseRecord
URL: https://github.com/apache/nifi/pull/3953#discussion_r364748950
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutDatabaseRecord.java
##########
@@ -252,6 +258,16 @@
.defaultValue("false")
.build();
+ static final PropertyDescriptor MAP_RECORD_TO_JSON = new PropertyDescriptor.Builder()
+ .name("put-db-record-map-record-to-json")
+ .displayName("Map \"record\" types to JSON")
Review comment:
Going to dig into this and see if there's a way to detect what the real type of the field is and apply this automagically (json vs xml as appropriate):
https://github.com/FasterXML/jackson-dataformat-xml
----------------------------------------------------------------
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