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 BigInt for integer primary key in sqlite for diesel-cli #3940

Merged
merged 1 commit into from
Feb 27, 2024
Merged

Use BigInt for integer primary key in sqlite for diesel-cli #3940

merged 1 commit into from
Feb 27, 2024

Conversation

stormshield-kg
Copy link
Contributor

@stormshield-kg stormshield-kg commented Feb 19, 2024

Fixes #852.

The sqlite-integer-primary-key-is-bigint config parameter is used to modify the generation.
With this option, the PRAGMA table_list query is used to get the WITHOUT ROWID attribute for a table (available from SQLite 3.37).

@weiznich weiznich requested a review from a team February 20, 2024 07:45
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this feature 👍

I generally like the idea to address the underlying issue, but I strongly suggest not to use a feature flag for this as this makes it:

  • harder to discover this feature
  • harder to document this feature
  • harder to test this feature (another entry in the feature matrix…)

Instead I would suggest making this a config option in the diesel.toml file and a CLI flag. This allows to control that feature at runtime.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
diesel_cli/src/infer_schema_internals/sqlite.rs Outdated Show resolved Hide resolved
diesel_cli/src/migrations/diff_schema.rs Show resolved Hide resolved
diesel_cli/tests/migration_generate.rs Outdated Show resolved Hide resolved
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. This looks now much closer to something that is ready to be merged. I've left a few minor comments. The most critical thing is the loading of the primary keys while checking if the column is an alias for rowid. That needs to be fixed, the other comments are more suggestions.

It would also be great to have an entry in the Changelog.md file for this feature.

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now 👍. Thanks for pushing this forward.

@weiznich weiznich added this pull request to the merge queue Feb 27, 2024
Merged via the queue into diesel-rs:master with commit ec91327 Feb 27, 2024
48 checks passed
@stormshield-kg stormshield-kg deleted the integer_primary_key_i64_for_sqlite_3_37 branch February 28, 2024 08:48
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.

INTEGER PRIMARY KEY in SQLite
2 participants