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.

Sign up for GitHub

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

Jump to bottom

Diffs when json key order changes #3583

Closed
gggdomi opened this issue Oct 11, 2022 · 5 comments · May be fixed by #3584
Closed

Diffs when json key order changes #3583

gggdomi opened this issue Oct 11, 2022 · 5 comments · May be fixed by #3584

Comments

@gggdomi
Copy link

gggdomi commented Oct 11, 2022

Describe the bug

If we update a json field with an identical value, only with a different key order, it's present in changeset (and thus triggers updates).

TLDR:

// pre-existing row.someJsonField = { a: 1, b: 2 }
row.someJsonField = { b: 2, a: 1 }
// the json didn't really changed, yet changeset and updates won't be empty

To Reproduce

Minimal full repro: https://stackblitz.com/edit/mikro-orm-repro-svx3gr?file=test.ts

Expected behavior

The JSON are identical, there should be nothing in changeset and no updates.
(or at least there should be an option to enable this behaviour)

I'm opening a PR with a suggested improvement right away.

Versions

Dependency Version
node v16.14.2
typescript 4.8.3
mikro-orm 5.4.2
your-driver Postgres (repro in SQLite)
@gggdomi gggdomi mentioned this issue Oct 11, 2022
Open
@B4nan
Copy link
Member

B4nan commented Oct 11, 2022

Different order means its a different value.

@B4nan B4nan closed this as not planned Won't fix, can't repro, duplicate, stale Oct 11, 2022
@rvion
Copy link

rvion commented Oct 11, 2022

@B4nan could you reopen this one ?

according to the json spec, it's not

https://www.json.org/json-en.html

An object is an unordered set of name/value pairs.

also, postgres jsonb are specifically order independent (only json, not jsonb use field order)

@B4nan
Copy link
Member

B4nan commented Oct 11, 2022

according to the json spec, it's not

So you say that the native implementation is not spec complaint, but that is rather a problem for TC39, right? JK :]

I dont mind the proposed solution in #3584 (lets discuss the implementation there), but I dont think it should be the default if its slower than the native implementation. But depends on how much slower, if its like 20%, I dont mind, if its 200% percent, its a different story.

@rvion
Copy link

rvion commented Oct 11, 2022

So you say that the native implementation is not spec complaint, but that is rather a problem for TC39, right? JK :]

yeah, fair point 😅
deep object comparing have always been a pain in js 🙃
Sorry if that came out a bit harsh.

I dont mind the proposed solution in #3584 (lets discuss the implementation there)

🙏

but I dont think it should be the default if its slower than the native implementation.

yeah, I tend to agree

But depends on how much slower, if its like 20%, I dont mind, if its 200% percent, its a different story.

No idea how much slower it is; I guess it is an acceptable compromise for those with the need to avoid unnecessary diffs.

I also think there could be an other implementation if Type had the ability to define custom comparer. for JSON, it would probably be a good alternative to stringifying.

@B4nan
Copy link
Member

B4nan commented Oct 11, 2022 •
edited

Not at all, and sorry from my end if my insta-close sometimes looks harsh, its just a way of me saying I dont plan to work on something as I feel its out of scope, especially if its something you can easily work around (like this particular issue, the proposed PR can be used without any changes to the ORM, you can just extend the JsonType and use that in your codebase, you could even override the default type mapping via discover.getMappedType option). It never means "the end of conversaion", or that I wont consider accepting a PR.

I also think there could be an other implementation if Type had the ability to define custom comparer. for JSON, it would probably be a good alternative to stringifying.

Thats a great idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(core): allow JsonType to use a stable JSON.stringify algorithm rvion/mikro-orm
3 participants
@B4nan @rvion @gggdomi