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

Cockroach DB for v5 #20373

Closed
wants to merge 5 commits into from
Closed

Cockroach DB for v5 #20373

wants to merge 5 commits into from

Conversation

cpaczek
Copy link
Collaborator

@cpaczek cpaczek commented May 25, 2024

CRDB is finally working with Strapi, passing all tests, documentation written, and ready for review.

to test use the following docker-compose file and connection config.

  connection: {
    client: 'cockroachdb',
    connection: {
      host: env('DATABASE_HOST', 'localhost'),
      port: env.int('DATABASE_PORT', 26257),
      database: env('DATABASE_NAME', 'strapi'),
      user: env('DATABASE_USERNAME', 'root'),
      password: env('DATABASE_PASSWORD', ''),
      ssl: env.bool('DATABASE_SSL', false) && {
        key: env('DATABASE_SSL_KEY', undefined),
        cert: env('DATABASE_SSL_CERT', undefined),
        ca: env('DATABASE_SSL_CA', undefined),
        capath: env('DATABASE_SSL_CAPATH', undefined),
        cipher: env('DATABASE_SSL_CIPHER', undefined),
        rejectUnauthorized: env.bool(
          'DATABASE_SSL_REJECT_UNAUTHORIZED',
          true
        ),
      },
      schema: env('DATABASE_SCHEMA', 'public'),
    },
    useNullAsDefault: true,
  },

Docker Compose

version: '3.7'

services:
  cockroachdb:
    image: cockroachdb/cockroach:latest
    container_name: cockroachdb
    command: start-single-node --insecure
    ports:
      - "26257:26257"
      - "8080:8080"
    environment:
      - COCKROACH_DATABASE=strapi
    volumes:
      - cockroachdb_data:/cockroach/cockroach-data

volumes:
  cockroachdb_data:

Copy link

vercel bot commented May 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
contributor-docs ❌ Failed (Inspect) May 26, 2024 3:50am

@ethan-gallant
Copy link

Excited to see this moving forward 😄

One thing to note (and maybe you've already looked in to it), but the client may need to retry for highly concurrent tables and entries. CRDB can return an "error" that's basically just a request that the client retries its transaction. Knex may handle that for you already though.

@innerdvations innerdvations added community source: core:database Source is core/database package pr: feature This PR adds a new feature labels Jun 4, 2024
@alexandrebodin
Copy link
Member

Hi @cpaczek thank you for all the hard work on this.

Currently we cannot merge this into the main repo as we do not have the capacity to maintain this extra dialect. We are looking at ways to open an API/config so someone could install such a dialect as a package and use it. Hopefully we can propose sth for v5 🙌

@cpaczek
Copy link
Collaborator Author

cpaczek commented Jun 11, 2024

Hi @cpaczek thank you for all the hard work on this.

Currently we cannot merge this into the main repo as we do not have the capacity to maintain this extra dialect. We are looking at ways to open an API/config so someone could install such a dialect as a package and use it. Hopefully we can propose sth for v5 🙌

Understandable, for reference for what an API would need to support these were the main things that needed to be changed/added in order to get cockroach to work. the rest is just copy and pasted from postgres config.

Changing sql_sequence and transaction isolation level to more closely mimic postgres (sql_seqence might not need to be changed since v5 now uses uid but haven't tested this)
https://github.com/strapi/strapi/pull/20373/files#diff-83df53a3c637cd43fe031952809b38b52a9860fd0efd9980eceec06dc3fef219R39-R41

Use the string_to_array function to get over the limitations of 2d arrays.
https://github.com/strapi/strapi/pull/20373/files#diff-90e7c1efa402d9798e5fc0967a25a378c3a55839a965ba30367f66157f0c63c2R91

Pass CockroachDB into knex
https://github.com/strapi/strapi/pull/20373/files#diff-13be2bd8641b8ef1449f2c1ad064fb579b580e33c5db6886e5d7e5be47d3179fR8

The main reason it took this long to make cockroach compatible with Strapi is mainly because of the transaction isolation level but cockroach now has official support for READ COMMITTED isolation level which makes it mimic default postgres behavior.

I'm not sure how helpful building out an API for this would be as adding any dialect that isn't support by knex would be impossible and relying on 3rd party devs would reduce reliability since the database is such a critical system unlike a plugin.

@cpaczek
Copy link
Collaborator Author

cpaczek commented Jun 11, 2024

Was also talking with @derrickmehaffy and if we wanted to have dialects be external packages and it would be relatively simple

Have this respect "custom" dialects from the config

export const createConnection = (userConfig: Knex.Config, strapiConfig?: Partial<Knex.Config>) => {
if (!isClientValid(userConfig)) {
throw new Error(`Unsupported database client ${userConfig.client}`);
}
const knexConfig: Knex.Config = {
...userConfig,
client: (clientMap as any)[userConfig.client],
};

Then have this file load the external package from the db config

const getDialectClass = (client: string): typeof Dialect => {
switch (client) {
case 'postgres':
return PostgresClass;
case 'cockroachdb':
return CockroachDBClass;
case 'mysql':
return MysqlClass;
case 'sqlite':
return SqliteClass;
default:
throw new Error(`Unknown dialect ${client}`);
}
};

so the config would look something like this

  connection: {
    client: 'cockroachdb',
    resolve: '@cpaczek/cockroach-strapi'
    connection: {
...
      },
      schema: env('DATABASE_SCHEMA', 'public'),
    },
    useNullAsDefault: true,
  },

and cockroach-strapi would just include the dialect index.ts and schema-inspector which is where all the changes are

Also Move this logic to dialect instead of being scattered around the codebase
https://github.com/strapi/strapi/pull/20373/files#diff-80832274c37f5c2cea8c3ef2cd52d267dc76283b4e99326e2ad965ccf084db02R450
https://github.com/strapi/strapi/pull/20373/files#diff-6639283f3ab6afc685de3662aff214e9856d856f5ba34d6b339c20fe108a7ff4R156

@alexandrebodin
Copy link
Member

Yup you summarized well what I had in mind if we go this route 👍

@cpaczek
Copy link
Collaborator Author

cpaczek commented Jun 13, 2024

if yall do decide to go this route I could take a crack at it in a new PR. Not sure if you'd want to do an RFC first tho. Could easily do it without breaking changes.

@alexandrebodin
Copy link
Member

If you fill like it that sounds great to do a draft. do you want to propose a mini spec here 1st ? so we can validate before you go ahead :D

@derrickmehaffy
Copy link
Member

Since cockroach is no longer open source, probably best not to integrate with them. Sorry @cpaczek

https://news.itsfoss.com/cockcroachdb-no-open-source/

https://www.cockroachlabs.com/blog/enterprise-license-announcement/?ref=news.itsfoss.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community pr: feature This PR adds a new feature source: core:database Source is core/database package
Projects
Development

Successfully merging this pull request may close these issues.

5 participants