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

Problem comparing json default value in postgresql migrations #4212

Closed
DeityLamb opened this issue Apr 10, 2023 · 6 comments
Closed

Problem comparing json default value in postgresql migrations #4212

DeityLamb opened this issue Apr 10, 2023 · 6 comments
Labels
bug Something isn't working

Comments

@DeityLamb
Copy link

Describe the bug
After we created the initial migration for the model with a JSON field that has a default value, all next migrations will attempt to modify the default value to the same one.

To Reproduce
https://github.com/DeityLamb/mikro-orm-issue
Create an entity that has a json field with default value

mikro-orm migration:create
# created first migration
mikro-orm migration:up
# we up migration and not to change anything
mikro-orm migration:create
# a new migration was created that changes the default value to the same

Expected behavior
Do not include changes to default values in migration if they have not been modified.

Versions

Dependency Version
node v18.14.2
typescript ^4.9.5
mikro-orm ^5.6.16
postgresql 15.1

Quick inspection

Entity

import { Entity, JsonType, PrimaryKey, Property } from "@mikro-orm/core";

interface UserMetadata {
  value: number;
}

@Entity()
export class User {

  @PrimaryKey()
  public id!: number;

  @Property({ type: JsonType, default: JSON.stringify({ value: 42 }) })
  public metadata!: UserMetadata;
}

Initial migration

import { Migration } from '@mikro-orm/migrations';

export class Migration20230410153819 extends Migration {

  async up(): Promise<void> {
    this.addSql('create table "user" ("id" serial primary key, "metadata" jsonb not null default \'{"value":42}\');');
  }

  async down(): Promise<void> {
    this.addSql('drop table if exists "user" cascade;');
  }

}

All subsequent migrations will include this even if the default value has not been changed.

import { Migration } from '@mikro-orm/migrations';

export class Migration20230410160922 extends Migration {

  async up(): Promise<void> {
    this.addSql('alter table "user" alter column "metadata" type jsonb using ("metadata"::jsonb);');
    this.addSql('alter table "user" alter column "metadata" set default \'{"value":42}\';');
  }

  async down(): Promise<void> {
    this.addSql('alter table "user" alter column "metadata" type jsonb using ("metadata"::jsonb);');
    this.addSql('alter table "user" alter column "metadata" set default \'{"value": 42}\';');
  }

}
@B4nan
Copy link
Member

B4nan commented Apr 10, 2023

default is the runtime value, so not string, but object.

@B4nan
Copy link
Member

B4nan commented Apr 10, 2023

Maybe the diffing does not work correctly for JSON type, it's a bit tricky - there are some changes in master that should help with that. Could you try latest dev version and drop the JSON.stringify?

Or try using defaultRaw instead.

@DeityLamb
Copy link
Author

DeityLamb commented Apr 10, 2023

The development version, unfortunately, does not solve the problem. We anyway cannot pass the object to default field.

Currently, we are converting the object to a jsonb string using JSON.stringify with additional indentations and field sorting.
This is a working solution, but it is difficult to call it reliable.

For my specific case, the ideal approach would be to generate default values based on a deep comparison of both objects.

Perhaps something like this can be made here, i guess

Something like this check:

if (to.mappedType instanceof JsonType && from.default && to.default) {

  // remove single quotes from the start and end of default json
  const normalize = (str: string) => str.replace(/(^'|'$)/g, '');

  const defaultValueFrom = parseJsonSafe(normalize(from.default));
  const defaultValueTo = parseJsonSafe(normalize(to.default));

  return Utils.equals(defaultValueTo, defaultValueFrom);
}

The problem may be much more complex. Unfortunately, I am not familiar with the codebase.

@B4nan
Copy link
Member

B4nan commented Apr 11, 2023

Defaults are compared via prop.defaultRaw, the link you mentioned is not comparing EntityProperty (which has default and defaultRaw) but a Column which only has default (and Column.default is exactly the same as EntityProperty.defaultRaw. And there the value should automatically stringified via the JsonType, but I think I see why it's not working - this seems to be hardcoded only for the ArrayType.

https://github.com/mikro-orm/mikro-orm/blob/master/packages/core/src/metadata/MetadataDiscovery.ts#L969-L971

Once this is fixed, you will most probably need to drop that stringify part.

@cloudever
Copy link

cloudever commented Apr 12, 2023

There is similar issue when persisting entities.

Example: Mikro-ORM loads entity list via repository.findAndCount() with filled json columns from database. And when I create a new entity and call repository.persist() mikro-orm update previously loaded entities with same JSON values, so I got unnecessary update work. Version is v.5.6.16

@B4nan
Copy link
Member

B4nan commented Apr 12, 2023

Example: Mikro-ORM loads entity list via repository.findAndCount() with filled json columns from database. And when I create a new entity and call repository.persist() mikro-orm update previously loaded entities with same JSON values, so I got unnecessary update work. Version is v.5.6.16

Try latest dev version, that should be now working correctly.

@B4nan B4nan added the bug Something isn't working label Apr 22, 2023
@B4nan B4nan closed this as completed in 41277a1 Apr 22, 2023
jsprw pushed a commit to jsprw/mikro-orm-full-text-operators that referenced this issue May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants