Fix: IllegalArgument Exception in String converter #3172
Conversation
Signed-off-by: Asif Sohail Mohammed <[email protected]>
asifsmohammed
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
kkondaka,
cmanning09,
dlvenable and
oeyh
as code owners
| if (Objects.nonNull(source)) { | ||
| return source.toString(); | ||
| } else { | ||
| return ""; |
There was a problem hiding this comment.
In the case of null, we can probably handle this in the ConvertEntryTypeProcessor class. Any null value can be converted to null for the target.
Also, it should not be an empty string. It should be null.
There was a problem hiding this comment.
I can return null string in case of null value instead of empty string. convert method is also used in translate processor.
| throw new IllegalArgumentException("Unsupported type conversion"); | ||
| LOG.error(EVENT, "Unable to convert {} to String", source); | ||
| if (Objects.nonNull(source)) { | ||
| return source.toString(); |
There was a problem hiding this comment.
I'm not sure this is really a good behavior. It could result in strange output. Say a list is provided. The user will get a string that looks like ListString@89879 or something like that.
I think this should continue to throw the exception.
We can handle the exception in the ConvertEntryTypeProcessor class to make it consistent across all types.
Perhaps by dropping the data?
There was a problem hiding this comment.
I think the solution to this issue should be done entirely within ConvertEntryTypeProcessor. Let's continue to have the TypeConverter class throw and expect non-null.
So change ConvertEntryTypeProcessor to:
- Handle
nullconversions since the result is alwaysnull. - Catch exceptions and handle these.
One item 2, the solution isn't completely clear. It should definitely log the error. But, what about the data? Does it drop the whole event or just the field? Or leave unconverted?
I tend to think we should add a user configuration to let the user decide how to handle. The most appropriate default to me seems to be to drop the data. Otherwise, we are sending garbage downstream.
Signed-off-by: Asif Sohail Mohammed <[email protected]>
asifsmohammed
force-pushed
the
string-converter-fix
branch
from
2d4c02e
to
460251e
Compare
|
I added |
| @@ -25,6 +27,9 @@ public String convert(Object source) throws IllegalArgumentException { | |||
| if (source instanceof String) { | |||
| return (String)source; | |||
| } | |||
| if (Objects.isNull(source)) { | |||
| return "null"; | |||
There was a problem hiding this comment.
I think we can return null, not a string named "null".
asifsmohammed
force-pushed
the
string-converter-fix
branch
from
3082521
to
b26af29
Compare
Signed-off-by: Asif Sohail Mohammed <[email protected]>
asifsmohammed
force-pushed
the
string-converter-fix
branch
from
b26af29
to
72d4584
Compare
Description
Fix IllegalArgumentException and return object.toString
Issues Resolved
Resolves #3135
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.