MINOR: Exclude expiryTimestamp from TokenInformation.hashCode - #23098
Open
kutsibalci wants to merge 1 commit into
Open
MINOR: Exclude expiryTimestamp from TokenInformation.hashCode#23098kutsibalci wants to merge 1 commit into
kutsibalci wants to merge 1 commit into
Conversation
TokenInformation.equals() compares six fields and deliberately leaves expiryTimestamp out, since renewing a token does not make it a different token. hashCode() hashes that field as well, so two instances that compare equal can produce different hash codes. That breaks the Object.hashCode contract and makes lookups in hash-based collections miss. expiryTimestamp is also the only non-final field on the class, and it has a public setter that renewal uses, so an instance's hash code can change while it is already an element of a HashSet or a key in a HashMap. Align hashCode() with equals() and add TokenInformationTest, which fails on trunk for three of its four cases and passes with this change. Generated-by: Claude Opus 5 (Claude Code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TokenInformation.equals()compares six fields and leavesexpiryTimestampout.hashCode()hashes seven — the same six plusexpiryTimestamp.So two instances can be
equaland still hash differently, which is the one thingObject.hashCoderequires never happens:There is a second edge to it.
expiryTimestampis the only non-finalfield on the class, andsetExpiryTimestampis public and is what renewal calls. Hashing it means an instance's hash code can change after it has been put in aHashSetor used as aHashMapkey, 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, andtokenIdalready identifies it. AddingexpiryTimestamptoequals()would change what token identity means and would leave a mutable field driving the hash. Dropping it fromhashCode()keeps the existing notion of equality and makes the hash stable, so that is the direction taken here.Observed behaviour
Built from the current
trunksources, with two instances of the same token that differ only inexpiryTimestamp:a.equals(b)truetruea.hashCode() == b.hashCode()falsetrueset.contains(b)afterset.add(a)falsetrueset.size()after adding both21map.get(b)aftermap.put(a, v)nullvset.contains(c)afterc.setExpiryTimestamp(...)falsetrueThe last row uses the same instance that was added to the set.
Scope
TokenInformationis public API —org.apache.kafka.common.security.token.delegation, not aninternalspackage — and instances reach callers throughAdmin.describeDelegationToken()andDelegationTokenCache.tokens(), so user code can put them in hash-based collections.I want to be accurate about the blast radius inside Kafka itself:
DelegationTokenCachekeys its maps by the token idString, not byTokenInformation, and I did not find a broker or client path that puts these objects in aHashSetor 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 unmodifiedtrunk, three of its four cases fail:With this change, all four pass.
testEqualsAndHashCodepasses 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/javathat declares bothequals()andhashCode()and compared the set of instance fields each one reads. 500 classes declare both.TokenInformationis the only one wherehashCode()reads a field thatequals()ignores.Seven classes have the mismatch the other way round —
equals()compares a field thathashCode()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), andProcessorMetadata, where a comment states thatneedsCommitis excluded from both on purpose.Checks
hashCode()reads a fieldequals()ignores.equals()is untouched, so what counts as an equal token is unchanged.AI disclosure
AI-assisted (Claude Code), per the AI-Generated Contributions section of
CONTRIBUTING.md; the commit carries aGenerated-bytrailer. The scan, the test and this description were produced with the tool, and I checked the result myself before opening: I read theequals()andhashCode()bodies, confirmedexpiryTimestampis the only non-final field and thatsetExpiryTimestampis public, built the two variants from the real sources and ran the test against both, searched for the placesTokenInformationis 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.