regarmukesh3g commented on PR #23324:
URL: https://github.com/apache/kafka/pull/23324#issuecomment-5595555063
Thanks @AndrewJSchofield — agreed, and done in 8ed68878d4.
I checked the three things your reasoning rests on and they all hold:
- `ProtocolType` is declared `{ "name": "ProtocolType", "type": "string",
"versions": "0+" }` in `ListGroupsResponse.json`, with no `nullableVersions`,
so it is non-nullable on the wire.
- Both production call sites in `KafkaAdminClient` pass
`group.protocolType()` straight through from the response.
- `isSimpleConsumerGroup()` calls `protocol.isEmpty()` directly, so a null
protocol throws an NPE away from the constructor that accepted it — the same
failure mode as `groupState`, which `toString()` dereferences.
Added `testNullProtocolConstructor` alongside the existing two. It fails
without the change:
```
testNullProtocolConstructor() :: org.opentest4j.AssertionFailedError:
Expected java.lang.NullPointerException to be thrown, but nothing was thrown.
```
and passes with it. The full `clients` admin suite (555 tests) and
`GroupsCommandTest` are green, so no existing caller was relying on passing
null.
One open question, entirely your call: `groupId` is now the only unchecked
parameter. It is also non-nullable in `ListGroupsResponse`, but unlike
`protocol` it is never dereferenced unsafely — `toString()` concatenates it and
`equals`/`hashCode` go through `Objects`, so a null would propagate silently
rather than throwing. Happy to add `requireNonNull` there too for consistency,
or leave it out to keep this PR narrow. Let me know which you prefer.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
