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

Design flaws with insert #1727

Open
spand opened this issue Apr 20, 2023 · 9 comments
Open

Design flaws with insert #1727

spand opened this issue Apr 20, 2023 · 9 comments
Assignees

Comments

@spand
Copy link
Contributor

spand commented Apr 20, 2023

For inserts Exposed will (for columns with no explicit value) populate all inserts with defaults/null from the Exposed table object and not actually use the one defined in the actual table on the database by leaving out the column in the insert statement.

This has a couple of problems:

  • It is impossible to actually perform an INSERT using the database default value. ie. it may not be known or controlled by another team.
  • It boils down to a requirement to keep the code in sync with the database but this is at odds with always on services that by definition must have new and old code running on the database at the same time.

A more flexible and less opinionated design would involve:

  • Only use .default() and .defaultExpression() for table creation
  • Open for simultaneous use of .clientDefault() (and .default()) which then serves as the application default (This is how I always assumed it worked) instead of having .default() act as both table and client insert defaults.
  • Not have an implicit default of NULL for nullable columns. This is a requirement for first bullet.

A note is that I am not talking about Exposed Dao but the plain query dsl. Looking at the git history it seems that these defaults were introduced as opinionated defaults for dao which I guess makes much more sense (other than they are still impossible to opt out of). They spilled over into the dsl though which I find problematic since the dsl ought to support all (or atleast standard) sql. I assume the current behaviour can be moved a layer up into the dao code to keep the behavior of dao intact.

I might be willing to do the bulk of the work involved in this but would like to confirm if it would be desirable or not.

Similar issue #345

@darmstrong1
Copy link

I agree with the solution proposed by @spand and would like to know if it might be implemented.
I added a comment to #1558 that could also apply to this one.

@e5l
Copy link
Member

e5l commented Jun 21, 2024

@obabichevjb, could you check if we can solve this?

@obabichevjb obabichevjb self-assigned this Jun 25, 2024
@obabichevjb
Copy link
Collaborator

It looks like the problem is already solved if I understand the problem correct.

If the problem is that Exposed aggressively requires to define values for all the fields, and does not allows to rely on the db side values generation, it could be solved using column method databaseGenerated() (and withDefinition() additionally)

Actually, at the current moment there are 3 ways (examples are made with PostgresDB):

  1. default()/defaultExpression() (customizes column DDL, and applied to insert statements)
object TestTable : IntIdTable("test") {
    val key = uuid("key")
        .default(UUID.fromString("a344698c-3c0d-46a7-9847-dc308665d9c8"))
}

TestTable.insert {  }

Performed sql:

CREATE TABLE IF NOT EXISTS test (
  id SERIAL PRIMARY KEY, 
  "key" uuid DEFAULT 'a344698c-3c0d-46a7-9847-dc308665d9c8' NOT NULL
)

INSERT INTO test ("key") VALUES ('a344698c-3c0d-46a7-9847-dc308665d9c8')
  1. clientDefault() (does not affect DDL):
object TestTable : IntIdTable("test") {
  val key = uuid("key")
      .clientDefault { UUID.randomUUID() }
}

TestTable.insert {  }
CREATE TABLE IF NOT EXISTS test (
  id SERIAL PRIMARY KEY, 
  "key" uuid NOT NULL
)

INSERT INTO test ("key") VALUES ('e69515f6-6bed-4c95-aa75-9c0ba7a0ac77')
  1. databaseGenerated().withDefinition() (this variant modifies table column, and the value is skipped for insert)
object TestTable : IntIdTable("test") {
    val key = uuid("key")
        .databaseGenerated()
        .withDefinition("DEFAULT gen_random_uuid()")
}

TestTable.insert {  }
CREATE TABLE IF NOT EXISTS test (
  id SERIAL PRIMARY KEY, 
  "key" uuid DEFAULT gen_random_uuid() NOT NULL
)

INSERT INTO test  DEFAULT VALUES

Looking at these variants, it's probably, that users could be confused, because there are variants for client-side generation, db-side generation and magic default() that applied to both.

Probably we actually do not need the default() variant, because it customized DDL to generate values on the database side, but after that anyway puts the same values into the queries.

Probably it would be better to have only two options (client or db side generations) and consistent methods set like:

clientGenerated(value)
clientGeneratedByExpression(expression: )
databaseGenerated()
databaseGeneratedByDefinition(definition)

I think that the most interesting question here: are there any use cases when default() can not be replaced by clientDefault() / databaseGenerated() options? If there are no such use cases we may mark default() as deprecated at least. @e5l @bog-walk what do you think about it?

Anyway, it's just some proposals of how to make it more consistent. According to the initial question of the issue, it looks to me like the problem is gone, if not, let me know about it.

@spand
Copy link
Contributor Author

spand commented Jun 26, 2024

I dont have time to create a comprehensive response but just to keep the ball rolling. I still think my proposal makes the most sense. I was not aware databaseGenerated() but I really dont see a need for it design wise. The api is actually pretty clear:

    /** Sets the default value for this column in the database side. */
    fun <T> Column<T>.default(defaultValue: T): Column<T>
    
    /** Sets the default value for this column in the client side. */
    fun <T> Column<T>.clientDefault(defaultValue: () -> T): Column<T>

Exposed just needs to live up to its api. If we follow my proposal we can drop databaseGenerated() and the api stays clear and simple.

Edit:
Reading it again it seems @obabichevjb concludes the same (just two options). I just propose we use the set of methods that are already in place. The default naming is more in line with actual sql rather than inventing new terms like xxGenerated().

@wissil
Copy link

wissil commented Jun 29, 2024

I think the issue I just reported might be related to this.

@obabichevjb
Copy link
Collaborator

@wissil

I dived deeper into the problem, and got some more information and understandings.

I created a proposal EXPOSED-437 Default values API with some ideas of how defaults API could be refactored.

Shortly: we should not explicitly put the default values into insert statements if it is created inside the database. We have more scenarios (more than 2 mentioned above). This is because Exposed uses the default value internally if it can (for example to provide the value with DAO entity without an actual request to the database), so I found 5 different cases mentioned in the proposal.

Feel free to check also and let me know if you see any potential problems there.

@mrfrosk
Copy link

mrfrosk commented Oct 14, 2024

The question may be off-topic, but I couldn't find a better place to ask it. How can I automatically create a database in Exposed? In the Exposed Spring Boot starter, I only found information about how to automatically create the schema.

@obabichevjb
Copy link
Collaborator

@mrfrosk In general, you can try to address it in Exposed's Slack channel.

According to you question, what do you mean by "create database"? Do you mean to create DB from scratch, or to create tables of your model inside existing DB?

If you already have database, and need to create tables you have several options.

The simplest one (if we're talking about Exposed) is to use Exposed's SchemaUtils class, that allows to manipulate DB structure from your code (like creating, deleting tables). So you can perform such actions on the start of your application or separately.

If you want more reliable (and production ready options) I'd suggest to use special migration tools (like flyway or liquibase). The migrations itself for that tools could be also created using SchemaUtils.

@mrfrosk
Copy link

mrfrosk commented Oct 16, 2024

@mrfrosk In general, you can try to address it in Exposed's Slack channel.

According to you question, what do you mean by "create database"? Do you mean to create DB from scratch, or to create tables of your model inside existing DB?

If you already have database, and need to create tables you have several options.

The simplest one (if we're talking about Exposed) is to use Exposed's SchemaUtils class, that allows to manipulate DB structure from your code (like creating, deleting tables). So you can perform such actions on the start of your application or separately.

If you want more reliable (and production ready options) I'd suggest to use special migration tools (like flyway or liquibase). The migrations itself for that tools could be also created using SchemaUtils.

By creating a database, I mean the following. For example, in the yaml file or application.properties I indicate the name of the database and if it does not exist, then Exposed creates it

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

No branches or pull requests

6 participants