Ensure custom KProperty include the name in the hashcode #1710
Conversation
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
| @@ -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?
There was a problem hiding this comment.
I suppose I followed the previous logic for the key.
Hashcodes alone don't provide enough protection from collisions:
data class Name(val name: String)
data class School(val house: String)
@Test
fun testToString() {
assertEquals(Name("Merlin").hashCode(), School("Merlin").hashCode())
}
In this case they appear to - I tested up to 2 million for naming collisions but I still have some concern that it could also cause an issue. What do you think?
There was a problem hiding this comment.
The hashcode is calculated for the KProperty1 so it will only match if you're using the same KProperty1
data class Name(val name: String)
data class School(val name: String)
val p1: KProperty1<Name, String> = Name::name
val p2: KProperty1<School, String> = School::name
println(p1.hashCode()) // 2000641095
println(Name::name.hashCode()) // 2000641095
println(School::name.hashCode()) // -1039559637| @@ -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.nameThere was a problem hiding this comment.
Not used for the direct path, but the hashcode for CustomProperty relies on the hashcode.
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