-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix error in RealmAny.equals when comparing similarly typed object #1523
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups 🙈 ...but this PR doesn't 100% fix the issue ;)
@@ -127,27 +127,27 @@ internal class RealmAnyImpl<T : Any> constructor( | |||
if (other.type != this.type) return false | |||
if (clazz == ByteArray::class) { | |||
if (other.internalValue !is ByteArray) return false | |||
if (!other.internalValue.contentEquals(this.internalValue as ByteArray)) return false | |||
return other.internalValue.contentEquals(this.internalValue as ByteArray) | |||
} else if (internalValue is BsonObjectId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only support the KBSON variant for RealmAny, right? And not also our deprecated variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. There is only an entry point for org.mongodb.kbson.BsonObjectId
. I have removed any logic guarding this and relied on type checking and internal value's equals
method for this.
is Number -> if (other.internalValue.toLong() != this.internalValue.toLong()) return false | ||
return when (other.internalValue) { | ||
is Char -> other.internalValue.code.toLong() == internalValue.toLong() | ||
is Number -> other.internalValue.toLong() == this.internalValue.toLong() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right for Float and Double?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, indeed not. Good catch. I have removed the specializations around Char
and Number
and are now just relying on type checks and inner value's equals
.
assertEquals(RealmAny.create(now), RealmAny.create(now)) | ||
assertNotEquals(RealmAny.create(RealmInstant.from(1, 1)), RealmAny.create(now)) | ||
} | ||
RealmAny.Type.FLOAT -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values here should be something that cannot be implicitly cast to a Long
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The toLong()
actually truncates it, so can't really choose other good numbers. I removed the toLong()
implementation in RealmAny.equals
and are now relying on inner value's equals
so should behave according to Kotlins semantics now ... which I would expect makes most sense!?
packages/test-base/src/commonTest/kotlin/io/realm/kotlin/test/common/RealmAnyTests.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking fine, but there seems to be a CI error that looks slightly concerning. Not 100% why this PR would cause that though.
packages/test-base/src/commonTest/kotlin/io/realm/kotlin/test/common/RealmAnyTests.kt
Show resolved
Hide resolved
This is failing because the serialization round trip will loose the nano-second precision as it is not representable in EJson and the RealmAny comparison does not catch the inequality prior to the update. |
e223d4b
to
9de45cb
Compare
No description provided.