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

Use Long as migration version #4297

Merged
merged 10 commits into from
Jun 27, 2023
Merged

Use Long as migration version #4297

merged 10 commits into from
Jun 27, 2023

Conversation

hfhbd
Copy link
Collaborator

@hfhbd hfhbd commented Jun 22, 2023

Fixes #4296

Draft because it needs a new sql-psi release (or use snapshot version)

@hfhbd hfhbd changed the title Use long as migration version Use Long as migration version Jun 22, 2023
@dellisd
Copy link
Collaborator

dellisd commented Jun 23, 2023

Won't this require a (breaking) runtime change too?

@hfhbd
Copy link
Collaborator Author

hfhbd commented Jun 23, 2023

This isn’t part of the runtime, but part of the compiler which isn’t stable.

@dellisd
Copy link
Collaborator

dellisd commented Jun 23, 2023

Right, but SqlSchema.version is derived from this, isn't it? And also the values used for SqlSchema.migrate().

maxVersion = maxOf(maxVersion, migrationFile.version + 1)

@hfhbd
Copy link
Collaborator Author

hfhbd commented Jun 23, 2023

Ah, you mean behavior, I thought about api only.
Yes, this will change the current behavior, but the current behavior is undefined. If we find a numerical value in the name, we try to parse this value to Int. If this fails, we use 0 as fallback as migration version, which will be overridden in an non deterministic way by another migration version with 0 too.

@AlecKazakova
Copy link
Collaborator

generated code breakages are okay, since you cant have a published library that depends on that API

@hfhbd
Copy link
Collaborator Author

hfhbd commented Jun 26, 2023

Will rebase once a new sql-psi release is out.

@hfhbd hfhbd marked this pull request as ready for review June 26, 2023 17:03
@hfhbd hfhbd enabled auto-merge (squash) June 27, 2023 11:19
@hfhbd hfhbd merged commit ce827d1 into master Jun 27, 2023
5 checks passed
@hfhbd hfhbd deleted the hfhbd/long-version branch June 27, 2023 12:01
@vanniktech
Copy link
Contributor

Should we also change it upstream in sqliter to be a Long? https://github.com/touchlab/SQLiter/blob/a5a532b012cd84d706d143938660094b87b2dfcc/sqliter-driver/src/nativeCommonMain/kotlin/co/touchlab/sqliter/DatabaseConfiguration.kt

On Android you'd be able to use Long but on native it would just crash:

version = if (schema.version > Int.MAX_VALUE) error("Schema version is larger than Int.MAX_VALUE: ${schema.version}.") else schema.version.toInt(),

@hfhbd
Copy link
Collaborator Author

hfhbd commented Jun 29, 2023

What do you mean with Android? The android driver does not support Long too and it also crashes.

@vanniktech
Copy link
Contributor

So we only changed it for non android/ios consumers?

@hfhbd
Copy link
Collaborator Author

hfhbd commented Jun 29, 2023

The change affects all targets, but from my view I would use a long version, eg version based on a timestamp, for backend systems. On mobile, you could easily use the build/release number as migration number, which is rarely bigger than Int.max.

Also, to get a crash, you need to change your versioning after using sqldelight 2.0.0-rc02. The old behaviour is still working.

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

Successfully merging this pull request may close these issues.

generateMainMyDatabaseInterface fails on timestamped naming convention sqm files
4 participants