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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: coerce property value defined as array of ObjectID #582

Merged
merged 2 commits into from Jul 10, 2020
Merged

Conversation

agnes512
Copy link
Contributor

@agnes512 agnes512 commented Jun 30, 2020

Fixes loopbackio/loopback-next#5848 : failed to set array of strings to ObjectId with dataType: objectid.

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Commit messages are following our guidelines

@agnes512 agnes512 changed the title fix: allow arrays of strings to be stored as type ObjectId in database fix: coerce property value defined as array of ObjectID Jun 30, 2020
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

馃憤 The fix looks reasonable. I left a comment about your note.

title: 'arrayOfObjectID',
});
const found = await Article.findOne({where: {title: 'arrayOfObjectID'}});
found.xidArr.should.be.an.Array().which.containDeep([new ds.ObjectID(objectIDLikeString)]);
// the type of the returned array is actually string even it's stored as ObjectIds in the db as expected
Copy link
Contributor

Choose a reason for hiding this comment

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

the type of the returned array is actually string

@agnes512 Isn't the returned xidArr in type objectId? From the test case, it's not string...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am confused too. I checked with found.xidArr[0].should.be.an.instanceOf(ds.ObjectID), and it fails, while the xid test case in L129 is an objectId. I couldn't find where the array ges converted though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what's the typeof found.xidArr[0] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

String!

@agnes512
Copy link
Contributor Author

agnes512 commented Jul 8, 2020

@slnode test please

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

馃憤 LGTM

]);
// check if the array is stored in ObjectId
const raw = await findRawModelDataAsync('ArticleC', found[0].id);
raw.xidArr[0].should.be.an.instanceOf(ds.ObjectID);
Copy link
Contributor

Choose a reason for hiding this comment

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

馃憤

@agnes512 agnes512 merged commit b12633b into master Jul 10, 2020
@delete-merged-branch delete-merged-branch bot deleted the array/objid branch July 10, 2020 15:52
@agnes512 agnes512 mentioned this pull request Aug 4, 2020
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 this pull request may close these issues.

@property.array does not convert string to MongoDB ObjectId
2 participants