Skip to content

fix: allows insert primary key with zero#13458

Merged
sdepold merged 5 commits into
sequelize:mainfrom
zbinlin:fixes-insert-zero-1
Oct 24, 2021
Merged

fix: allows insert primary key with zero#13458
sdepold merged 5 commits into
sequelize:mainfrom
zbinlin:fixes-insert-zero-1

Conversation

@zbinlin
Copy link
Copy Markdown
Contributor

@zbinlin zbinlin commented Aug 25, 2021

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Before now, I create a row, the primary key pass 0, the sql generator will ignore the 0, use DEFAULT instead, this pr will fixes the problem.

Examples:

const { Sequelize, Model, DataTypes } = require('sequelize');
const sequelize = new Sequelize('sqlite::memory:');
const asserts = require('assert');

class User extends Model {}
User.init({
  id: {
    type: DataTypes.INTEGER,
    primaryKey: true,
    autoIncrement: true,
  },
  username: DataTypes.STRING,
}, {
  sequelize,
  modelName: 'user',
  timestamps: false,
});

User.sync().then(() => {
  return User.create({
    id: 0,
  });
}).then(user => {
    asserts.equal(user.id, 0); // it will creates record, the id expected is 0, actual is 1.
}).catch(err => console.error(err));

Copy link
Copy Markdown
Member

@wbourne0 wbourne0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself looks fine, but can you add some tests to prevent regression and some examples which show where this can be an issue to the PR description? Thanks!

@wbourne0 wbourne0 added type: bug DEPRECATED: replace with the "bug" issue type status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action labels Oct 10, 2021
@sdepold sdepold self-assigned this Oct 17, 2021
Copy link
Copy Markdown
Member

@sdepold sdepold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good tests 👌

Comment thread lib/dialects/abstract/query-generator.js
Comment thread lib/dialects/abstract/query-generator.js
@wbourne0 wbourne0 dismissed their stale review October 18, 2021 00:31

Looks like the requested changes were made.

@sdepold sdepold merged commit e4aff2f into sequelize:main Oct 24, 2021
@github-actions
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 6.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
* fix: allows insert primary key with zero

* test: allows insert primary key with zero

Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action type: bug DEPRECATED: replace with the "bug" issue type

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants