sqldelight icon indicating copy to clipboard operation
sqldelight copied to clipboard

unable to reference tables defined in common sqm file with deriveSchemaFromMigrations = true

Open sphrak opened this issue 2 years ago • 17 comments

SQLDelight Version

2.0.0-alpha03

Operating System

Android 11

Gradle Version

7.5.1

Kotlin Version

1.7.10

Dialect

app.cash.sqldelight:sqlite-3-35-dialect

AGP Version

7.2.2

Describe the Bug

Hello, i am not sure if this is a bug or if im doing something wrong. I have a project with common tables that both my client and server db uses.

When using deriveSchemaFromMigrations = true (on both client, server and common) -- from the consuming module point of view I cannot reference tables defined in the common sqm file (ie common/migrations/1.sqm).

Now I don't know if that should even be possible, but I'm not sure how else I would go about this because when using deriveSchemaFromMigrations = true I was under the impression that queries goes into .sq files and .sqm takes care of the actual table schema..

-- mycommonmodule/common/migrations/1.sqm
CREATE TABLE user (
    id INTEGER PRIMARY KEY AUTOINCREMENT,
    username TEXT NOT NULL
);

-- myservermodule/server/migrations/1.sqm
CREATE TABLE todo (
    id INTEGER PRIMARY KEY AUTOINCREMENT,
    title TEXT NOT NULL,
    description TEXT,
    user_id INTEGER

    FOREIGN KEY (user_id) REFERENCES user(id) <-- gives 'No table found with name user'
    ON DELETE CASCADE
    ON UPDATE CASCADE
);

edit: thinking more about this, it doesn't make sense for this to be possible. But I think it should be possible if I depend on a module that does not have deriveSchemaFromMigrations = true, however that suffers from the same problem.

Stacktrace

No table found with name user

Gradle Build Script

No response

sphrak avatar Sep 18 '22 16:09 sphrak

I think it makes sense, although I often use different Gradle modules instead.

hfhbd avatar Sep 19 '22 14:09 hfhbd

@hfhbd hmm these are different gradle modules, or how do you mean?

sphrak avatar Sep 19 '22 16:09 sphrak

Oh I thought with common you mean commonMain: so 1 gradle module with different kotlin source sets, which should also work in theory, but I never tested it.

So you already use different gradle modules? Do you config the database dependencies correctly in sqldelight block? Take a look at https://github.com/cashapp/sqldelight/tree/master/sqldelight-gradle-plugin/src/test/diamond-dependency

hfhbd avatar Sep 19 '22 16:09 hfhbd

@hfhbd ah sorry for the confusion then, yes different two different client and server modules that depend on a shared "common" module just like in the diamond-dependency test you linked to. It works as expected when deriveSchemaFromMigrations=false its just when its enabled that it complains that it cannot find the table.

sphrak avatar Sep 19 '22 18:09 sphrak

Thats very strange, it should work. Could you upload a reproducer or share all gradle files?

hfhbd avatar Sep 19 '22 18:09 hfhbd

@hfhbd sure, here you go shared-sqm-files-sample.zip

client/1.sqm cannot reference user table defined in shared/1.sqm

sphrak avatar Sep 20 '22 10:09 sphrak

Thanks. I am able to reproduce it with some changes: shared-sqm-files-sample.zip You need a "normal" dependency on the shared project too

Issues:

  • [x] Generate a database even if there are no queries: Rare use case because I would always create some crud queries, even to test the schema but sqldelight should support this use case https://github.com/cashapp/sqldelight/issues/3518
  • [ ] Dependencies do not work in schema/migration files. They work in query files. This is this issue.

hfhbd avatar Sep 20 '22 10:09 hfhbd

@hfhbd man you are really fast, yes I realize I missed that -- in my other project I have a normal dependency so im experiencing the issues you listed then

sphrak avatar Sep 20 '22 10:09 sphrak

@sphrak I just opened the project :D But this issue comes from sql-psi which has no support for dependencies. So the question is, how does sqldelight inject the tables to sq files 🤔 Okay, it just copy them virtually by setting the file index.

hfhbd avatar Sep 20 '22 10:09 hfhbd

Okay, I found a solution: The 1.sqm in clients should have a higher version than 1 otherwise sqldelight does not know, which file should be the first one. Using the dependency graph as execution order does not work, if you have a diamond graph:

Simple structure, which works:

- shared: 1.sqm
- shared: 2.sqm
- - client: 1.sqm
- - client: 2.sqm

expected: run shared/1.sqm -> client/1.sqm -> shared/2.sqm -> client/2.sqm Todo: add a test with 1,2,3,4

But if you have a diamond dependency, this will fail:

- shared: 1.sqm
- shared: 2.sqm
- client: 1.sqm
- client: 2.sqm
- - ios: 1.sqm
- - ios: 2.sqm

Which one should come first, shared or client? shared/1.sqm -> client/1.sqm -> ios/1.sqm -> shared/2.sqm -> client/2.sqm -> ios/2.sqm client/1.sqm -> shared/1.sqm -> ios/1.sqm -> client/2.sqm -> shared/2.sqm -> ios/2.sqm

Maybe we could add an error to have distinct migration numbers. This would be a aceptable fix.

hfhbd avatar Sep 20 '22 12:09 hfhbd

@hfhbd hmm, tricky question not quite sure i'm following -- you mean no repetition of migration numbers in the entire dependency graph? so if 1.sqm is defined, then it cannot be used anywhere else in that graph? could an evolution of a db look like this then?

shared/1.sqm
client/2.sqm
server/3.sqm
client/4.sqm
shared/5.sqm
shared/6.sqm
shared/7.sqm
client/8.sqm

sphrak avatar Sep 20 '22 12:09 sphrak

@sphrak Yes, exactly.

hfhbd avatar Sep 20 '22 12:09 hfhbd

@hfhbd got it, but that would still require more work than adding an error message about distinct numbers?

sphrak avatar Sep 20 '22 15:09 sphrak

@sphrak It does? (I don't know) What use-case do you mean?

hfhbd avatar Sep 20 '22 15:09 hfhbd

@hfhbd I mean in the sample you sent back, i still get this:

shared-sqm-files-sample/client/src/main/sqldelight/com/example/migrations/1.sqm: (7, 42): No column found with name id
01    CREATE TABLE todo (
02        id INTEGER PRIMARY KEY AUTOINCREMENT,
03        title TEXT NOT NULL,
04        description TEXT,
05        user_id INTEGER,
06        FOREIGN KEY (user_id) REFERENCES user(id)
07        ON DELETE CASCADE
                                                ^^
08        ON UPDATE CASCADE
09    )

sphrak avatar Sep 20 '22 18:09 sphrak

@sphrak If I change clients/1.sqm to clients/2.sqm it works.

hfhbd avatar Sep 20 '22 18:09 hfhbd

@hfhbd yes you are right, it does compile its just the IDE plugin that cant resolve it image

sphrak avatar Sep 20 '22 18:09 sphrak

Maybe, I got a better way to solve diamond dependencies. We could use the order of the dependency specified in the gradle plugin. So first running all migrations of shared, then all of clients and finally iOS.

hfhbd avatar Mar 30 '23 10:03 hfhbd

@AlecStrong I added it to 2.0. Do you want it in this release?

hfhbd avatar Mar 30 '23 10:03 hfhbd

i think so, this sounds like a bug to me and less of a feature

AlecKazakova avatar Mar 30 '23 11:03 AlecKazakova

So migration is still an incomplete feature? I had the same problem. I need to change the primary key in the data table, but I can't reference the data table.

yangwuan55 avatar Nov 20 '23 10:11 yangwuan55