Fix parsing unknown fields breaks record deserialisation #666
Conversation
- cleaned up decryptRecord()
| ) { | ||
| override fun equals(other: Any?): Boolean { | ||
| if (this === other) return true | ||
| if (javaClass != other?.javaClass) return false | ||
|
|
||
| other as KeeperFile | ||
|
|
||
| if (!fileKey.contentEquals(other.fileKey)) return false | ||
| if (fileUid != other.fileUid) return false | ||
| if (data != other.data) return false | ||
| if (url != other.url) return false | ||
| if (thumbnailUrl != other.thumbnailUrl) return false | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| override fun hashCode(): Int { | ||
| var result = fileKey.contentHashCode() | ||
| result = 31 * result + fileUid.hashCode() | ||
| result = 31 * result + data.hashCode() | ||
| result = 31 * result + url.hashCode() | ||
| result = 31 * result + (thumbnailUrl?.hashCode() ?: 0) | ||
| return result | ||
| } | ||
| } |
There was a problem hiding this comment.
We have a bytearray in this data class which means we need to implement our own equals and hashcode
There was a problem hiding this comment.
No, we don't do equality checks on this class
There was a problem hiding this comment.
That hashcode is more important here - if this ever goes in a hashed data structure without it it won't work
| if (decryptedRecord != null) { | ||
| records.add(decryptedRecord) | ||
| } | ||
| decryptRecord(it, recordKey).onSuccess { keeperRecord -> records.add(keeperRecord) } |
There was a problem hiding this comment.
this is the magic part where if there's a failure it skips gracefully (i.e. onSuccess only gets called if we ge a successful Result)
| val decryptedRecordFiles = encryptedRecord.files?.map { recordFile -> | ||
| val fileKey = decrypt(recordFile.fileKey, recordKey) | ||
| val decryptedFile = decrypt(recordFile.data, fileKey) | ||
| KeeperFile( | ||
| fileKey, | ||
| recordFile.fileUid, | ||
| Json.decodeFromString(bytesToString(decryptedFile)), | ||
| recordFile.url, | ||
| recordFile.thumbnailUrl | ||
| ) | ||
| } ?: emptyList() |
There was a problem hiding this comment.
functional > imperative (plus why allocate when you don't need to?)
| ) | ||
| } ?: emptyList() | ||
|
|
||
| val recordData: KeeperRecordData = try { |
There was a problem hiding this comment.
Restructuring this ensures that there are only two non exception pathways out - either you get the record data you want, or you don't, and both are ok.
idimov-keeper
changed the base branch from
master
to
release/sdk/java/core/v16.6.6
borrow-checker
changed the title
…nto fix/parsing-unknown-fields # Conflicts: # sdk/java/core/src/main/kotlin/com/keepersecurity/secretsManager/core/SecretsManager.kt
|
There is no jira ticket for this. Can’t release to prod w/o a ticket
…On Tue, Oct 8, 2024 at 5:13 AM TJ Challstrom ***@***.***> wrote:
Merged #666 <#666>
into release/sdk/java/core/v16.6.6.
—
Reply to this email directly, view it on GitHub
<#666 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABNXKUHV3EIBYGU6SOE52DZ2PD7NAVCNFSM6AAAAABO3RCGP2VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGU2TKNZUGAZDINA>
.
You are receiving this because your review was requested.Message ID:
***@***.***
com>
|
How apt that this PR is #666. 😈 |
This PR fixes an issue where deserialising a record with unknown fields breaks deserialisation of all records. With this change, an error message is displayed and the record is skipped.