thinkjs / thinkjs

Use full ES2015+ features to develop Node.js applications, Support TypeScript.
https://thinkjs.org/
MIT License
5.31k stars 616 forks source link

[think-model-sqlite] Default values surrounded by single quotes #1721

Closed charlyisidore closed 1 year ago

charlyisidore commented 1 year ago

DESC

I use a SQLite database. I create the following table:

CREATE TABLE `post` (
  `id` integer NOT NULL PRIMARY KEY AUTOINCREMENT,
  `title` text NOT NULL,
  `content` text NOT NULL DEFAULT 'abc'
);

Expected behavior: When I add a post to the database without specifying its content value, the default content value should be the string abc.

Actual behavior: Instead, the default value is surrounded by two single quotes ('abc'), as shown by the log:

[INFO] - SQL: INSERT INTO post (title,content) VALUES ('hello','''abc''')

ENV

OS Platform: Ubuntu 22.04

Node.js Version: 16.20.1

ThinkJS Version: 3.2.15 (think-model: 1.5.4, think-model-sqlite: 1.3.1)

code

// Creates a post, but unexpectedly sets the `content` value to `'abc'`
const id = await this.model('post')
  .add({ title: 'hello' });

const post = await this.model('post')
  .where({ id })
  .find();

console.log(post);
// { id: 1, title: 'hello', content: "'abc'" }

how to reproduce

I attached a project zip to this issue.

thinkjs-sqlite-issue.zip

To reproduce the bug, create the SQLite database:

mkdir -p runtime
rm -f runtime/bugfix.sqlite
sqlite3 runtime/bugfix.sqlite '.read table.sql'

Run the server:

npm install
npm start

Access http://localhost:8360/

more description

Running PRAGMA table_info( post ) in SQLite shows:

0|id|INTEGER|1||1
1|title|TEXT|1||0
2|content|TEXT|1|'abc'|0

This is where the 'abc' value comes from.

The 'abc' value is assigned here: https://github.com/thinkjs/think-model-sqlite/blob/d6f52a6629b069c6d47f4700c6ad54bed5432421/lib/schema.js#L75

It is later passed to _parseItemSchema() but the original value 'abc' remains.

workaround

The default value can be overridden by creating a file src/model/post.js:

module.exports = class extends think.Model {
  get schema() {
    return {
      content: {
        // Force the default value
        default: 'abc',
      }
    };
  }
};

possible fix

Remove the surrounding quotes from item.default in _parseItemSchema(): https://github.com/thinkjs/think-model-sqlite/blob/d6f52aa6629b069c6d47f4700c6ad54bed5432421/lib/schema.js#L30

lizheming commented 1 year ago

Thank you for your feedback, I will try to reproduce and confirm this problem in the future. The solution you provided is great. If possible, you can submit a PR according to this idea. After I confirm the problem, I can merge it directly.

charlyisidore commented 1 year ago

Many thanks.

I realize that a simpler fix for lib/schema.js (line 75) is to ignore item.dflt_value and set item.default to the empty string '', as done in think-model-mysql and think-model-postgresql.

It results in letting the native SQLite driver set the default value, which might be a more desirable behavior, and avoids parsing the SQLite syntax.

I plan to submit a PR with this solution.

lizheming commented 1 year ago

It has been confirmed locally that there is indeed this problem, thank you very much.

Your issue and problem-solving ideas are very clear and correct, and your understanding of ThinkJS is also in place

think-model-sqlite@1.3.2 released, thanks for your feedback and fix commits.