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

fix: preserve id on update #533

Merged
merged 1 commit into from
Jul 26, 2019
Merged

fix: preserve id on update #533

merged 1 commit into from
Jul 26, 2019

Conversation

hacksparrow
Copy link
Contributor

@hacksparrow hacksparrow commented Jul 1, 2019 •
edited
Loading

Leave the id property alone on update.

Description

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@hacksparrow
Copy link
Contributor Author

I wonder why delete data[idName]; was there in the first place. Maybe a remnant of copy-pasting from the implementation of other methods.

Do we need to add tests for this?

raymondfeng
raymondfeng reviewed Jul 1, 2019
View reviewed changes
lib/mongodb.js
@@ -1606,8 +1606,6 @@ MongoDB.prototype.update = MongoDB.prototype.updateAll = function updateAll(
const idName = this.idName(modelName);

where = self.buildWhere(modelName, where, options);

delete data[idName];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should clone data, excluding id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why clone? data could be deeply nested, and similar operations are performed in other places. It will introduce a new level of complexity and add to the scope.

data is passed around multiple layers and maybe be referenced from a controller, for example in the reported issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

For update to an existing document in mongodb, we delete the id on purpose IIRC. The current code changes the original input as complained by loopbackio/loopback-next#3267. I think the fix should leave data intact, but remove id from the object to be written to the database.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos @b-admike @jannyHou thoughts?

Deleting the id is ideal and works perfectly, unless data is referenced in a controller. In the controller data exists as an instance of the Model, but it is the same object.

So, I guess we should go ahead with deep copy of data without the id.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we only have to remove id, shadow copy is probably good enough.

bajtos
bajtos reviewed Jul 2, 2019
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Please add a test to verify the changes and prevent regressions in the future.

Removing id (PK) from data makes sense to me. I agree with @raymondfeng that a shallow clone is enough, e.g.:

data = Object.assign({}, data);
delete data[idName];

bajtos
bajtos reviewed Jul 11, 2019
View reviewed changes
test/mongodb.test.js Outdated Show resolved Hide resolved
test/mongodb.test.js Outdated Show resolved Hide resolved
test/mongodb.test.js Outdated
it('should preserve the id', async function() {
let user = await User.create({name: 'Al', age: 31, email: 'al@strongloop'});
const userId = user.id;
user = user.toObject();
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this property userData to make it clear that it's a plain-data object, not a model instance. (And also to avoid a mutable variable user.)

Suggested change
user = user.toObject();
userData = user.toObject();

Copy link
Member

@bajtos bajtos Jul 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I am afraid my comment was not resolved :(

Let's call this property userData to make it clear that it's a plain-data object, not a model instance. (And also to avoid a mutable variable user.)

Suggested change
user = user.toObject();
const userData = user.toObject();

raymondfeng
raymondfeng reviewed Jul 15, 2019
View reviewed changes
test/mongodb.test.js Outdated Show resolved Hide resolved
bajtos
bajtos suggested changes Jul 16, 2019
View reviewed changes
test/mongodb.test.js Outdated Show resolved Hide resolved
@hacksparrow
Copy link
Contributor Author

@bajtos @raymondfeng looks good now?

b-admike
b-admike reviewed Jul 16, 2019
View reviewed changes
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

The patch overall LGTM 👍 ; I have one question for the tests.

test/mongodb.test.js Show resolved Hide resolved
b-admike
b-admike approved these changes Jul 25, 2019
View reviewed changes
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

LGTM

bajtos
bajtos approved these changes Jul 25, 2019
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Looks like you haven't addressed one of my comments, PTAL. The rest of the pull request looks good.

test/mongodb.test.js Outdated
it('should preserve the id', async function() {
let user = await User.create({name: 'Al', age: 31, email: 'al@strongloop'});
const userId = user.id;
user = user.toObject();
Copy link
Member

@bajtos bajtos Jul 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I am afraid my comment was not resolved :(

Let's call this property userData to make it clear that it's a plain-data object, not a model instance. (And also to avoid a mutable variable user.)

Suggested change
user = user.toObject();
const userData = user.toObject();

@hacksparrow
Copy link
Contributor Author

hacksparrow commented Jul 25, 2019 •
edited
Loading

@bajtos errr thanks for pointing out. Updated.

fix: preserve id on update
c5200e2 
Don't delete the id on update
@hacksparrow hacksparrow merged commit c5200e2 into master Jul 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/update-id branch July 26, 2019 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b-admike b-admike b-admike approved these changes

@bajtos bajtos bajtos approved these changes

@raymondfeng raymondfeng raymondfeng approved these changes

@dhmlau dhmlau Awaiting requested review from dhmlau dhmlau is a code owner

@emonddr emonddr Awaiting requested review from emonddr

@jannyHou jannyHou Awaiting requested review from jannyHou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hacksparrow @raymondfeng @bajtos @b-admike