Closed cpaczek closed 1 week ago
Attention: Patch coverage is 0%
with 11 lines
in your changes are missing coverage. Please review.
Project coverage is 60.67%. Comparing base (
b9e92de
) to head (fcfc414
). Report is 5357 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
You can use alterType
parameter in newer version of knex instead: https://github.com/knex/documentation/commit/20adb5c0f4b3590f38a2bedfbda144f0c22356bb
You can use
alterType
parameter in newer version of knex instead: https://github.com/knex/documentation/commit/20adb5c0f4b3590f38a2bedfbda144f0c22356bb
Ya was waiting for newest npm publish, which happened 7 mins ago
Still will have to migrate to knex 1.0 for the other dbs
(This PR is still full of logs but you can use this branch and use yarn link to test it on a Strapi instance)
You can use
alterType
parameter in newer version of knex instead: knex/documentation@20adb5c
I just migrated to knex 1.0.2 and implemented the alterNullable and that fixed the drop null on id column. (https://github.com/Cpaczek/strapi/blob/8a3b09759c76c80a996f99b76b3f7cdb0ff0debe/packages/core/database/lib/schema/builder.js#L307)
However I am running into a new issue. On knex .95.x our issue outside of the drop nullable was the int -> int on column types now its the following.
I also "implemented" alterType by setting it to false on all alter()
let alterNullable = false;
if (updatedColumn.name === 'id') {
alterNullable = true;
continue;
}
createColumn(tableBuilder, object).alter({ alterNullable, alterType: true });
not sure if I have to selectively change if alterType is true of false depending on the change. this is just a very rough test. Still doing research.
[2022-02-02 19:50:51.706] error: alter table "admin_permissions_role_links" drop constraint "primary" - constraint "primary" of relation "admin_permissions_role_links" does not exist
error: alter table "admin_permissions_role_links" drop constraint "primary" - constraint "primary" of relation "admin_permissions_role_links" does not exist
[2022-02-02 19:55:11.954] error: alter table "admin_permissions" alter column "created_by_id" type integer using ("created_by_id"::integer) - unimplemented: ALTER COLUMN TYPE for a column that has a constraint is currently not supported
error: alter table "admin_permissions" alter column "created_by_id" type integer using ("created_by_id"::integer) - unimplemented: ALTER COLUMN TYPE for a column that has a constraint is currently not supported
With it set to true looks like its trying to use a cdb experimental feature: Experimental alter column in use, see issue: https://github.com/cockroachdb/cockroach/issues/49329
The errors above happen when I am trying to add a new collection. You can see that it wants to add the table just fine however pretty much every table has updated columns which I am not sure why yet.
Why would adding a new collection modify the so many different other table's columns. Pretty sure there is something really wrong with the diff function within Strapi (https://github.com/Cpaczek/strapi/blob/8a3b09759c76c80a996f99b76b3f7cdb0ff0debe/packages/core/database/lib/schema/diff.js#L325)
Makes no sense why the schema of these tables need to be updated.
@derrickmehaffy Any idea why Strapi wants to change so many table's schemas?
Breaking changes Dropped support for Node 10; <- This won't effect anything Replaced unsupported sqlite3 driver with @vscode/sqlite3; <- Will have to fix sqlite driver but that is a future problem Changed data structure from RETURNING operation to be consistent with SELECT; <- Not sure if this effect the Strapi codebase Changed Migrator to return list of migrations as objects consistently. <- Not sure about this either
I have been following along with this as well.
The behaviour seems like whenever another table is added which has a foreign key, the table owning the foreign key is updated for no reason.
For example, if I have table A which references a column on table B. When table A is created, an update is issued to table B (which doesn't change anything at all).
@derrickmehaffy Any idea why Strapi wants to change so many table's schemas?
I don't have any ideas, @alexandrebodin would need to weigh in here as he wrote basically all of this himself (Alex is out of the office this week)
The behaviour seems like whenever another table is added which has a foreign key, the table owning the foreign key is updated for no reason.
This would be my guess though as we added FQ now to basically everything (for good reason) but I don't know why doing so would mean doing updates if nothing changed.
I would assume our diff code could be improved but I haven't dug that deep into it myself as I've been slammed with multiple other topics lately.
Are there any existing plans to migrate to knex v1? I am planning on doing this in this PR but I was wondering if that was already in the works.
Are there any existing plans to migrate to knex v1? I am planning on doing this in this PR but I was wondering if that was already in the works.
We didn't have any specific plans but I don't think we have any specific arguments not to either.
More so wasn't a concern because we didn't need anything in the update yet, but if adding cockroach needs it, I don't see why we can't.
So long as we don't introduce any breaking changes.
Are there any existing plans to migrate to knex v1? I am planning on doing this in this PR but I was wondering if that was already in the works.
We didn't have any specific plans but I don't think we have any specific arguments not to either.
More so wasn't a concern because we didn't need anything in the update yet, but if adding cockroach needs it, I don't see why we can't.
So long as we don't introduce any breaking changes.
Afaik the only thing that would have to be changed from the user's perspective is the SQLite driver and that is rarely using in prod anyway, and that is probably for the better as its not longer supported.
There are a lot of cool new features in the 1.0 release as well https://github.com/knex/knex/releases
Are there any existing plans to migrate to knex v1? I am planning on doing this in this PR but I was wondering if that was already in the works.
We didn't have any specific plans but I don't think we have any specific arguments not to either. More so wasn't a concern because we didn't need anything in the update yet, but if adding cockroach needs it, I don't see why we can't. So long as we don't introduce any breaking changes.
Afaik the only thing that would have to be changed from the user's perspective is the SQLite driver and that is rarely using in prod anyway, and that is probably for the better as its not longer supported.
There are a lot of cool new features in the 1.0 release as well https://github.com/knex/knex/releases
Did they finally drop mapbox's SQLite package
Are there any existing plans to migrate to knex v1? I am planning on doing this in this PR but I was wondering if that was already in the works.
We didn't have any specific plans but I don't think we have any specific arguments not to either. More so wasn't a concern because we didn't need anything in the update yet, but if adding cockroach needs it, I don't see why we can't. So long as we don't introduce any breaking changes.
Afaik the only thing that would have to be changed from the user's perspective is the SQLite driver and that is rarely using in prod anyway, and that is probably for the better as its not longer supported.
There are a lot of cool new features in the 1.0 release as well https://github.com/knex/knex/releases
Did they finally drop mapbox's SQLite package
Yes, I think they support 2 driver's now, vscode's and something else
Edit:
So we will need to change the default SQLite driver we install when someone chooses quickstart.
VSCode sqlite one is a drop-in replacement for mapbox, though.
VSCode sqlite one is a drop-in replacement for mapbox, though.
We still have a method on project generation that adds the specific database package to the main package.json, usually sqlite3, MySQL, or pg.
I'll find the generator logic and point it out after my lunch.
VSCode sqlite one is a drop-in replacement for mapbox, though.
We still have a method on project generation that adds the specific database package to the main package.json, usually sqlite3, MySQL, or pg.
I'll find the generator logic and point it out after my lunch.
Here we go, this is where we set the db-client + version to set in the new project's package.json: https://github.com/strapi/strapi/blob/81934f515c211c94c623bf2064238e3829d7eba8/packages/generators/app/lib/utils/db-client-dependencies.js#L6
I've got a draft PR open to work on upgrading knex: https://github.com/strapi/strapi/pull/12557
I'm excited about this 😊
This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:
https://forum.strapi.io/t/mongodb-compatibility-delayed-on-v4/4549/135
Spent tonight catching everything up with master after the knex migration (Had a PITA time building Strapi because of the vscode sqlite and its depending on VS), still having the same issues however that was expected as it seems to be a logical issue with how Strapi is updating columns.
At this point I know this issue https://github.com/cockroachdb/cockroach/issues/49329 is still an issue, either we wait for this to be fixed on cockroach's end or I look into what is going on with Strapi.
Currently with this PR Cockroach will work with the implemented dialect (it just uses the pg driver but needs its own dialect because of knex v1) However the issue is to do something with the migrations. I still think it has something to do with the diff function not working properly on default columns such as ID and "created_by_id"
As of right now here is the error after creating a collection:
alter table "admin_permissions" alter column "created_by_id" type integer using ("created_by_id"::integer) - unimplemented: ALTER COLUMN TYPE for a column that has a constraint is currently not supported
It seems like knex knows that the current implementation is experiential and hence enables the proper flag with cockroach but cockroach still throws and error during this query.
Fairly certain once the above issue gets resolved on cockroaches end it will fix a lot of these bugs that I am having but fixing the diff function within Strapi may be a better approach either way.
Setting alter Nullable to false fixes the above issue, YAY, however now we are on the next issue:
alter table "admin_permissions_role_links" drop constraint "primary" - constraint "primary" of relation "admin_permissions_role_links" does not exist
I think what the issue is that builder.js is using a deprecated function of Knex which sets the constraints of tables
I've also narrowed it down to the migrations as if you already have collection configs in your strapi instance everything works it the adding new collections with a server restart that causes errors.
The issue is actually after an admin account is created then doing a migration, if there is no admin account there are no issues.
What really weird is that the "primary" constraint gets dropped at some point during initial db setup.
Cockroach says that the "primary" constraint exists however when actually checking it doesn't
Honestly this is just really weird behavior and I have no clue where the primary constraint went.
@rafiss Wonder if this is related to primary key being added post-factum. Does CRDB require primary keys to be created together with column itself?
@kibertoad Do you think this is most likely an issue with Cockroach or within the Knex Dialect? My initial thought was that is was an cockroach issue as it seems like you can do it this way.
I'm not sure whether an issue or a design choice, would be great to get an opinion from CRDB team
This is a VERY long trace of what exactly is happening and is a good overview of how Strapi handles migrations Please scroll down to the bottom for a TLDR
Okay I think I know what the core issue may be thanks to this issue (https://github.com/cockroachdb/cockroach/issues/80035)
First thing to note: CRDB does not allow you to drop the primary key constraint, (Kinda) You are allowed to drop the constraint assuming you add it back within the same statement.
See this: https://www.cockroachlabs.com/docs/stable/drop-constraint.html
PRIMARY KEY constraints can be dropped with DROP CONSTRAINT if an ADD CONSTRAINT statement follows the DROP CONSTRAINT statement in the same transaction.
Currently here is what is happening:
It all starts here:
This is where we first alter the
admin_permissions_role_links
table which is the one that is currently mad about droping PK ConstraintsThis then goes and drops all of the existing indexes columns etc. One thing to note is in the logs you see this:
Dropping index primary Dropping updated foreign key admin_permissions_role_links_fk Dropping updated foreign key admin_permissions_role_links_inv_fk
We can clearly see that the primary index is being dropped
We can then see that we are calling the dropPrimary on
And this is droping the "primary" constraint on the table as shown by the knex docs
This shouldn't be a problem right? becuse its being added back in later? Wrong, due to a reason I will get to in just a second the primary constraint is being dropped on a CRDB migration not updated.
We can disable dropping primary key constraints and get a bit further but the root of the problem is somewhere else
If we go back to the original alterTable() call you can see that we diffed 2 schemas
This is
Synchronizing database schema
which calls this function (gets remapped to diffSchemas)This takes the
srcSchema
which is what is currently present on CRDB and thedestSchema
which is the schema that strapi wants to be.This will then call a new function called diffTables:
This will then go on to diff each column index and fk
After digging into this I found what I think the issue is. CRDB has a "hidden" column called rowid
and what is happening is that the srcTable
(However that is actually generated) is INCLUDING the rowid
column however when its comparing to the destSchema
that rowid
column doesn't exist becuase whatever code is generating doesn't include the rowid
column (kinda ill explain more in a second)
And when we check which columns are different we can see that
Removed Index [ { columns: [ 'rowid' ], name: 'primary', type: 'primary' } ]
The diff source is from the rowid
column
We can fix/bypass the primary key drop constraint issue by not adding it to the removed index array in the diff function
We also need to skip dropping the rowid column to prevent the next error
We then get a brand new error YAY :(
error: insert into "strapi_core_store_settings" ("environment", "key", "tag", "type", "value") values ($1, $2, $3, $4, $5) returning "id" - missing "id" primary key column
I have not spent too much time on this error quite yet and maybe someone else can give some insight on what this is exactly trying to do. One thing to note is that this error is happening AFTER the migration to the new schema as this error is happening outside of the main updateSchema
function
This new error is in the bootstrap
lifecycle function
Within this function the error is being thrown in the plugins section, I am starting to be out of my depth at this point
I think what the issue is is that the lifecycle function is looking for a column that is titled id
when with CRDB, from what I know, Strapi should be using rowid
I think? This is where I am not sure.
Whats weird is that the id
column should be there? Looking at the CRDB web panel it sets an id column.
And when you query it there is an id
column, not sure what this is on about
Another Idea that I had was that because its a "Family" in order to insert into it you nee to make sure you are including all of the values and this is not adding the id
insert into "strapi_core_store_settings" ("environment", "key", "tag", "type", "value") values ($1, $2, $3, $4, $5) returning "id" - missing "id" primary key column
This seems much more likely to be the problem
CRDB has a column called rowid
which is "hidden" when strapi is doing its migration it compares the srcSchema
with destSchema
srcSchema: Current schema of CRDB destSchema: Schema of what Strapi wants to be
In the srcSchema the rowid
exists however in destSchema
it does not therefor it trys to do some illlegal stuff with dropping columns and constraints.
If you go into the diff function that Strapi has an add a bypass statement for rowid and primary constraints you can bypass these error (This is probably not the best solution for these errors but it gets a bit further so its temp)
You then run into an issue where in Strapi's lifecycle functions it is trying to insert data into strapi_core_store_settings however because this table has their columns as a FAMILY its throwing an error becuase the insert does not include an id
Not really sure how to fix this as I don't yet have a deep understanding of how FAMILYs work in CRDB
My assumption is that Strapi is expecting the id
field to auto increment however CRDB is expecting it to be explicitly declared.
@derrickmehaffy @alexandrebodin any ideas on how we would go about solving this? (either the diff issue or the insert family error)
@kibertoad Was looking into our newest issue and found when the strapi_core_store_settings
table is being created we have this data for the 'id' column
If we have alterType
false we run into the issue where the auto increment for the id column is removed which makes sense because strapi never really "alters" tables but only ever drops columns and such and re creates them in the same transaciton. hence if we have alterType
false it will break. (at least I think that is what the reasoning is)
createColumn(tableBuilder, object).alter({alterNullable: false, alterType: false});
createColumn returns the column that was made
if we then allow alterType
i.e
createColumn(tableBuilder, object).alter({alterNullable: false, alterType: true});
we now get a syntax error
alter table "strapi_core_store_settings" alter column "id" type serial primary key using ("id"::serial primary key) - at or near "primary": syntax error
I think Knex is using the SERIAL keyword because we are using the pg driver however for some reason the implementation causes a syntax error. https://www.cockroachlabs.com/docs/stable/serial.html
SERIAL is provided only for compatibility with PostgreSQL. New applications should use real data types and a suitable DEFAULT expression.
Do you think this is an issue with Knex?
What's odd is that when the column is first being created it works just fine.
Here is the query that fails: alter table "strapi_core_store_settings" alter column "id" type serial primary key using ("id"::serial primary key)
Serial IIRC is an alias in PG that sets the id as unsigned, auto-incrementing, int. :thinking: we could probably change it to something similiar to what we use in MySQL/MariaDB.
Serial IIRC is an alias in PG that sets the id as unsigned, auto-incrementing, int. 🤔 we could probably change it to something similiar to what we use in MySQL/MariaDB.
Gotcha, figured as much because when the column is initially created it looks fine. What I'm trying to find out is why there is a error here and what is causing it, either the first or second primary and if its an issue with Strapi code or an issue with Knex.
Seems like its the first Primary:
However I am not even sure why Strapi is wanting to alter this column as nothing is changing about it
Here are the new values that it wants to be
{
name: 'id',
type: 'increments',
args: [ { primary: true, primaryKey: true } ],
defaultTo: null,
notNullable: true,
unsigned: false
}
Seems like its the first Primary:
However I am not even sure why Strapi is wanting to alter this column as nothing is changing about it
Here are the new values that it wants to be
{ name: 'id', type: 'increments', args: [ { primary: true, primaryKey: true } ], defaultTo: null, notNullable: true, unsigned: false }
ooooooo I think that's a legacy thing from before we upgraded to knex v1
ooooooo I think that's a legacy thing from before we upgraded to knex v1
You're referring to the fact that Strapi is trying to alter a column but alter nothing about it right? I believe the syntax error is still an error in Knex. I made a test repo where I replicated the syntax error with very minimal code. https://github.com/Cpaczek/knex-test/tree/minimize-test
ooooooo I think that's a legacy thing from before we upgraded to knex v1
You're referring to the fact that Strapi is trying to alter a column but alter nothing about it right? I believe the syntax error is still an error in Knex. I made a test repo where I replicated the syntax error with very minimal code. https://github.com/Cpaczek/knex-test/tree/minimize-test
There was some odd issue we had when we upgraded to knex v1 where we needed to put primaryKey: true in addition to primary: true in order for it to set the id column as a primary key but I don't remember why we couldn't remove primary: true :thinking:
ooooooo I think that's a legacy thing from before we upgraded to knex v1
You're referring to the fact that Strapi is trying to alter a column but alter nothing about it right? I believe the syntax error is still an error in Knex. I made a test repo where I replicated the syntax error with very minimal code. https://github.com/Cpaczek/knex-test/tree/minimize-test
There was some odd issue we had when we upgraded to knex v1 where we needed to put primaryKey: true in addition to primary: true in order for it to set the id column as a primary key but I don't remember why we couldn't remove primary: true thinking
I don't think that is the issue, I was able to replicate the syntax error without setting either of those values. https://github.com/Cpaczek/knex-test/blob/minimize-test/index.js
This causes the same syntax error:
table.increments('id').alter({ alterNullable: false, alterType: true });
It may not be needed to have both primary and primaryKey but I don't think thats the issue here.
I'll take a look at this from knex side, wonder if we need some additional flag to prevent noop in this case.
@kibertoad Any update on this?
Have you looked into using YugabyteDB with Strapi? My take is Knex is attempting to execute Postgres features that haven't been implemented (i.e. noop) in CRDB. Prisma has released a preview of CRDB support and described some of these discrepancies in their docs. When CRDB adds Postgres functionality they have to rewrite features in Golang, whereas Yugabute is partially built with Postgres' source. This allows them to maintain compatibility without having to reinvent the wheel. I like CRDB and distributed Postgres in general, but this bottleneck will make support on current and future Postgres clients very cumbersome. I'm unsure how Yugabyte handles migrations on Strapi so there's definitely testing to be done on both platforms.
Have you looked into using YugabyteDB with Strapi? My take is Knex is attempting to execute Postgres features that haven't been implemented (i.e. noop) in CRDB. Prisma has released a preview of CRDB support and described some of these discrepancies in their docs. When CRDB adds Postgres functionality they have to rewrite features in Golang, whereas Yugabute is partially built with Postgres' source. This allows them to maintain compatibility without having to reinvent the wheel. I like CRDB and distributed Postgres in general, but this bottleneck will make support on current and future Postgres clients very cumbersome. I'm unsure how Yugabyte handles migrations on Strapi so there's definitely testing to be done on both platforms.
Wow this looks amazing, Ill spin up a local cluster and see if it works.
@derrickmehaffy So I tried out YugabyteDB thank to @edgarnet and it seems like like a perfect replacement. I was able to spin up a single node cluster locally using docker and creating, updating and adding rows seems to work perfectly however there is an issue with deleting an single type which causes an error only once so it may be a fluke.
Select count(*) as "count" from "up_roles" as "t0" limit $1 - Query error: The catalog snapshot used for this transaction has been invalidated: MISMATCHED_SCHEMA
I have tested basically everything (relations, components, CRUD) is there anything else I should try. Asumming nothing else goes wrong I will probably end up abandoning this PR. Up to you if you want to officially support Yugabyte from my testing it seems to work really well.
Edit: seems like the bug is a known issue in the latest version https://github.com/yugabyte/yugabyte-db/issues/12621
@Cpaczek sorry for delay, I'll take a look today
Any updates on this? It would be awesome to use CockroachDB with Strapi
Any updates on this? It would be awesome to use CockroachDB with Strapi
Not really, Strapi uses some features of postgres that are not supported by cockroach meaning that its a lot of work to get it working. unless this gains focus by the core team there is a low chance of it ever getting implemented. (hence why we're switching away from Strapi)
@Cpaczek Can you try with latest crdb and knex, though? https://github.com/knex/knex/pull/5233 should address at least one of the issues that you had, I think.
@Cpaczek Can you try with latest crdb and knex, though? knex/knex#5233 should address at least one of the issues that you had, I think.
Just upgraded to knex 2.2.0 and crdb 22.1.4
Still get this error
alter table "strapi_core_store_settings" alter column "id" type serial primary key using ("id"::serial primary key) - at or near "primary": syntax error
@Cpaczek What code is triggering that error? I want to move the needle on this one and will investigate tonight if I can get repro.
@Cpaczek What code is triggering that error? I want to move the needle on this one and will investigate tonight if I can get repro.
This altertable and if you scroll down to line 302 that's where its trying to alter the id column
This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:
https://forum.strapi.io/t/has-anyone-used-planetscale-com/9875/11
@alexandrebodin the original author has informed me via Discord that he doesn't believe he can finish this as is. Do you want me to close it or would you like to take a look?
No hurt feelings either way, as a first a attempt from the community to add an entirely new DB it was an awesome job but maybe a bit too much without more guidance from us that we couldn't give at the moment.
I'm still working on this from knex side
I'm still working on this from knex side
Thanks for letting us know. I'll keep this PR open for now.
Since we haven't had any work on this for a few months I'll go ahead and close it but if someone wants to take over let us know.
At the request of the PR author, reopening as they believe they can get it working
To test this PR here are some commands to get you up and running with a CRDB docker container.
Get an SQL shell
Strapi DB Config
fixes https://github.com/strapi/strapi/issues/11738