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

useReturning is not correctly set for the SQLite DB dialect #14193

Closed
WalkingPizza opened this issue Aug 24, 2022 · 7 comments · Fixed by #14267
Closed

useReturning is not correctly set for the SQLite DB dialect #14193

WalkingPizza opened this issue Aug 24, 2022 · 7 comments · Fixed by #14267
Labels
issue: bug Issue reporting a bug severity: medium If it breaks the basic use of the product but can be worked around source: core:database Source is core/database package status: confirmed Confirmed by a Strapi Team member or multiple community members

Comments

@WalkingPizza
Copy link
Contributor

Bug report

Required System information

  • Node.js version: v16.16.0
  • Yarn version: 1.22.19
  • Strapi version: 4.3.4
  • Database: SQLite
  • Operating system: MacOS 12

Describe the bug

The SQLite Dialect class does not definite a useReturning method, which defaults the value to false even though SQLite does support RETURNING statements.

This causes a series of issues, including insert queries not returning IDs appropriately from the query builder.

Steps to reproduce the behavior

  1. Launch a test Strapi instance with SQLite.
  2. Edit the entity-manager.js file to console.log the result of await this.createQueryBuilder(uid).insert(dataToInsert).execute().
  3. Try calling strapi.db.query(<UID>).createMany({ data: [<ENTRIES>] }).
  4. Notice how the line from step 2 is only returning one of the multiple IDs it should be returning.

Expected behavior

useReturning should be defined so that it returns true.

@bolg55 bolg55 added issue: bug Issue reporting a bug severity: medium If it breaks the basic use of the product but can be worked around status: confirmed Confirmed by a Strapi Team member or multiple community members source: core:database Source is core/database package labels Aug 31, 2022
@not-vainglorious
Copy link

not-vainglorious commented Sep 15, 2022

Can you check it out again? Im getting node-sqlite3 does not currently support RETURNING clause` with 4.3.8 while using sqlite in tests.

@WalkingPizza
Copy link
Contributor Author

Can you check it out again? I`m getting node-sqlite3 does not currently support RETURNING clause with 4.3.8 while using sqlite in tests.

Are you using better-sqlite3 or the legacy sqlite3 package?

@not-vainglorious
Copy link

not-vainglorious commented Sep 15, 2022

Can you check it out again? I`m getting node-sqlite3 does not currently support RETURNING clause with 4.3.8 while using sqlite in tests.

Are you using better-sqlite3 or the legacy sqlite3 package?

As referenced in docs. I'm using sqlite3. Should i change it to better-sqlite3?

@WalkingPizza
Copy link
Contributor Author

I would, yes, as it is generally better than sqlite3!
@alexandrebodin should we update that piece of documentation to mention better-sqlite3 instead?

@not-vainglorious
Copy link

not-vainglorious commented Sep 15, 2022

I would, yes, as it is generally better than sqlite3! @alexandrebodin should we update that piece of documentation to mention better-sqlite3 instead?

Also you can change this example
This code is old
const dbSettings = strapi.config.get("database.connections.default.settings");
Now seems like it should look like this
const dbSettings = strapi.config.get('database.connection.connection');

async function cleanupStrapi() {
    const dbSettings = strapi.config.get("database.connections.default.settings");
  
    //close server to release the db-file
    await strapi.server.httpServer.close();
  
    //delete test database after all tests have completed
    if (dbSettings && dbSettings.filename) {
      const tmpDbFile = `${__dirname}/../${dbSettings.filename}`;
      if (fs.existsSync(tmpDbFile)) {
        fs.unlinkSync(tmpDbFile);
      }
    }
    // close the connection to the database
    await strapi.db.connection.destroy();
  } 

I changed to next code

async function cleanupStrapi() {
    const dbSettings = strapi.config.get('database.connection.connection');

    // close server to release the db-file
    await strapi.server.httpServer.close();

    // delete test database after all tests have completed
    if (dbSettings && dbSettings.filename) {
        if (fs.existsSync(dbSettings.filename)) {
            fs.unlinkSync(dbSettings.filename);
        }
    }

    // close the connection to the database
    await strapi.db.connection.destroy();
}

P.S. Thanks. With better-sqlite3 everything is fine.

@alexandrebodin
Copy link
Member

ping @pwizla Could you have a look at this :)

@strapi-bot
Copy link

This issue has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/issue-4-3-8-after-update-sqlite-error-near-returnin/22172/5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Issue reporting a bug severity: medium If it breaks the basic use of the product but can be worked around source: core:database Source is core/database package status: confirmed Confirmed by a Strapi Team member or multiple community members
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants