Sqlite schema discovery #34

tyt2y3 · 2021-12-12T12:57:43Z

continue from #32

Add enum type for writer

Fix a few typos I found while digging through the source code

… processing

to allow the ` #[derive(sqlx::FromRow, Debug)]` to be used to derive an sqlx row for handling PostgreSQL enums

Move types into types files under postgres::def::Type Add documentation on types Output the result by calling `discover_enum` on `SchemaDiscovery` Add method to parse a raw SQL create query for enum creation Add missing documentation for the added types for enum creation Test case to discover all the enums in a given database schema Write into SQL and then parse the raw SQL asserting that all discovered enums are the same as their raw SQL queries Solve issues with github actions not detecting types Add feature flag for new enum type and associated functions Switch to using re-exported sqlx types within sea-schema Fix CI errors Support enums with crazy names that include special characters Add support for ordered Vecs Add tests for discovery of enums Remove redundant module postgres_enum from the tests dir Ensure a custom enum type is dropped first from the database so that CI runs successfully Refactoring Ensures the writer derives the enum name from the EnumDef

Refactoring Refactoring Testing enum column... Refactoring Refactoring Refactoring Fix test Refactoring Refactoring Refactoring Cleanup

Update dependencies for compatibility with newer versions of SeaQL ecosystem libraries Add assertion tests Ensure that the autoincrement is called only on a primary key column Ensure that the `sqlite_sequence` table is populated by inserting data. This ensures that a primary key column with autoincrement will be entered into this table Fixes foreign key actions on tests Test both default values and foreign key actions Feature gate sqlite discovery to allow allowing it when it needed Move to `runtime-async-std-native-tls` to allow CI to build Bug fixes for github actions Configure test actions for SQLite in github actions file Switch to `cargo run` Remove unnecessary databases Ensure a database is created whenever the test is run

CI run sqlite discovery & writer Typo Fixup

Move to using a `Sqlitepool` Move tests to using a `Sqlitepool` Restructure to remove unused code Removes duplicate sql statements Returns `TableCreateStatement` on table discovery Returns `IndexCreateStatement` on index discovery Add assertion tests for creating and discovering indexes Add tests for discovery of indexes Restructure tests for table discovery Write discovered schema using new methods Add tests for index discovery Modify test to use new methods

tyt2y3 · 2021-12-12T14:59:12Z

@billy1624 please give one final look on it

billy1624 · 2021-12-13T09:17:30Z

src/sqlite/tabledef.rs

+                new_column.auto_increment();
+            }
+
+            new_column.custom(Alias::new(&column_info.r#type.stringify_type()));


All column types are specified in terms of string

@billy1624 this is because the current sea-query statements don't seem to handle all sqlite types like DOUBLE PRECISION , VARYING CHARACTER etc. Am I wrong about this ?

billy1624 · 2021-12-13T09:30:45Z

tests/live/sqlite/src/main.rs

+fn create_bakery_table() -> TableCreateStatement {
+    Table::create()
+        .table(Alias::new("bakery"))
+        .col(
+            ColumnDef::new(Alias::new("id"))
+                .integer()
+                .not_null()
+                .auto_increment()
+                .primary_key(),
+        )
+        .col(ColumnDef::new(Alias::new("name")).string())
+        .col(ColumnDef::new(Alias::new("profit_margin")).double())
+        .to_owned()
+}


And I'm not sure why it fail to discover the AUTOINCREMENT for the bakery table

https://github.com/SeaQL/sea-schema/runs/4503988152#step:5:527

There must be at least one row inserted

I think we better not depends on this assumption. We could get around it with https://stackoverflow.com/a/18694881/7059723

See 3336198

3336198 LGTM .Thanks for providing a fix that. I wish I would have seen that stackoverflow post earlier. But I guess it depends of one's google search 😂.

Is there anything else that needs fixing?

billy1624 · 2021-12-13T09:32:40Z

tests/live/sqlite/src/main.rs

+    // Creates a new test table
+    let create_table = Table::create()
+        .table(Alias::new("Programming_Langs"))
+        .col(
+            ColumnDef::new(Alias::new("Name"))
+                .custom(Alias::new("INTEGER"))
+                .not_null()
+                .auto_increment()
+                .primary_key(),
+        )


But it can discover this AUTOINCREMENT column

https://github.com/SeaQL/sea-schema/runs/4503988152#step:5:120

My tests ensure a row is first inserted before trying to check for autoincremented columns

billy1624 · 2021-12-13T09:36:03Z

tests/live/sqlite/src/main.rs

+    let insert_into_table = Query::insert()
+        .into_table(Alias::new("Programming_Langs"))
+        .columns(vec![Alias::new("SLOC"), Alias::new("SemVer")])
+        .values(vec![4.into(), "0.1.0".into()])
+        .unwrap()
+        .to_owned();


Wait... it must have at least one row in order discover the AUTOINCREMENT column?

As it depends on the last inserted AUTOINCREMENT value?

sea-schema/src/sqlite/tabledef.rs

Lines 35 to 54 in ccf2ee0

/// Check if the primary key in the table is set to autoincrement as a result of using query

/// `SELECT COUNT(*) from sqlite_sequence where name = 'table_name';

pub async fn pk_is_autoincrement(&mut self, executor: &Executor) -> DiscoveryResult<&mut Self> {

let check_autoincrement = Query::select()

.expr(Expr::cust("COUNT(*)"))

.from(Alias::new("sqlite_sequence"))

.and_where(Expr::col(Alias::new("name")).eq(self.name.as_str()))

.to_owned();

let autoincrement_enabled = executor.fetch_one(check_autoincrement).await;

let autoincrement_result: &SqliteRow = &autoincrement_enabled;

let autoincrement: PrimaryKeyAutoincrement = autoincrement_result.into();

if autoincrement.0 == 1_u8 {

self.auto_increment = true;

}

Ok(self)

}

@charleschege

Yes! There must be at least one row. This issue took me a while to realize because the official SQLite documentation does not mentions this AFAIK. This is an issue with SQLite and I don't think it can be solved on our end. Any suggestions ? @billy1624 @tyt2y3

Any suggestions?

See my comment on the second review above.

Yeah... sadly there is no official way of doing schema discovery

billy1624 · 2021-12-13T11:30:45Z

How to handle composite primary key for SQLite?

https://github.com/SeaQL/sea-schema/runs/4505431579#step:5:637

Diff < left / right > :
<"CREATE TABLE `cakes_bakers` ( `cake_id` integer NOT NULL, `baker_id` integer NOT NULL, CONSTRAINT `cakes_bakers_pkey`PRIMARY KEY (`cake_id`, `baker_id`) )"
>"CREATE TABLE `cakes_bakers` ( `cake_id` integer NOT NULL PRIMARY KEY, `baker_id` integer NOT NULL PRIMARY KEY )"

', tests/live/sqlite/src/main.rs:307:9

charleschege · 2021-12-13T11:41:03Z

How to handle composite primary key for SQLite?

Diff < left / right > :
<"CREATE TABLE `cakes_bakers` ( `cake_id` integer NOT NULL, `baker_id` integer NOT NULL, CONSTRAINT `cakes_bakers_pkey`PRIMARY KEY (`cake_id`, `baker_id`) )"
>"CREATE TABLE `cakes_bakers` ( `cake_id` integer NOT NULL PRIMARY KEY, `baker_id` integer NOT NULL PRIMARY KEY )"

', tests/live/sqlite/src/main.rs:307:9

@billy1624
I am not sure how to detect this from a database

billy1624 · 2021-12-13T11:43:05Z

For any primary key(s) that does not contain AUTOINCREMENT

CREATE TABLE something (
  column1 INTEGER NOT NULL,
  column2 INTEGER NOT NULL,
  value,
  PRIMARY KEY ( column1, ... )
);

For any primary key that does contain AUTOINCREMENT

CREATE TABLE something (
  column1 INTEGER NOT NULL AUTOINCREMENT,
  column2 INTEGER NOT NULL,
  value,
);

charleschege · 2021-12-14T04:13:31Z

@billy1624
Oh my! I had already started working on a fix to find the composite keys, but thanks for the contribution.

billy1624 · 2021-12-14T04:14:30Z

Opppps... Sorry

charleschege · 2021-12-14T04:16:08Z

@billy1624 It's fine. No worries. I guess all the issues are now fixed, right?

billy1624

Thanks!! @charleschege

* Implement Error & Display for SQLite error * MySQL & PostgreSQL write to `TableCreateStatement` * Ignore "sqlite_sequence" table * Discover timestamp column * Map text column to string

charleschege added 13 commits November 12, 2021 13:55

Correct type from Alias::new("ploygon") to Alias::new("polygon")

fcbff8f

Add enum type for writer

Add type

a2b063a

Add support for PostgreSQL types

57e5a41

Fix a few typos I found while digging through the source code

Add tests for checking the correctness of PostgreSQL enum parsing and…

7e36665

… processing

Gate PgEnum and its impl block under the sqlx feature

d2016a1

to allow the ` #[derive(sqlx::FromRow, Debug)]` to be used to derive an sqlx row for handling PostgreSQL enums

Add support for fetching enums

d70fe56

Merge branch 'SeaQL:master' into master

c41f23d

Refactoring

6899a37

Refactoring Refactoring Testing enum column... Refactoring Refactoring Refactoring Fix test Refactoring Refactoring Refactoring Cleanup

Test sqlite discovery & writer

a37a8b5

CI run sqlite discovery & writer Typo Fixup

Resolve conflict

922679b

tyt2y3 force-pushed the sqlite branch from 0b74679 to 922679b Compare December 12, 2021 13:01

tyt2y3 added 2 commits December 12, 2021 21:03

Merge branch 'master' into sqlite

df315c8

Fix build error

e9832eb

tyt2y3 force-pushed the sqlite branch from 88e55fc to e9832eb Compare December 12, 2021 13:34

billy1624 reviewed Dec 13, 2021

View reviewed changes

billy1624 approved these changes Dec 15, 2021

View reviewed changes

billy1624 mentioned this pull request Dec 17, 2021

Support SQLite SeaORM Codegen #36

Merged

billy1624 added 2 commits December 19, 2021 03:31

Refactoring & cleanup

249fe91

Support SQLite SeaORM Codegen (#36)

04456bb

* Implement Error & Display for SQLite error * MySQL & PostgreSQL write to `TableCreateStatement` * Ignore "sqlite_sequence" table * Discover timestamp column * Map text column to string

tyt2y3 force-pushed the sqlite branch from 10230e0 to 04456bb Compare December 19, 2021 08:50

tyt2y3 merged commit 1036e11 into master Dec 19, 2021

tyt2y3 deleted the sqlite branch December 19, 2021 09:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sqlite schema discovery #34

Sqlite schema discovery #34

tyt2y3 commented Dec 12, 2021

tyt2y3 commented Dec 12, 2021

billy1624 Dec 13, 2021

charleschege Dec 13, 2021

billy1624 Dec 13, 2021

charleschege Dec 13, 2021

billy1624 Dec 13, 2021

billy1624 Dec 13, 2021

charleschege Dec 13, 2021 •

edited

Loading

charleschege Dec 13, 2021

billy1624 Dec 13, 2021

charleschege Dec 13, 2021

billy1624 Dec 13, 2021

billy1624 Dec 13, 2021

charleschege Dec 13, 2021

billy1624 Dec 13, 2021 •

edited

Loading

billy1624 Dec 13, 2021

billy1624 commented Dec 13, 2021 •

edited

Loading

charleschege commented Dec 13, 2021

billy1624 commented Dec 13, 2021

charleschege commented Dec 14, 2021

billy1624 commented Dec 14, 2021

charleschege commented Dec 14, 2021 •

edited

Loading

billy1624 left a comment

	/// Check if the primary key in the table is set to autoincrement as a result of using query
	/// `SELECT COUNT(*) from sqlite_sequence where name = 'table_name';
	pub async fn pk_is_autoincrement(&mut self, executor: &Executor) -> DiscoveryResult<&mut Self> {
	let check_autoincrement = Query::select()
	.expr(Expr::cust("COUNT(*)"))
	.from(Alias::new("sqlite_sequence"))
	.and_where(Expr::col(Alias::new("name")).eq(self.name.as_str()))
	.to_owned();

	let autoincrement_enabled = executor.fetch_one(check_autoincrement).await;

	let autoincrement_result: &SqliteRow = &autoincrement_enabled;
	let autoincrement: PrimaryKeyAutoincrement = autoincrement_result.into();

	if autoincrement.0 == 1_u8 {
	self.auto_increment = true;
	}

	Ok(self)
	}

Sqlite schema discovery #34

Sqlite schema discovery #34

Conversation

tyt2y3 commented Dec 12, 2021

tyt2y3 commented Dec 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleschege Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billy1624 Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billy1624 commented Dec 13, 2021 • edited Loading

charleschege commented Dec 13, 2021

billy1624 commented Dec 13, 2021

charleschege commented Dec 14, 2021

billy1624 commented Dec 14, 2021

charleschege commented Dec 14, 2021 • edited Loading

billy1624 left a comment

Choose a reason for hiding this comment

charleschege Dec 13, 2021 •

edited

Loading

billy1624 Dec 13, 2021 •

edited

Loading

billy1624 commented Dec 13, 2021 •

edited

Loading

charleschege commented Dec 14, 2021 •

edited

Loading