GH-4422: Add test coverage for null/empty override properties in DefaultKafkaConsumerFactory

[Flicko75]

Fixes GH-4422 (#4422)

• add test verifying null value in Properties overrides throws NullPointerException
• add test verifying empty string value in Properties overrides is applied to consumer config

[Gautam-aman]

Thanks for adding the tests!

I noticed a couple of things:

1. The null-value test is currently validating Properties.put(key, null) behavior, which throws a NullPointerException at the JDK level itself. So this doesn't actually test the behavior of DefaultKafkaConsumerFactory.

2. In the empty string test, createRawConsumer returns null — depending on the internal flow, this might be causing the build failure or unintended side effects.

Maybe we should:

• Validate null handling at the factory level (if applicable), instead of relying on Properties.put
• Return a mock/stub Consumer instead of null

Happy to help refine or extend the tests if needed.

[Flicko75]

Thanks for the feedback!
The build failure was checkstyle violations; Test methods were placed after an inner class and missing spaces before { , both fixed now.

Regarding the null test, I initially tried testing that null values are ignored by the factory, but Properties.put(key, null) throws a NullPointerException at the JDK level before the value even reaches the factory code. So the current test documents that behavior instead.
I considered using Properties defaults to bypass this, but Properties.put() throws for null even in that layer in newer Java versions.

Could you clarify what specific scenario you had in mind for null values reaching the factory?

[Gautam-aman]

Good point you're right that Properties.put(key, null) fails at the JDK level, so null values won't normally reach the factory through the overrides Properties.

What I had in mind was more around defining expected behavior at the factory boundary, in case null values are introduced indirectly (e.g., via custom map manipulation or future changes in how overrides are handled internally).

That said, given the current API constraints, it probably makes more sense to focus on:

• empty string values
• missing keys
• possibly invalid/non-string values in overrides

So I think the empty string test is definitely valid and useful, while the null case may not be applicable in practice unless there's a different entry point I'm missing.

Let me know your thoughts.

[Gautam-aman]

This looks much cleaner now, thanks for the updates!

One thing I’m unsure about is the expected behavior for empty string overrides:

• In testEmptyStringValueInOverridePropertiesIsApplied, an empty string replaces the original value
• But in testNewKeyInOverridePropertiesIsAddedToConfig, max.poll.records is set to "" in overrides, yet the assertion expects the original value ("10")

So it seems like we’re treating empty string both as:

1. a valid override
2. and something to ignore

Could you clarify which behavior is intended?

If empty string is considered a valid override, then the second test might need to assert "" instead of "10".

[Gautam-aman]

Maybe we should explicitly document the behavior:

• empty string → applied as override
  OR
• empty string → ignored

and align all tests accordingly

[Flicko75]

@Gautam-aman So I removed the null test entirely and added another test that checks if new keys that are not already in the config are added or not; addressing the missing keys point you mentioned.
And for other changes I just returned a mocked Consumer.class as you requested.

Please check if anything else is needed or feel free to improve the tests.

[Flicko75]

Thanks for pointing this out. I see where the confusion is coming from.

In the second test testNewKeyInOverridePropertiesIsAddedToConfig , we’re not actually overriding max.poll.records with an empty string. The override only introduces a new key fetch.min.bytes , so the original value "10" is expected to remain unchanged.

So the current behavior is consistent with:

• empty string → applied as a valid override
• missing key → original value is retained

If you think we should explicitly document or test the behavior for empty string overrides on existing keys more clearly, I can add an additional test to make that intent explicit.

[Gautam-aman]

Got it , thanks for clarifying, that makes sense.

I had assumed max.poll.records was being overridden with an empty string in the second test, but since it's not part of the overrides there, the behavior is consistent.

The current tests look good to me 👍

[Gautam-aman]

@sobychacko

[sobychacko]

I don't think this test is related to the issue in #4422 - the scenarios you're testing here are already covered by other tests in this class. I suggest dropping it.

Also, the PR description in one of the commit messages says you've added a test for the null-value case, but I don't see one in the diff. There isn't really a meaningful "null value" path through DefaultKafkaConsumerFactory to test since Properties.put(key, null) throws NPE. Please fix that message.

[Flicko75]

Will drop testNewKeyInOverridePropertiesIsAddedToConfig as suggested. I'll also fix the commit message to remove the reference to the null value test.

[sobychacko]

Any reason why you didn't follow the same strategies used in testMixedTypeOverridesApplied for example where it uses and AtomicReference to capture the consumer config and then set it using the captured config? That way, we assert against the real Kafka ConsumerConfig.

[Flicko75]

I tried updating the test to use AtomicReference<ConsumerConfig> pattern, but ran into an issue : ConsumerConfig validates field values strictly, so empty string fails validation for standard Kafka config fields like max.poll.records and auto.offset.reset. Could you suggest a specific config key that would work for testing empty string override behavior, or should we keep the raw map assertion for this case?

[sobychacko]

@Flicko75 Any chance to address my concerns above? Our release is almost around the corner and we only have a few days to address this.