kutsibalci opened a new pull request, #23098:
URL: https://github.com/apache/kafka/pull/23098
`TokenInformation.equals()` compares six fields and leaves `expiryTimestamp`
out. `hashCode()` hashes seven — the same six plus `expiryTimestamp`.
```java
// equals()
return issueTimestamp == that.issueTimestamp &&
maxTimestamp == that.maxTimestamp &&
Objects.equals(owner, that.owner) &&
Objects.equals(tokenRequester, that.tokenRequester) &&
Objects.equals(renewers, that.renewers) &&
Objects.equals(tokenId, that.tokenId);
// hashCode()
return Objects.hash(owner, tokenRequester, renewers, issueTimestamp,
maxTimestamp, expiryTimestamp, tokenId);
```
So two instances can be `equal` and still hash differently, which is the one
thing `Object.hashCode` requires never happens:
> If two objects are equal according to the `equals(Object)` method, then
calling the `hashCode` method on each of the two objects must produce the same
integer result.
There is a second edge to it. `expiryTimestamp` is the only non-`final`
field on the class, and `setExpiryTimestamp` is public and is what renewal
calls. Hashing it means an instance's hash code can change *after* it has been
put in a `HashSet` or used as a `HashMap` key, so the collection can no longer
find an object it still holds.
## Which side to change
The omission in `equals()` looks deliberate rather than accidental: renewing
a token does not make it a different token, and `tokenId` already identifies
it. Adding `expiryTimestamp` to `equals()` would change what token identity
means and would leave a mutable field driving the hash. Dropping it from
`hashCode()` keeps the existing notion of equality and makes the hash stable,
so that is the direction taken here.
## Observed behaviour
Built from the current `trunk` sources, with two instances of the same token
that differ only in `expiryTimestamp`:
| | before | after |
|---|---|---|
| `a.equals(b)` | `true` | `true` |
| `a.hashCode() == b.hashCode()` | **`false`** | `true` |
| `set.contains(b)` after `set.add(a)` | **`false`** | `true` |
| `set.size()` after adding both | **`2`** | `1` |
| `map.get(b)` after `map.put(a, v)` | **`null`** | `v` |
| `set.contains(c)` after `c.setExpiryTimestamp(...)` | **`false`** | `true`
|
The last row uses the same instance that was added to the set.
## Scope
`TokenInformation` is public API —
`org.apache.kafka.common.security.token.delegation`, not an `internals` package
— and instances reach callers through `Admin.describeDelegationToken()` and
`DelegationTokenCache.tokens()`, so user code can put them in hash-based
collections.
I want to be accurate about the blast radius inside Kafka itself:
`DelegationTokenCache` keys its maps by the token id `String`, not by
`TokenInformation`, and I did not find a broker or client path that puts these
objects in a `HashSet` or uses them as a map key today. So this is a defect in
the published contract of a public class rather than a bug with a current
reproducer in the broker. I would rather say that plainly than overstate it.
## Tests
Adds `TokenInformationTest`, which did not exist. Against unmodified
`trunk`, three of its four cases fail:
```
+-- TokenInformationTest
| +-- testHashCodeIsStableAcrossRenewal() [X] expected:
<1465872210> but was: <1465934210>
| +-- testEqualsAndHashCode() [OK]
| +-- testEqualInstancesCollapseInHashSet() [X] expected:
<true> but was: <false>
| '-- testHashCodeIgnoresExpiryTimestampLikeEquals() [X] expected:
<1465872210> but was: <1465903210>
[ 1 tests successful ]
[ 3 tests failed ]
```
With this change, all four pass. `testEqualsAndHashCode` passes in both runs
by design — it is the baseline case where the two instances carry the same
expiry, and it would catch a fix that broke ordinary equality.
## How this was found, and what I left alone
I walked every class body under `src/main/java` that declares both
`equals()` and `hashCode()` and compared the set of instance fields each one
reads. 500 classes declare both. `TokenInformation` is the only one where
`hashCode()` reads a field that `equals()` ignores.
Seven classes have the mismatch the other way round — `equals()` compares a
field that `hashCode()` skips:
`StreamsMetadataImpl` (`topologyName`), `FetchParams` (`maxWaitMs`),
`UnionSet` (`size`), `MemoryRecords` (`batches`), `KafkaChannel`
(`remoteAddress`, `state`), `MetadataResponse.TopicMetadata` (`topicId`).
I have not touched any of those. That direction does not violate the
contract — equal objects still hash the same; unequal ones just collide more
often than they need to. Whether any of them is worth tightening is a separate
judgement about each class, not a correctness fix.
I also looked at, and deliberately left alone, cases the scan raised that
turned out to be fine on reading: classes that cache the hash in a field
(`ConnectMetrics.MetricGroupId`), classes that delegate to a helper
(`TestRecord.equalsFields`), fields that are derived from others already
compared (`LagInfo.offsetLag`), and `ProcessorMetadata`, where a comment states
that `needsCommit` is excluded from both on purpose.
## Checks
- The scan re-run after this change reports no class where `hashCode()`
reads a field `equals()` ignores.
- `equals()` is untouched, so what counts as an equal token is unchanged.
- One line of production code; the rest is the new test.
## AI disclosure
AI-assisted (Claude Code), per the AI-Generated Contributions section of
`CONTRIBUTING.md`; the commit carries a `Generated-by` trailer. The scan, the
test and this description were produced with the tool, and I checked the result
myself before opening: I read the `equals()` and `hashCode()` bodies, confirmed
`expiryTimestamp` is the only non-final field and that `setExpiryTimestamp` is
public, built the two variants from the real sources and ran the test against
both, searched for the places `TokenInformation` is actually stored to work out
how far this reaches, and went through the seven opposite-direction classes and
the false positives above one at a time rather than reporting the scan output
as-is. I understand the change and take responsibility for it.
--
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]
