search

SeaQL / sea-schema

🌿 SQL schema definition and discovery
https://docs.rs/sea-schema
Apache License 2.0
192 stars 40 forks source link

Fix entity generation in Sqlite #99

Closed addap closed 10 months ago

addap commented 1 year ago

PR Info

New Features

Bug Fixes

Breaking Changes

Changes

addap commented 1 year ago

Ah sorry I was testing something and it affected the pull request. The master branch should not be changed now.

addap commented 1 year ago

Hi @billy1624, no I think the situation with indexes was just more messy than I originally thought. Luckily the tests caught that. So, in Sqlite you can either create a (possibly UNIQUE) index using the CREATE INDEX statement, or you can add CONSTRAINT clauses to a CREATE TABLE statement (which will result in the creation of an index with two exceptions [0]). These two cases can be differentiated with column 4 of the output of the index_list pragma [1]. Additionally, Sqlite apparently does not use the name used in a CONSTRAINT clause for the index, but autogenerates a new one like "sqlite_autoindex_Programming_Langs_1".

I tried to solve it as follows:

  1. A TableDef now has an indexes and a constraints vector. Both hold IndexInfo.
  2. In the table discovery, we only search for indexes of type "u" and add them to the constraints vector. Indexes of type "pk" are already special cased in existing code and are handled correctly afaik.
  3. In the index discovery, we only search for indexes of type "c" and add them to the indexes vector.
  4. Due to the name autogeneration, we do not save the names in the discovered constraints, as they are not the same as in the original TableCreateStatement.

For point 3. I was not sure. It could also make sense to get all indexes, even if they were created due to CONSTRAINT clauses. I added another case for non-integer primary keys to the test that was failing before and it passes now.

[0] https://www.sqlite.org/lang_createtable.html#uniqueconst [1] https://www.sqlite.org/pragma.html#pragma_index_list

tyt2y3 commented 1 year ago

Sorry for the delay. However I don't think I am an expert in SQLite internals to be able to determine whether this is an ideal solution.

Would appreciate some external input here...

tyt2y3 commented 10 months ago

Sorry for the delay. I finally have a clear mind to read through this, I think this should work.