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

footgun/regression: Creating a new model with a JSONB column set to null inserts a JSONB null value rather than leaving the column as null #578

Closed
3 of 6 tasks
papandreou opened this issue Sep 12, 2023 · 4 comments · Fixed by sequelize/sequelize#16861

Comments

@papandreou
Copy link

papandreou commented Sep 12, 2023

Issue Creation Checklist

  • I understand that my issue will be automatically closed if I don't fill in the requested information
  • I have read the contribution guidelines

Bug Description

sequelize/sequelize#15598 tightened up the handling of JSONB values in WHERE clauses for the better. However, it subtly changed how Model.create works with nullable JSONB columns. Before, creating a model with such a column explicitly set to null would make the column null:

INSERT INTO "my_models" ("myModelId","jsonbColumn") VALUES (DEFAULT,NULL) RETURNING "myModelId", "jsonbColumn";

After sequelize/sequelize#15598 was merged, 'null' is inserted, which makes the column contain a JSONB null value:

INSERT INTO "my_models" ("myModelId","jsonbColumn") VALUES (DEFAULT,'null') RETURNING "myModelId", "jsonbColumn";

A workaround is to leave out the value (or pass undefined) when you mean to leave the column as null, as that'll omit it from the INSERT, leaving it as null:

INSERT INTO "my_models" ("myModelId") VALUES (DEFAULT) RETURNING "myModelId", "jsonbColumn";

This change in behavior doesn't seem to be called out in the PR description for sequelize/sequelize#15598 or in #417, so I assume it's a regression. The spirit of that change was to tighten up how JSONB columns are handled, where it now forces you to actively use { [Op.is]: null } or { [Op.eq]: null } in WHERE clauses, which is great. Equivalent to that you should probably be forced to select between populating a column with null or a JSONB null value. Until we can get there, I'd argue that the semantics shouldn't change.

Reproducible Example

import Sequelize from '@sequelize/core';

const sequelize = new Sequelize({
  dialect: 'postgres',
  port: 5432,
  // ... credentials
});

class MyModel extends Sequelize.Model {}

MyModel.init(
  {
    myModelId: {
      type: Sequelize.DataTypes.BIGINT,
      primaryKey: true,
      autoIncrement: true,
    },
    jsonbColumn: {
      type: Sequelize.DataTypes.JSONB,
    },
  },
  {
    sequelize,
    tableName: 'my_models',
    timestamps: false,
  },
);

await sequelize.sync();

await sequelize.models.MyModel.create({
  jsonbColumn: null,
});

await sequelize.close();

What do you expect to happen?

That Sequelize populates the jsonbColumn column with null.

What is actually happening?

It gets populated with a JSONB null value.

Environment

  • Sequelize version: 7.0.0-alpha.28
  • Node.js version: 18.16.1
  • If TypeScript related: TypeScript version: 5.2.2
  • Database & Version: PostgreSQL 15.3
  • Connector library & Version: pg 8.10.0

Would you be willing to resolve this issue by submitting a Pull Request?

  • Yes, I have the time and I know how to start.
  • Yes, I have the time but I will need guidance.
  • No, I don't have the time, but my company or I are supporting Sequelize through donations on OpenCollective.
  • No, I don't have the time, and I understand that I will need to wait until someone from the community or maintainers is interested in resolving my issue.

Indicate your interest in the resolution of this issue by adding the 👍 reaction. Comments such as "+1" will be removed.

@papandreou papandreou changed the title footgun/regression: Creating a new model with a jsonb column set to null inserts a JSONB null value rather than leaving the column as null footgun/regression: Creating a new model with a JSONB column set to null inserts a JSONB null value rather than leaving the column as null Sep 12, 2023
@ephys
Copy link
Member

ephys commented Sep 12, 2023

It is intended, but I did indeed forget to call it out explicitly, very sorry about that

I'll add extra documentation on https://sequelize.org/docs/v7/querying/json/#json-null-vs-sql-null and on https://sequelize.org/docs/v7/models/data-types/#json--jsonb

@ephys ephys transferred this issue from sequelize/sequelize Sep 12, 2023
@papandreou
Copy link
Author

Thanks for clarifying! I'm worried that this will cause problems for people who upgrade to Sequelize 7, because they might not immediately realize that the behavior is changing? Also, I think the old behavior is less surprising, as that's what passing null does for all other nullable columns?

@ephys
Copy link
Member

ephys commented Dec 11, 2023

I've opened a PR to implement this feature: sequelize/sequelize#16861

@papandreou
Copy link
Author

Thanks a lot for following up with that! ❤️

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 a pull request may close this issue.

2 participants