Skip to content
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

KTOR-1264 - Add UUID to DefaultConversionService #2188

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

hfhbd
Copy link
Contributor

@hfhbd hfhbd commented Nov 8, 2020

Subsystem
Server-Core, JVM, DefaultConversionService

Motivation
Add UUID to DefaultConversionService - https://youtrack.jetbrains.com/issue/KTOR-1264

val id: UUID by call.parameters

Solution
Added UUID

@hfhbd hfhbd changed the title Ktor 1264 KTOR-1264 Nov 8, 2020
@hfhbd hfhbd changed the title KTOR-1264 KTOR-1264 - Add UUID to DefaultConversionService Nov 8, 2020
@cy6erGn0m cy6erGn0m self-requested a review November 9, 2020 06:47
@cy6erGn0m cy6erGn0m self-assigned this Nov 9, 2020
@cy6erGn0m
Copy link
Contributor

I remember we already had similar PR in the past. The problem with UUID is that it is JVM-only. Once one starts using it on the server, it appears to be logical to have it in client. However, in the client, it only works on JVM but not on iOS. So it's preventing seamless porting.
On the other hand, we already have support for several JVM-only types like BigInteger or BigDecimal So we potentially can try adding it.
The other solution would be to write our own multiplatform UUID implementation, but it requires additional design and more effort to introduce.

@cy6erGn0m cy6erGn0m added this to the 1.5.0 milestone Nov 9, 2020
@hfhbd
Copy link
Contributor Author

hfhbd commented Nov 9, 2020

@cy6erGn0m Yeah, working on a MPP with UUID (not only ktor, but exposed etc also) is very annoying, until Kotlin will finally have a common UUID implementation...

@cy6erGn0m
Copy link
Contributor

Ok, let me see if we can add our own UUID at some point. I need to investiage the problem and measure costs and risks.

@hfhbd
Copy link
Contributor Author

hfhbd commented Nov 9, 2020

Here is a very simple UUID4 implementation using typealias in JVM to support older frameworks. Alternativ: Use converter function like in Kotlinx.Datetime.

// Common
expect class UUID

//JVM
actual typealias UUID = java.util.UUID

// JS
actual /* inline */ class UUID(private val uuidString: String) {

    init {
        require(uuidString matches uuid4Regex) { "$uuidString is not a UUIDv4" }
    }

    fun version(): Int = 4
    fun variant(): Int = 1

    companion object {
        private val uuid4Regex = Regex("[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}", RegexOption.IGNORE_CASE)

        fun randomUUID(): UUID {
            val randomBytes = Random.nextUBytes(16)
            val clearVersion: UByte = 0x0fu
            val version4: UByte = 0x40u
            val clearVariant: UByte = 0x3fu
            val variant: UByte = 0x80u
            randomBytes[6] = randomBytes[6] and clearVersion
            randomBytes[6] = randomBytes[6] or version4
            randomBytes[8] = randomBytes[8] and clearVariant
            randomBytes[8] = randomBytes[8] or variant
            return UUID(randomBytes.formatUUID())
        }

        private fun UByteArray.formatUUID(): String = joinToString("") {
            val char = it.toInt()
            if (char <= 15) {
                "0" + char.toString(16)
            } else {
                char.toString(16)
            }
        }.let {
            StringBuilder(it).apply {
                insert(8, "-")
                insert(12 + 1, "-")
                insert(16 + 2, "-")
                insert(20 + 3, "-")
            }.toString()
        }
    }

    override fun toString() = uuidString
}

@cy6erGn0m
Copy link
Contributor

cy6erGn0m commented Nov 9, 2020

Still a lot of open questions anyway:

  • why it is secure on JVM and it is not on JS?
  • what if one needs a hash-based UUID?
  • what is hashCode/equals contract? Should it be the same across platforms? Comparable?
  • what about serialization? How does one serialize it with kotlinx.serialization?
  • finally, why it is ktor who holds the type? shouldn't it be a separate 3rd party lib or even stdlib (no)?

When introducing such a basic type, it is always difficult because it's not enough to just ad-hoc it, you need to design it 😞

@cy6erGn0m
Copy link
Contributor

cy6erGn0m commented Nov 9, 2020

So, the first question: why do you need it? What is your use-case, why don't you use regular String and why it is so boring so that you made this PR?

How do you use UUID:

  • where it comes from?
  • do you generate them? does it make sense to have UUID if you don't?
    • if you do, how do you fight secure random seeding issues?
  • do you store it somewhere in a database, using ORM or something?

@hfhbd
Copy link
Contributor Author

hfhbd commented Nov 9, 2020

Thank you for your feedback!

One basic use case is a multiplatform application, eg a Todo service.
The backend uses Ktor + Exposed on JVM.
The frontend uses Kotlin/JS, without a db, because its always online, ignoring offline web-apps...
The Android app uses Room as DB with common code and the iOS app Kotlin/Native and CoreData as DB.

The main advantage using UUID over an Integer as the identifier is the possibility creating a new Todo offline without sync conflicts. Because a UUIDv4 is "unique", the probability having the same UUID in your application scope is extremely low (see https://en.wikipedia.org/wiki/Universally_unique_identifier#Collisions). You can upload this local generated id directly without problems to you server and store them in the server DB. The CloudKit framework by Apple uses this advantage.

Creating a UUID:

Compareable, hashable:
A UUID is either a 128 bit string or a 32 hex string (depending on the implementation), so comparing, hashCode/equals should always use the (bit/hex) string which has to be the same over all platforms.

Support for other UUID versions:
Yes, for a full design all versions needed to be supported.

Serialization:
This is indeed an open question. For JSON for example you might want to use the hex string as string, for photobuf you should use the bit string, see
Kotlin/kotlinx.serialization#1184. So when creating the expected builtin support you need to keep this in mind.

Absolute, it is way lot more difficult to design this "simple" class for public use with binary compatibility than implementing a UUID class in a non-public project without binary compatibility, which needs only UUIDv4 :)

I would love to see a UUID implementation in the Standard lib instead in Ktor! (https://youtrack.jetbrains.com/issue/KT-43042)

But unfortunately a UUID is still not implemented in the std lib.
Like base64Encoding, which is also missing in the std lib, but implement by yourself, you/I could implement a UUID for ktor.

Back to the initial "problem" of this pull request :)
As a current workaround, you could overload the getValue function.

operator fun Parameters.getValue(receiver: Any?, property: KProperty<*>): java.util.UUID =
    UUID.fromString(get(property.name))

I simple created this PR because in ConversionService you already have JVM only types.

@cy6erGn0m
Copy link
Contributor

@hfhbd See https://github.com/cy6erGn0m/kotlinx-uuid for prevew

@hfhbd
Copy link
Contributor Author

hfhbd commented Nov 14, 2020

@cy6erGn0m WOW! I did not see coming this so fast.

@cy6erGn0m
Copy link
Contributor

@hfhbd Remember that it's not even a real kotlinx library: it's not under the organization and it's status is undecided yet. I am planning to publish it to a separate bintray repo next week but I can't guarantee anything else for now.

@cy6erGn0m
Copy link
Contributor

@hfhbd added exposed support to the library

@hfhbd
Copy link
Contributor Author

hfhbd commented Nov 16, 2020

@cy6erGn0m Thanks! I did try to use it with my playground/experimental app ComposeTodo, and releasing the artifacts would be super.

@cy6erGn0m cy6erGn0m modified the milestones: 1.5.0, 1.6.0 Dec 22, 2020
@e5l e5l removed the request for review from cy6erGn0m June 29, 2021 09:26
@hfhbd hfhbd changed the base branch from main to 2.0.0-eap September 7, 2021 16:02
@hfhbd
Copy link
Contributor Author

hfhbd commented Sep 7, 2021

@rsinukov Rebased on 2.0.0-eap.

@rsinukov
Copy link
Contributor

rsinukov commented Sep 7, 2021

Good job, thanks!

@rsinukov rsinukov merged commit 142c141 into ktorio:2.0.0-eap Sep 7, 2021
@hfhbd hfhbd deleted the ktor-1264 branch September 7, 2021 20:57
e5l pushed a commit that referenced this pull request Sep 28, 2021
Co-authored-by: hfhbd <hfhbd@users.noreply.github.com>
e5l pushed a commit that referenced this pull request Oct 4, 2021
Co-authored-by: hfhbd <hfhbd@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants