NIFI-4857: Support String<->byte[] conversion #2570

mattyb149 · 2018-03-21T01:51:31Z

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?
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.

markap14

@mattyb149 this looks good overall. I think we can simply things a bit, though, by avoiding support for byte[] and ByteBuffer in the Record and instead just support use of byte[]. I also think we need to reconsider how we handle some of the RuntimeExceptions and conversion failures. I commented inline on specific areas that I noticed.

markap14 · 2018-03-22T14:21:11Z

...mmons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java

+                // Either an object array or a String to be converted to byte[] or a ByteBuffer (from Avro, e.g.)
+                && (value instanceof Object[]
+                || (value instanceof String && RecordFieldType.BYTE.getDataType().equals(elementDataType))
+                || value instanceof ByteBuffer);


I don't think we should be supporting ByteBuffer here, just byte[]. The more we allow for, the more complex this gets and the more error-prone and less consistent it will become. While Avro may use ByteBuffers, when we use an Avro Reader to create a Record, we should be doing the conversion there from ByteBuffer to byte[].

markap14 · 2018-03-22T14:21:34Z

...mmons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java

+            return new String((byte[])value, charset);
+        }
+
+        if (value instanceof ByteBuffer) {


Same as above, I think we should avoid the use of ByteBuffer here

markap14 · 2018-03-22T14:22:09Z

...mmons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java

+        if (value instanceof byte[]) {
+            return new String((byte[]) value, charset);
+        }
+        if (value instanceof ByteBuffer) {


Same as above.

markap14 · 2018-03-22T14:23:04Z

...mmons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java

+        } else {
+            try {
+                return Charset.forName(charsetName);
+            } catch(Exception e) {


If given an invalid character set, I think I would prefer to just throw the Exception. If there is a typo somewhere, this can lead to some very unexpected results that are difficult to track down.

markap14 · 2018-03-22T14:24:08Z

nifi-commons/nifi-record-path/src/main/java/org/apache/nifi/record/path/functions/ToString.java

+                            stringValue = ""; // Empty array = empty string
+                        }
+                    } else if (!(fv.getValue() instanceof byte[])) {
+                        return fv;


This probably warrants throwing an Exception. It seems wrong to me to have the user explicitly indicating that they want a conversion to a String and then return something different, like an Integer.

markap14 · 2018-03-22T14:25:39Z

nifi-commons/nifi-record-path/src/main/java/org/apache/nifi/record/path/functions/ToString.java

+                        try {
+                            stringValue = DataTypeUtils.toString(fv.getValue(), (String) null, charset);
+                        } catch (final Exception e) {
+                            return fv;


If any RuntimeException is thrown here, I don't think we want to silently ignore it. Should probably let it fly.

markap14 · 2018-03-22T14:26:45Z

nifi-commons/nifi-record-path/src/main/java/org/apache/nifi/record/path/functions/ToBytes.java

+                .map(fv -> {
+
+                    if (!(fv.getValue() instanceof String)) {
+                        return fv;


We should probably be throwing an Exception in this case? The user in this case is attempting to coerce a type that is not valid to coerce. Or otherwise filter it out from the results. It seems wrong to me to just ignore the conversion.

Makes sense to me, but I copied that from ToDate, seemed like since it's a Stream, the top-level caller is expecting a new object back rather than catching an exception?

Agreed, the top-level caller is expecting that... but in this case, we cannot give the caller the correct type of data. So I think it's best to just throw instead of giving the caller the wrong data... if that's happening in ToDate then it's either a bug there as well, or perhaps there's some undocumented assumption being made about what else the type could be??

markap14 · 2018-03-22T14:27:24Z

nifi-commons/nifi-record-path/src/main/java/org/apache/nifi/record/path/functions/ToBytes.java

+                            bytesValue[i] = src[i];
+                        }
+                    } catch (final Exception e) {
+                        return fv;


Should probably let the Exception fly - as-is, it is just silently swallowing the Exception and then returning an object of the wrong type.

markap14 · 2018-03-22T14:30:22Z

...ifi-record-utils/nifi-avro-record-utils/src/main/java/org/apache/nifi/avro/AvroTypeUtil.java

@@ -609,6 +623,9 @@ private static Object convertToAvroObject(final Object rawValue, final Schema fi
                if (rawValue instanceof byte[]) {
                    return ByteBuffer.wrap((byte[]) rawValue);
                }
+                if (rawValue instanceof String) {
+                    return ByteBuffer.wrap(((String) rawValue).getBytes(charset));


I would prefer to avoid ByteBuffer here and instead use just byte[]

In the clause above, a byte[] is wrapped in a ByteBuffer as well (that's where I got the code from), won't we be returning two different objects in that case?

Whoops - my bad on this one. This is #convertToAvroObject, and I was thinking of #normalizeValue. In this case, we are converting into the object that Avro wants, so a ByteBuffer is the correct thing to do.

… tests

markap14 · 2018-04-09T18:54:31Z

@mattyb149 thanks for the update! Sorry about the delay in getting back to this. All looks good now from my POV. There was a checkstyle violation (unused import) but I addressed that and all else looks good so merged to master.

mattyb149 force-pushed the NIFI-4857_2 branch 2 times, most recently from a1f9d46 to 206f069 Compare March 21, 2018 02:03

markap14 requested changes Mar 22, 2018

View reviewed changes

mattyb149 added 2 commits March 23, 2018 16:01

NIFI-4857: Support String<->byte[] conversion for records

23213bd

NIFI-4857: Incorporated review comments, updated doc, added more unit…

26083e5

… tests

mattyb149 force-pushed the NIFI-4857_2 branch from 206f069 to 26083e5 Compare March 23, 2018 20:01

asfgit closed this in b29304d Apr 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-4857: Support String<->byte[] conversion #2570

NIFI-4857: Support String<->byte[] conversion #2570

mattyb149 commented Mar 21, 2018

markap14 left a comment

markap14 Mar 22, 2018

markap14 Mar 22, 2018

markap14 Mar 22, 2018

markap14 Mar 22, 2018

markap14 Mar 22, 2018

markap14 Mar 22, 2018

markap14 Mar 22, 2018

mattyb149 Mar 22, 2018

markap14 Mar 22, 2018

markap14 Mar 22, 2018

markap14 Mar 22, 2018

mattyb149 Mar 22, 2018

markap14 Mar 22, 2018

markap14 commented Apr 9, 2018

NIFI-4857: Support String<->byte[] conversion #2570

NIFI-4857: Support String<->byte[] conversion #2570

Conversation

mattyb149 commented Mar 21, 2018

For all changes:

For code changes:

For documentation related changes:

Note:

markap14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markap14 commented Apr 9, 2018