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

fix(knex): use driver name to identify client #3527

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

jawadst
Copy link
Contributor

@jawadst jawadst commented Aug 28, 2024

In @feathers/knex, when inserting data, use the client.driverName property to identify the clients that can return fields when inserting instead of the client config as a string.

Problem

The current implementation of @feathersjs/knex expects the client to be passed as a string when initializing Knex. Example:

const db: Knex = knex({
      client: "pg",
      connection: "postgres://postgres:postgres@localhost:5432/api",
});

However, a client can be passed as a class instance to Knex instead of a string. Example:

const db: Knex = knex({
    client: ClientPgLite,
    dialect: "postgres",
    connection: {},
});

(The example is for using https://github.com/czeidler/knex-pglite)

When passing the client as an object, @feathersjs/knex throws an error because it cannot identify the client as a returning client (even though, in this example, the client is a derivative of the pg client and is a returning client):

ERROR:  TypeError: Cannot read properties of undefined (reading 'id')
          at KnexService._create (/node_modules/.pnpm/@feathersjs+knex@5.0.29_knex@3.1.0/node_modules/@feathersjs/knex/src/adapter.ts:247:33)
          at processTicksAndRejections (node:internal/process/task_queues:95:5)
          at KnexService.<anonymous> (/node_modules/.pnpm/@feathersjs+schema@5.0.29_typescript@5.5.4/node_modules/@feathersjs/schema/src/hooks/resolve.ts:97:5)
          at KnexService.<anonymous> (/node_modules/.pnpm/@feathersjs+schema@5.0.29_typescript@5.5.4/node_modules/@feathersjs/schema/src/hooks/resolve.ts:150:5)
          at KnexService.logError (/packages/feathers/dist/index.js:45:5)
          at /node_modules/.pnpm/@feathersjs+koa@5.0.29_typescript@5.5.4/node_modules/@feathersjs/koa/src/rest.ts:36:21
          at /node_modules/.pnpm/@feathersjs+koa@5.0.29_typescript@5.5.4/node_modules/@feathersjs/koa/src/handlers.ts:6:5

The error happens at https://github.com/feathersjs/feathers/blob/dove/packages/knex/src/adapter.ts#L242

Solution

This PR fixes the issue by switching to client.driverName for identifying the client.

client.driverName is a string that contains the driver name:

  • When the client is provided as a string in the config, client.driverName will have the same value as client.config (pg, postgresql, etc.)
  • When the client is provided as a class instance in the config, client.driverName will have the name of the driver as a string

Use the `client.driverName` property to identify the clients that can return fields instead of the client config as a string.
@daffl daffl merged commit bb075ec into feathersjs:dove Sep 2, 2024
2 checks passed
@daffl
Copy link
Member

daffl commented Sep 2, 2024

That makes a lot of sense, thank you!

@jawadst jawadst deleted the patch-1 branch September 3, 2024 08:52
@jawadst
Copy link
Contributor Author

jawadst commented Sep 3, 2024

Thanks @daffl!

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.

2 participants