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

fix: correctly compute update changes for json / jsonb columns with transformers #6929

Merged

Conversation

dimitryvolkov
Copy link
Contributor

Changes introduced in #5700 lead to JSON / JSONB columns with transformers incorrectly computed for updates, leading to an update being computed even when the column data has not changed.

@dimitryvolkov dimitryvolkov force-pushed the fix-json-transform-update-compute branch from 16c962b to 4639ce8 Compare October 17, 2020 20:37
@dimitryvolkov dimitryvolkov changed the title Correctly compute update changes for json / jsonb columns with transformers fix: correctly compute update changes for json / jsonb columns with transformers Oct 18, 2020
@idobh2
Copy link

idobh2 commented Oct 21, 2020

Just ran into this as well and can confirm the fix works - can this be merged?

@dimitryvolkov
Copy link
Contributor Author

@imnotjames I know you guys are busy and I don't mean to nag, but how should I normally get attention for pull requests?
should I open an issue first and link it? should I assign one of you as reviewers?
Thanks again for your awesome work!

@imnotjames
Copy link
Contributor

Hey there. It's been a few days and I've been doing what I can to review every pull request. There's just a bit of a backlog.

I'll be definitely getting to this, but have been unable to because I've been trying to address some of the older pull requests as well.

I've reviewed roughly 120 pull requests this month, merged 65 - and responded to 1400 issues, closing 900. 😅 I will get to this one, for sure, but I've not yet.

@dimitryvolkov
Copy link
Contributor Author

Thanks I didn't mean to pressure you I know you guys are busy and are doing a great job, I was just wondering if there's something I should be doing as part of the PR process which I was failing to do

@imnotjames
Copy link
Contributor

;) You're doin' great. Wasn't trying to shame you or anything!

This has tests, so I mean, it's amazing & is a small change to review.

Maybe what I'd said was closer to gloating and glee that so much has been accomplished than anything else.

Copy link
Contributor

@imnotjames imnotjames left a comment

Choose a reason for hiding this comment

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

Is there an issue related to this change? If so, don't forget to include it!

I've got one question & possible suggestion?


// get database value of the column
let databaseValue = column.getEntityValue(subject.databaseEntity, true);
let databaseValue = column.getEntityValue(subject.databaseEntity, shouldTransformDatabaseEntity);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that transformers won't run? Perhaps we want to instead change how we do the comparisons?

I think disabling the transformation outright could lead to undesired behavior.

What do you think - where is this comparison failing that we need to not transform it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding transformation here is done only for comparison purposes, if you observe the changes originally made in #5700, this was false by default so I don't expect it will affect anything.

As mentioned, the comparison is failing in the json / jsonb case where the code path is different, for all other cases the normalized entity value is again transformed before comparing, I initially thought to do the same for the 'json' / 'jsonb' case but figured transforming JSONs might incur a performance penalty so if I can spare that then why not?
However I open to suggestions :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link to where the comparison is taking place that you're talking about? I'd like to step through this to verify I understand.

Also, two questions that will help with understanding:

  • So this does not prevent us from running transformers against JSON/JSONB fields?
  • The JSON/JSONB comparisons that you're talking about are doing a deep comparison and not a by-reference comparison?

Copy link

Choose a reason for hiding this comment

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

As far as I understand this does not affect transformation of JSON / JSONB values and only affects comparison used to compute changes for update statements.
You can see the comparison being performed as a deep comparison in lines 103 of the same file, also if you look at the original fix #5700 you might gain insight as to what the original intention was and how not handling JSON fields seems like an oversight in the original PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for the late response, idobh2 is correct here, the transformation here is only use for computing changes, in the end what's being saved as the changed value is the value on the column, the transformation I altered only affects the current database value used for comparison.
As he also mentioned, you can find the JSON / JSONB case comparison at line 103, before the original fix in #5700 what was being compared are the non-transformed versions of the changed value and DB value, following that fix what's being compared is the entity value and transformed entity value.

This would cause (as the test case shows) the comparison for JSON / JSONB columns with transformers to always show a change, causing an unnecessary update to be generated for such columns

@dimitryvolkov
Copy link
Contributor Author

You have every right to boast about your accomplishments you guys have been doing a great job with this project!

@imnotjames imnotjames added the hacktoberfest-accepted label hacktoberfest label Oct 26, 2020
@dimitryvolkov
Copy link
Contributor Author

anything else you guys need from me to get this moving?

@dimitryvolkov
Copy link
Contributor Author

@imnotjames I know you're probably busy but any way to show this some love before it gets bumped down to the second page of PRs?

@dimitryvolkov
Copy link
Contributor Author

@imnotjames @pleerock what's needed to get this moving?

@dimitryvolkov
Copy link
Contributor Author

@pleerock @imnotjames any chance to give this some love?

@AlexMesser AlexMesser merged commit 6be54d4 into typeorm:master Mar 27, 2021
@AlexMesser
Copy link
Collaborator

thank you for contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted label hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants