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

Compilation error in KotlinGenerator generated code when GeneratorStrategy produces identifiers conflicting with property access syntax of AbstractRoutine getters #16006

Closed
reubenfirmin opened this issue Jan 4, 2024 · 8 comments

Comments

@reubenfirmin
Copy link

reubenfirmin commented Jan 4, 2024

Use case

From a style point of view, devs may prefer table names to be lowercased (uppercase is LOUD). Using a generator strategy in kotlin, you can do something like:

class SingularizingStrategy() : DefaultGeneratorStrategy() {
    override fun getJavaClassName(definition: Definition?, mode: GeneratorStrategy.Mode?): String {
        val defaultName = super.getJavaClassName(definition, mode)
        return defaultName.singularize()
    }

    override fun getJavaIdentifier(definition: org.jooq.meta.Definition): String {
        return super.getJavaIdentifier(definition).lowercase()
    }
}

This generally works, but certain generated classes won't compile. For example, if you use a uuid generating function in your schema:

CREATE TABLE person
(
    id uuid primary key default uuid_generate_v4(),
    ...

then the generated code for UUIDGeneratorV3 and V5 (though not V4) will have broken code:

open class UuidGenerateV3 : AbstractRoutine<UUID>("uuid_generate_v3", rcp.model.jooq.Public.`public`, SQLDataType.UUID) {
    companion object {

        /**
         * The parameter <code>public.uuid_generate_v3.RETURN_VALUE</code>.
         */
        val return_value: Parameter<UUID?> = Internal.createParameter("RETURN_VALUE", SQLDataType.UUID, false, false)

        /**
         * The parameter <code>public.uuid_generate_v3.namespace</code>.
         */
        val namespace: Parameter<UUID?> = Internal.createParameter("namespace", SQLDataType.UUID, false, false)

        /**
         * The parameter <code>public.uuid_generate_v3.name</code>.
         */
        val name: Parameter<String?> = Internal.createParameter("name", SQLDataType.CLOB, false, false)
    }

    init {
        returnParameter = return_value
        addInParameter(namespace)
        addInParameter(name) // <--
    }
    ...

Here "name" is meant to reference the companion object name, but in Kotlin at least it fails to resolve to anything. It seems like the naming strategy 95% works, with just some edge cases that need disambiguation.

Possible solution

No response

Possible workarounds

Using capitalized words is still less loud, and seems to work reasonably:

return super.getJavaIdentifier(definition).lowercase().capitalized()

jOOQ Version

Jooq open source 3.18.6

Database product and version

Postgres 15

Java Version

OpenJDK version 17

OS Version

Ubuntu 23.10

JDBC driver name and version (include name if unofficial driver)

org.postgresql:postgresql:42.5.1

@lukaseder
Copy link
Member

Thanks for your report. I'm assuming this is about the functions from this extension?

CREATE EXTENSION IF NOT EXISTS "uuid-ossp";

I'll try reproducing this soon.

@reubenfirmin
Copy link
Author

@lukaseder yeah I guess so, though there may be other cases.

@lukaseder
Copy link
Member

I see now. This isn't specific to that particular function (or PostgreSQL), it can happen with any other function. E.g. I've created this function in an HSQLDB database, and it reproduces the problem:

CREATE FUNCTION stored_functions.f_name (name int)
RETURNS int
BEGIN ATOMIC
    RETURN name;
END

The problem is that it seems the property access convenience syntax resolves before the companion object access syntax.

Making the identifier explicit would solve the problem here:

addInParameter(FName.name)

This could happen for any of the getters in AbstractRoutine:

image

At least for those where the casing will conflict (single word only, if you don't maintain camel casing). For example, this function will produce conflicts on all parameters:

CREATE FUNCTION stored_functions.f_name (name int, catalog int, package int, results int)
RETURNS int
BEGIN ATOMIC
    RETURN name;
END

There are at least two solutions for this:

  • Try to re-implement the property convenience access logic from Kotlin and qualify only those identifiers where there is a suspected conflict
  • Just qualify all of these identifiers in AbstractRoutine subclasses. This could address also other name resolution edge cases that I can't think of right now.

Other solutions might include avoiding the companion object, but that's probably more complicated, and might lead to new edge cases.

I'm probably going for the second option.

@lukaseder
Copy link
Member

lukaseder commented Jan 5, 2024

Also, not just the addInParameter(name) call is affected, but also setters below:

    /**
     * Set the <code>NAME</code> parameter IN value to the routine
     */
    internal fun setName_(value: Int?): Unit = setValue(name, value) // Same problem here

    /**
     * Set the <code>NAME</code> parameter to the function to be used with a
     * {@link org.jooq.Select} statement
     */
    internal fun setName_(field: Field<Int?>): Unit {
        setField(name, field) // Same problem here
    }

@lukaseder
Copy link
Member

Interestingly, we already do this qualification in the ScalaGenerator

@lukaseder lukaseder changed the title Allow lowercasing of table names Compilation error in KotlinGenerator generated code when GeneratorStrategy produces identifiers conflicting with property access syntax of AccessRoutine getters Jan 5, 2024
@lukaseder lukaseder changed the title Compilation error in KotlinGenerator generated code when GeneratorStrategy produces identifiers conflicting with property access syntax of AccessRoutine getters Compilation error in KotlinGenerator generated code when GeneratorStrategy produces identifiers conflicting with property access syntax of AbstractRoutine getters Jan 5, 2024
@lukaseder
Copy link
Member

lukaseder commented Jan 5, 2024

I'm probably going for the second option.

Hmm, IntelliJ will complain about "redundant qualifier name" in cases where this doesn't happen, i.e. in 99.9% of all cases. Perhaps, I will indeed try option 1 instead. Unlikely. Users can have their own subclass of AbstractRoutine placed into the type hierarchy, and there might be other conflicts that we cannot detect. I'll just do the same thing as in the ScalaGenerator.

3.14 Better Kotlin and Scala Support automation moved this from To do to Done Jan 5, 2024
@reubenfirmin
Copy link
Author

Thanks!

lukaseder added a commit that referenced this issue Jan 5, 2024
when GeneratorStrategy produces identifiers conflicting with property
access syntax of AbstractRoutine getters
lukaseder added a commit that referenced this issue Jan 5, 2024
when GeneratorStrategy produces identifiers conflicting with property
access syntax of AbstractRoutine getters
lukaseder added a commit that referenced this issue Jan 5, 2024
when GeneratorStrategy produces identifiers conflicting with property
access syntax of AbstractRoutine getters
lukaseder added a commit that referenced this issue Jan 5, 2024
when GeneratorStrategy produces identifiers conflicting with property
access syntax of AbstractRoutine getters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants