Skip to content

[REQ] Kotlin, request for feedback on proposed model pattern for 3.1.0 spec processing #14657

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

Closed
spacether opened this issue Feb 9, 2023 · 14 comments

Comments

@spacether
Copy link
Contributor

spacether commented Feb 9, 2023

How does the kotlin client generator handle properties with invalid names? One could inherit from HashMap and make a class where one could create enums for each known key. And use those to perform type safe extraction of map values.
Here is an example which works for:

  • keys with invalid kotlin variable names
  • allows type definition of additional properties also
  • allows normal map usage w/ string keys too

For example given this openapi component schema:

MyMap:
  type: object
  properties:
    #:
      type: string
    "1":
      type: integer
    "none":
      type: "null",
  additionalProperties:
    type: boolean

Here is a sample Kotlin Class:

interface MyMapKey {
    fun value(): String
}

class MyMap: HashMap<String, Any?>() {
    // this is an example of how one could write an openapi type object model
    class Keys_ {
        // red, one , none
        enum class DollarSignEnum(val value: String): MyMapKey {
            DOLLAR_SIGN("$");  // value type is String
            override fun value(): String {
                return this.value
            }
        }

        enum class OneEnum(val value: String): MyMapKey {
            ONE("1");  // value type is Int
            override fun value(): String {
                return this.value
            }
        }

        enum class NoneEnum(val value: String): MyMapKey {
            NONE("none");  // value type is null
            override fun value(): String {
                return this.value
            }
        }

        companion object {
            val DOLLAR_SIGN = DollarSignEnum.DOLLAR_SIGN
            val ONE = OneEnum.ONE
            val NONE = NoneEnum.NONE
            val propertyKeyToEnum: Map<String, MyMapKey> = mapOf(
                "$" to DOLLAR_SIGN,
                "1" to ONE,
                "none" to NONE
            )
            fun isAdditionalProperty(propertyName: String): Boolean {
                return !propertyKeyToEnum.containsKey(propertyName)
            }
        }
    }

    operator fun get(key: Keys_.DollarSignEnum): String {
        return super.get(key.value).toString()
    }

    operator fun get(key: Keys_.OneEnum): Int {
        return super.get(key.value) as Int
    }

    operator fun get(key: MyMapKey): Any? {
        return super.get(key.value())
    }

    fun getAdditionalProperty(key: String): Boolean {
        if (Keys_.isAdditionalProperty(key)) {
            return super.get(key) as Boolean
        }
        throw IllegalArgumentException("key was not an additional property")
    }

    operator fun get(key: Keys_.NoneEnum): Nothing? {
        val value = super.get(key.value)
        if (value == null) {
            return null
        }
        throw IllegalStateException("value was not null and it must be null")
    }
}

Example usage code:

import org.junit.Test

class MyMapTest {
    @Test
    fun testMyMap() {
        val mapData = mapOf(
            "$" to "hi",
            "1" to 123,
            "none" to null,
            "add-Prop" to true
        )
        val castMap = MyMap()
        castMap.putAll(mapData)

        val dollarSignVal = castMap[MyMap.Keys_.DOLLAR_SIGN] // type is String
        val oneVal = castMap[MyMap.Keys_.ONE] // type is Int
        val noneVal = castMap[MyMap.Keys_.NONE] // type is Nothing?
        if (MyMap.Keys_.isAdditionalProperty("add-Prop")) {
            val addPropVal = castMap.getAdditionalProperty("add-Prop")  // type is Boolean
        }

        val dollarSignValue = castMap["$"] // type is Any? and is actually String
        val oneValue = castMap["1"] // type is Any? and is actually Int
        val noneValue = castMap["none"] // type is Any? and is actually null
        val addPropValue = castMap.getAdditionalProperty("add-Prop")  // type is Any? and is actually Boolean

    }
}

What do people think of this solution?
This could be a way of implementing type object models which allows:

  • typed access
  • typed additional property access
  • key names to have invalid Kotlin names + still have typed access
  • object models would have normal map methods also and required properties could have accessor functions defined for them when the keys have valid kotlin variable names.

Note:
A solution like this would also work for ArrayList (json schema type array) in openapi 3.1.0. In that version of the spec, array values depend upon the index being retrieved via prefixItems + items definitions.

@spacether
Copy link
Contributor Author

spacether commented Feb 9, 2023

@gerak-cz
Copy link

I am not sure I understand your proposal completely.

Having enum with a value property is definitely one solution. Could even be value class?

What about using backticks? Would that work?

@spacether
Copy link
Contributor Author

spacether commented Feb 15, 2023

The keys of required and optional properties could be value classes but only one value would ever be used. So I think that enums make more sense for known keys. For additional properties the key could be a value class.

What is a code sample of how back ticks would work?

@gerak-cz
Copy link

As I test this, the backticks really only allow you to put numbers at the start of a variable name and escape the keywords.

They do allow more but only in test method names. So they are not useful here.

@Drekorian
Copy link

@spacether, backticks need to be used for properties that would match Kotlin's reserved keywords. For a full reference, see Kotlin docs: Keywords and operators. Is that the answer to your question?

keys with invalid kotlin variable names

@spacether
Copy link
Contributor Author

Do people prefer the above solution or explicit getters instead?

@Drekorian
Copy link

The primary need for backticks arose from compatibility with Java libraries. For example, one may want to use Hamcrest's Is.is() matcher method, however is is a Kotlin reserved keyword. Therefore it's possible to call the function via `is`() instead.

Given the choice between

foo.`value`

and

foo.getValue()

I believe the latter is more preferable since it provides a universal access towards properties.

@spacether
Copy link
Contributor Author

spacether commented Jul 14, 2023

Having getters will not easily cover corner cases for similarly named variables like

  • someProp
  • SomeProp
  • some-prop

One model can have all of those properties and the getter should not collide. Advanced getter logic would need to be written to handle that use case.

@Drekorian
Copy link

Drekorian commented Jul 15, 2023

One model can have all of those properties and the getter should not collide.

Sure. Such design would be quite terrible. Generating getters (e.g., fun getSomeProp()) would create colliding names for someProp and SomeProp. Moreover, generating both:

fun getSomeProp()   // someProp
fun `getSomeProp`() // SomeProp

is not allowed. It will result in colliding names. However, generating all three of them as backticked properties:

val `someProp`: Any
val `SomeProp`: Any
val `some-prop`: Any

seems like a pretty big sacrifice of convenience for the sake of a corner case. I would still like to see backticks only when the naming is not allowed in Kotlin (e.g., val some-prop: Any) or for the reserved keywords.

@spacether
Copy link
Contributor Author

This corner case is allowed by json schema, though unlikely. It looks like proto gets around this by requiring that variables follow their style guide (lowercase with underscores) which prevents collision.
https://protobuf.dev/reference/java/java-generated/

@spacether
Copy link
Contributor Author

It would be interesting and useful to analyze specs in the wild and see how many key names

  • follow recommended naming practices
  • follow possible but not accepted naming
  • would not work as a variable name
  • have getters that would collide with adjacent keys

@spacether
Copy link
Contributor Author

spacether commented Jul 17, 2023

Can someone point me to a regex or implemntation that can check if a string is a valid Kotlin identifier?
I analyzed required keys in 1,878 openapi documents and checked for:

  • keys that would collide with python reserved words
  • keys with invalid identifiers and how they could be most easily fixed

If someone can get me the Kotlin info, I can run the numbers for Kotlin too.
I will also check this for property keys.

@spacether
Copy link
Contributor Author

Another option for this very rare corner case (1.4%) of all required keys is for keys that can't produce valid in-language identifiers, depend up on the developer to extract the value from a map using map.get(key)
The reason that this is bothering me is that to properly pass all json schema validations, the keys must be preserved, at least at ingestion time.

  • it can be confusing to have different inputs and outputs that both correspond to the same variable
  • if the inputs were kept the same as the outputs
    • inputs would immediately need to be converted to their map value in the constructor
    • a different constructor would need to be written to be able to ingest json input (not using output var names)

@Drekorian
Copy link

I guess the closest thing is the propertyDeclaration in Kotlin grammar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants