KAFKA-13306: Null connector config value passes validation, but fails creation#11333
KAFKA-13306: Null connector config value passes validation, but fails creation#11333mimaison merged 2 commits intoapache:trunkfrom
Conversation
| .collect(Collectors.joining(", ")); | ||
|
|
||
| if (!keysWithNullValue.isEmpty()) { | ||
| throw new ConnectException(String.format("Null value found in config for key(s) %s", keysWithNullValue)); |
There was a problem hiding this comment.
Should this just be IllegalArgumentException?
There was a problem hiding this comment.
Thank you for the comment, updated it in the latest commit.
| } | ||
|
|
||
| @Test | ||
| public void testConfigValidationNullConfig() { |
There was a problem hiding this comment.
Could you also add a test case with multiple null values as well?
There was a problem hiding this comment.
Thank you for the comment, updated it in the latest commit.
lhunyady
force-pushed
the
kafka-13306
branch
2 times, most recently
from
ebef6c0 to
b6013f7
Compare
|
@C0urante @kkonstantine Can you please review this change for me? |
|
Thanks @lhunyady. I wonder if instead of throwing an exception we can add an error message to the offending config properties? This would allow other configuration errors to be surfaced immediately in calls to If this approach is desirable, we could implement it with an additional step in Thoughts? |
|
Thanks @C0urante for the feedback, I agree with you and updated the PR accordingly. |
There was a problem hiding this comment.
Thanks @lhunyady. I left a few comments but I was also wondering about an alternative approach that seems a little less verbose; I've drafted it on a personal branch (including the tests you've added in this PR, which both pass); what do you think?
Also, higher-level, I'm wondering if it might be easiest to just filter out null values from connectorProps instead of failing the connector? I'm a little hesitant to do that just because people might become reliant on this behavior but it's probably worth considering before we merge this PR.
There was a problem hiding this comment.
Since this is no longer used in the main code base, could we remove it from this class and, where necessary, replace it either with a call to the new variant created below, or a testing-only wrapper function that has the same body as this one?
There was a problem hiding this comment.
I believe that including the name of the property in the error message is redundant as that information will be available already in the REST response.
I also think we may want to be clearer about the error message here. Users can't supply null values, but developers (by specifying null as the default value for a property in a ConfigDef, for example) definitely can, and we may want to make it clear which variety we're prohibiting.
What do you think about this?
| .map(configEntry -> new ConfigValueInfo(configEntry.getKey(), String.format("Null Value Not Allowed for key %s.", configEntry.getKey()))) | |
| .map(configEntry -> new ConfigValueInfo(configEntry.getKey(), "The JSON literal `null` may not be used in connector configurations")) |
… creation This patch adds null value check to the connector config validation, and extends unit tests to cover this functionality. Co-authored-by: Chris Egerton <chrise@confluent.io>
… creation This patch adds null value check to the connector config validation, and extends unit tests to cover this functionality. Co-authored-by: Chris Egerton <chrise@confluent.io>
|
Thank you @C0urante, I have updated the pull request with your valuable suggestions. |
|
@C0urante what are the next steps to get this pull request merged? |
|
@tombentley @kkonstantine would either of you have time for this small but valuable fix? |
6 participants
This patch adds null value check to the connector config validation, and extends unit tests to cover this functionality.