Ensure custom KProperty include the name in the hashcode #1710
Conversation
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
The `pathCache` utilised the string representation of the KProperty instance. The custom implementations didn't include the calculated path value, this can lead to naming collisions in the pathCache key. This commit adds `hashCode` and `equals` methods to ensure the `name` value is included in our custom implementations of `KProperty1`. Finally, the cache key is explicitly created using the same default `Any.toString()` implementation to ensure that all implementations of `KProperty1` use the same logic to create the cacheKey. JAVA-5868
nhachicha
reviewed
May 8, 2025
| @@ -71,8 +71,7 @@ public fun <T> KProperty<T>.path(): String { | |||
| return if (this is KPropertyPath<*, T>) { | |||
| this.name | |||
| } else { | |||
| pathCache.computeIfAbsent(this.toString()) { | |||
|
|
|||
| pathCache.computeIfAbsent("${this.javaClass.getSimpleName()}@${Integer.toHexString(hashCode())}") { | |||
There was a problem hiding this comment.
Good catch!
Can't we just refactor the map to
private val pathCache: MutableMap<Int, String> by lazySoft { ConcurrentHashMap<Int, String>() }then use the hashCode
pathCache.computeIfAbsent(this.hashCode())not sure why you concatenate the class name with the hash?
| @@ -84,6 +85,15 @@ public open class KPropertyPath<T, R>( | |||
| override fun callBy(args: Map<KParameter, Any?>): R = unSupportedOperation() | |||
| override fun get(receiver: T): R = unSupportedOperation() | |||
| override fun getDelegate(receiver: T): Any? = unSupportedOperation() | |||
| override fun hashCode(): Int = Objects.hash(previous, property, name) | |||
There was a problem hiding this comment.
It's a good idea to provide the hashCode and equals but these weren't involved in the code path since we return the name directly if the instance is KPropertyPath in
return if (this is KPropertyPath<*, T>) {
this.name
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.
The
pathCacheutilized the string representation of theKPropertyinstance. The custom implementations didn't include the name (the calculated path value), this can lead to naming collisions in thepathCachekey.This commit adds
hashCodeandequalsmethods to ensure thenamevalue is included in our custom implementations ofKProperty. Finally, the cache key is now explicitly created using the same defaultAny.toString()approach to ensure that all implementations ofKPropertyuse the same logic to create the cache key.JAVA-5868