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 2021/01/13 17:06:57 UTC
[GitHub] [nifi] ChrisSamo632 opened a new pull request #4755: NIFI-8133 Add ability to supress null/empty values in ElasticSearchCl…
ChrisSamo632 opened a new pull request #4755:
URL: https://github.com/apache/nifi/pull/4755
…ientService for PutElasticsearchRecord output
#### Description of PR
Ability to suppress null/empty values when outputting data to Elasticsearch with PutElasticsearchRecord (fixes NIFI-8133)
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 `main`)?
- [ 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 JDK 8?~
- [ x ] Have you verified that the full build is successful on 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`?~
- [ 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 GitHub Actions 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
[GitHub] [nifi] mattyb149 commented on a change in pull request #4755: NIFI-8133 Add ability to suppress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560577910
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java
##########
@@ -90,6 +93,15 @@ public void onEnabled(final ConfigurationContext context) throws InitializationE
try {
setupClient(context);
responseCharset = Charset.forName(context.getProperty(CHARSET).getValue());
+
+ // re-create the ObjectMapper in case the SUPPRESS_NULLS property has changed - the JsonInclude settings aren't dynamic
+ mapper = new ObjectMapper();
+ if (ALWAYS_SUPPRESS.getValue().equals(context.getProperty(SUPPRESS_NULLS).getValue())) {
+ mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
Review comment:
I tried the same thing with ES 7.9.3 and I get the same results. I even used an explicit schema saying `y` is a nullable field, but when I sent in a document with `"y": null` and then query it with `curl 'http://localhost:9200/nulltest/_search?q=*'`, I get no `y` element in that doc. Integration test passes (when using Maven) though...
----------------------------------------------------------------
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
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #4755: NIFI-8133 Add ability to supress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560474183
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java
##########
@@ -90,6 +93,15 @@ public void onEnabled(final ConfigurationContext context) throws InitializationE
try {
setupClient(context);
responseCharset = Charset.forName(context.getProperty(CHARSET).getValue());
+
+ // re-create the ObjectMapper in case the SUPPRESS_NULLS property has changed - the JsonInclude settings aren't dynamic
+ mapper = new ObjectMapper();
+ if (ALWAYS_SUPPRESS.getValue().equals(context.getProperty(SUPPRESS_NULLS).getValue())) {
+ mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
Review comment:
You are correct, I would expect the `"y": null` output when using `NEVER_SUPPRESS` (but not for `ALWAYS_SUPPRESS`).
I've included this as a test in the `ElasticSearch5ClientService_IT` as `testNullSuppression`, which involves:
* creating a document with `NEVER_SUPPRESS` that contains and asserts the inserted document matches:
* "is_null": null
* "is_empty": empty ("")
* "is_blank": blank (" ")
* "msg": "test"
* creating the same document with `ALWAYS_SUPPRESS` then asserting that the document *does not* contain the `"is_null"` or `"is_empty"` fields
Switching between the two states involves disabling the `ElasticSearchClientService`, changing the `SUPPRESS_NULLS` property, then re-enabling the service before using it to to `bulk` index the document then `get` it back from Elasticsearch.
How are you running the code and checking the output if not via the tests?
----------------------------------------------------------------
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
[GitHub] [nifi] mattyb149 closed pull request #4755: NIFI-8133 Add ability to suppress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
mattyb149 closed pull request #4755:
URL: https://github.com/apache/nifi/pull/4755
--
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
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #4755: NIFI-8133 Add ability to suppress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560717188
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java
##########
@@ -90,6 +93,15 @@ public void onEnabled(final ConfigurationContext context) throws InitializationE
try {
setupClient(context);
responseCharset = Charset.forName(context.getProperty(CHARSET).getValue());
+
+ // re-create the ObjectMapper in case the SUPPRESS_NULLS property has changed - the JsonInclude settings aren't dynamic
+ mapper = new ObjectMapper();
+ if (ALWAYS_SUPPRESS.getValue().equals(context.getProperty(SUPPRESS_NULLS).getValue())) {
+ mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
Review comment:
I agree, for some reason the behaviour of the controller service once built is different to that in the integration tests (which is kind of disturbing in several ways).
Will investigate the lack of `null` values when using the `NEVER_SUPPRESS` option. If possible, I'll also try to see whether there's a general issue with the integration tests.
----------------------------------------------------------------
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
[GitHub] [nifi] mattyb149 commented on pull request #4755: NIFI-8133 Add ability to suppress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#issuecomment-850869384
+1 LGTM, ran the integration test(s) and verified the expected behavior. Thanks for the improvement! Merging to main
--
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
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #4755: NIFI-8133 Add ability to suppress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r565542497
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java
##########
@@ -90,6 +93,15 @@ public void onEnabled(final ConfigurationContext context) throws InitializationE
try {
setupClient(context);
responseCharset = Charset.forName(context.getProperty(CHARSET).getValue());
+
+ // re-create the ObjectMapper in case the SUPPRESS_NULLS property has changed - the JsonInclude settings aren't dynamic
+ mapper = new ObjectMapper();
+ if (ALWAYS_SUPPRESS.getValue().equals(context.getProperty(SUPPRESS_NULLS).getValue())) {
+ mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
Review comment:
@mattyb149 managed to confirm that this is a problem for my flow in 1.11.4 when outputting data to Elasticsearch that is a Record containing nested Records with Arrays or Records in them... the fields within the nested Records come through as `null` when that's not wanted. If it makes a difference, we're using Avro as the data format between processors in this Flow.
I think the logic in PutElasticsearchRecord is missing an if-else to handle `Object[]` as well as `List` and/or other structures (the `DataTypeUtils` to convert between NiFi Record structure and "plain" Java Objects is a little flawed with some of these conversions - I've recently raised separate Jira tickets documenting some of those).
The switch to using the Jackson-based null/empty suppression should allow for better null field control.
----------------------------------------------------------------
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
[GitHub] [nifi] github-actions[bot] commented on pull request #4755: NIFI-8133 Add ability to suppress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#issuecomment-850023143
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.
--
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
[GitHub] [nifi] mattyb149 commented on a change in pull request #4755: NIFI-8133 Add ability to supress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560392427
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/pom.xml
##########
@@ -196,7 +196,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-failsafe-plugin</artifactId>
- <version>3.0.0-M5</version>
+ <version>3.0.0-M3</version>
Review comment:
Why the downgrade here?
----------------------------------------------------------------
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
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #4755: NIFI-8133 Add ability to suppress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560819377
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java
##########
@@ -90,6 +93,15 @@ public void onEnabled(final ConfigurationContext context) throws InitializationE
try {
setupClient(context);
responseCharset = Charset.forName(context.getProperty(CHARSET).getValue());
+
+ // re-create the ObjectMapper in case the SUPPRESS_NULLS property has changed - the JsonInclude settings aren't dynamic
+ mapper = new ObjectMapper();
+ if (ALWAYS_SUPPRESS.getValue().equals(context.getProperty(SUPPRESS_NULLS).getValue())) {
+ mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
Review comment:
I couldn't re-produce the `null` scenario I'm sure I saw while trying to use the PutElasticsearchRecord processor in 1.11.4.
However, I do still think there's something to change here, in that you can't currently output `null`/empty values to Elasticsearch using this processor (so kind of the reverse of the original ticket) due to the `removeEmpty` method in the `PutElasticsearchRecord` processor. Looking at this method, it could also result in empty nested objects/arrays being sent to Elasticsearch currently, which wouldn't always be ideal.
So, I've changed the code to:
* Not `removeEmpty` within `PutElasticsearchRecord`
* Use the Jackson ObjectMapper to remove `null`/empty fields (including empty nested arrays/objects) when `ALWAYS_SUPPRESS` is selected
* Leave the `null`/empty fields in the output to Elasticsearch when `NEVER_SUPPRESS` is selected
I've added a Flow template/export to the Jira ticket (included converting the data into Avro as that's what I use in the Flow where I believe I saw this behaviour in 1.11.4 before switching to the `PutElasticsearchHttpRecord` instead - although that didn't cause a re-production of the issue either).
Re-based the branch from `main` and updated both unit and integration tests to cover these scenarios. Default behaviour of the `PutElasticsearchRecord` processor remains one of removing `null`/empty fields (i.e. `ALWAYS_SUPPRESS` in `ElasticSearchClientService`). I think my concerns with the integration tests above are explained by the fact that testing the service directly wasn't executing the `remoteEmpty` method of the processor.
----------------------------------------------------------------
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
[GitHub] [nifi] mattyb149 commented on a change in pull request #4755: NIFI-8133 Add ability to supress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560510439
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java
##########
@@ -90,6 +93,15 @@ public void onEnabled(final ConfigurationContext context) throws InitializationE
try {
setupClient(context);
responseCharset = Charset.forName(context.getProperty(CHARSET).getValue());
+
+ // re-create the ObjectMapper in case the SUPPRESS_NULLS property has changed - the JsonInclude settings aren't dynamic
+ mapper = new ObjectMapper();
+ if (ALWAYS_SUPPRESS.getValue().equals(context.getProperty(SUPPRESS_NULLS).getValue())) {
+ mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
Review comment:
For the behavior I saw above, I compiled and built the ES NARs, dropped them into a NiFi based off `main`, and ran against a Docker container running ES 5.6.16. Using PutElasticsearchRecord I wasn't seeing the `"y": null` when set to Never Suppress. I forgot to look at the current (main not this PR) behavior to see if the "y" was in the document returned by the query, but it wasn't with this PR and the aforementioned setup.
----------------------------------------------------------------
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
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #4755: NIFI-8133 Add ability to supress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560396671
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/pom.xml
##########
@@ -196,7 +196,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-failsafe-plugin</artifactId>
- <version>3.0.0-M5</version>
+ <version>3.0.0-M3</version>
Review comment:
Turns out there's a bug in M4/5 that prevents the classpath being populated correctly for the integration tests - i.e. they don't run with -M5, but this works with M3
Apparently it's been fixed in the latest snapshot versions of the plugin, but not yet available
----------------------------------------------------------------
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
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #4755: NIFI-8133 Add ability to supress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560466124
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/test/groovy/org/apache/nifi/elasticsearch/integration/ElasticSearch5ClientService_IT.groovy
##########
@@ -175,6 +177,41 @@ class ElasticSearch5ClientService_IT {
runner.assertValid()
}
+ @Test
+ void testNullSuppression() {
Review comment:
The integration tests run as part of the [Maven build](https://github.com/apache/nifi/blob/main/nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/pom.xml#L193) in the `nifi-elasticsearch-client-service` bundle, but need the correct profiles actiating, for example:
* ES 7: `mvn -P integration-7,integration-tests clean install`
* ES 6: `mvn -P integration-6,integration-tests clean install`
* ES 5: `mvn -P integration-tests clean install`
These include the setup of indices and expected documents for the tests to run (see the [test/resources](https://github.com/apache/nifi/tree/main/nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/test/resources) script files).
Running these locally with the code from this branch using JDK 11, I get successful builds with the above commands.
I'm asuming the `_type` error you're getting above is because you're not setting the `type_name` System property if you've trying to run these tests without using the Maven build (you'd also need to setup the indices/documents in the same way as the test scripts).
----------------------------------------------------------------
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
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #4755: NIFI-8133 Add ability to supress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560499832
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java
##########
@@ -90,6 +93,15 @@ public void onEnabled(final ConfigurationContext context) throws InitializationE
try {
setupClient(context);
responseCharset = Charset.forName(context.getProperty(CHARSET).getValue());
+
+ // re-create the ObjectMapper in case the SUPPRESS_NULLS property has changed - the JsonInclude settings aren't dynamic
+ mapper = new ObjectMapper();
+ if (ALWAYS_SUPPRESS.getValue().equals(context.getProperty(SUPPRESS_NULLS).getValue())) {
+ mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
Review comment:
I also temporarily logged the body being sent to Elasticsearch within `bulk` and the response body from Elasticsearch in `get` prior to the ObjectMapper being used... both showed the expected lack of `null` value fields with `ALWAYS` but inclusion of the same with `NEVER`.
----------------------------------------------------------------
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
[GitHub] [nifi] mattyb149 commented on a change in pull request #4755: NIFI-8133 Add ability to supress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560426686
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/test/groovy/org/apache/nifi/elasticsearch/integration/ElasticSearch5ClientService_IT.groovy
##########
@@ -175,6 +177,41 @@ class ElasticSearch5ClientService_IT {
runner.assertValid()
}
+ @Test
+ void testNullSuppression() {
Review comment:
I couldn't get any of these tests to pass, looks like it expects the `messages` index to already exist, and for things like testBasicSearch it appears that it was supposed to be primed with 15 messages. I created an empty `messages` index and ran just this testNullSuppression, and got the following error (using ES 5.6.16):
```
org.apache.nifi.elasticsearch.ElasticsearchError: org.elasticsearch.client.ResponseException: POST http://localhost:9200/_bulk: HTTP/1.1 400 Bad Request
{"error":{"root_cause":[{"type":"action_request_validation_exception","reason":"Validation Failed: 1: type is missing;"}],"type":"action_request_validation_exception","reason":"Validation Failed: 1: type is missing;"},"status":400}
```
Am I doing something wrong? Couldn't find anything in the ES docs about ignoring the document type...
----------------------------------------------------------------
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
[GitHub] [nifi] mattyb149 commented on a change in pull request #4755: NIFI-8133 Add ability to supress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560508878
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/test/groovy/org/apache/nifi/elasticsearch/integration/ElasticSearch5ClientService_IT.groovy
##########
@@ -175,6 +177,41 @@ class ElasticSearch5ClientService_IT {
runner.assertValid()
}
+ @Test
+ void testNullSuppression() {
Review comment:
Ah yeah I was running the test from my IDE. Will try with Maven instead.
----------------------------------------------------------------
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
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #4755: NIFI-8133 Add ability to suppress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r641355712
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java
##########
@@ -90,6 +93,15 @@ public void onEnabled(final ConfigurationContext context) throws InitializationE
try {
setupClient(context);
responseCharset = Charset.forName(context.getProperty(CHARSET).getValue());
+
+ // re-create the ObjectMapper in case the SUPPRESS_NULLS property has changed - the JsonInclude settings aren't dynamic
+ mapper = new ObjectMapper();
+ if (ALWAYS_SUPPRESS.getValue().equals(context.getProperty(SUPPRESS_NULLS).getValue())) {
+ mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
Review comment:
@mattyb149 realise it's been a while, but I still think this is a worthwhile change to make - are you able to take another look at the latest version (rebased against `main`)?
--
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
[GitHub] [nifi] ChrisSamo632 commented on a change in pull request #4755: NIFI-8133 Add ability to suppress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560939470
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/test/groovy/org/apache/nifi/elasticsearch/integration/ElasticSearch5ClientService_IT.groovy
##########
@@ -175,6 +177,41 @@ class ElasticSearch5ClientService_IT {
runner.assertValid()
}
+ @Test
+ void testNullSuppression() {
Review comment:
Happy for this thread to be resolved given you mention having run the integration tests below?
----------------------------------------------------------------
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
[GitHub] [nifi] mattyb149 commented on a change in pull request #4755: NIFI-8133 Add ability to supress null/empty values in ElasticSearchCl…
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #4755:
URL: https://github.com/apache/nifi/pull/4755#discussion_r560432077
##########
File path: nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java
##########
@@ -90,6 +93,15 @@ public void onEnabled(final ConfigurationContext context) throws InitializationE
try {
setupClient(context);
responseCharset = Charset.forName(context.getProperty(CHARSET).getValue());
+
+ // re-create the ObjectMapper in case the SUPPRESS_NULLS property has changed - the JsonInclude settings aren't dynamic
+ mapper = new ObjectMapper();
+ if (ALWAYS_SUPPRESS.getValue().equals(context.getProperty(SUPPRESS_NULLS).getValue())) {
+ mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
Review comment:
I ran this with Never once and Always once, and both times with a field `y` set to `null` it doesn't appear to get into the document. I expect that for Always but if I query the document that had Never Suppress set and one field (that is non-null in other docs in the index) that is null, shouldn't I expect `"y": null` in the document returned by the 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