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

Allow usage of schema reserved keywords #10414

Merged
merged 56 commits into from Aug 20, 2021

Conversation

AbdelrahmanHafez
Copy link
Collaborator

fixes #9010

@AbdelrahmanHafez
Copy link
Collaborator Author

I think we should replace the error throwing with a warning when someone uses a key that is previously reserved, the warning should include a link to an FAQ letting people know the history of reserved, why it's there, and advise them to prefer $method over method, then we'll need to offer an option to shut that warning off on each schema and globally.

@vkarpov15
Copy link
Collaborator

I agree with this comment: #10414 (comment)

My only concern thus far is changing the docs to use $save() instead of save(). Does that change feel too heavy to you? We will always support .save() unless the user explicitly creates a save property on the schema, so I'm a little wary of changing it in the docs because it implies that .save() will no longer work.

@AbdelrahmanHafez
Copy link
Collaborator Author

Actually, I agree. I don't think we should change it in the docs now that we'll warn users when they're using reserved keys, my initial thought was that we'll prefer $save(...) as a default over save(...), but that's not true for users, it should be the default for plugin authors and mongoose internal usage.

I'll revert the changes in the docs.

@AbdelrahmanHafez
Copy link
Collaborator Author

Assumption to be tested:
any object that isMongooseObject(obj) === true inherits from Document, therefore it has access to $toObject(...)

@AbdelrahmanHafez AbdelrahmanHafez marked this pull request as ready for review August 5, 2021 00:10
@AbdelrahmanHafez
Copy link
Collaborator Author

AbdelrahmanHafez commented Aug 5, 2021

@vkarpov15
All ready except for the following:

  • Preferring $toObject over toObject internally, the name toObject is used all over the place and I am not sure what inherits from what
  • Allowing schema.reserved.collection and using collection in schema results in an error that I yet haven't been able to resolve:
TypeError: Cannot read property 'Symbol(mongoose#Document#scope)' of undefined
    at Model.set [as collection] (lib\helpers\document\compile.js:176:32)
    at Function.compile (lib\model.js:4804:30)
    at Mongoose._model (lib\index.js:554:27)
    at NativeConnection.Connection.model (lib\connection.js:1139:23)
    at NativeConnection.conn.model (test\common.js:106:18)
    at Context.<anonymous> (test\document.test.js:10536:25)
    at processImmediate (internal/timers.js:464:21)

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Great work, thanks 👍

@vkarpov15 vkarpov15 merged commit 14dd60b into Automattic:6.0 Aug 20, 2021
@AbdelrahmanHafez
Copy link
Collaborator Author

Happy to help 👍

@AbdelrahmanHafez AbdelrahmanHafez deleted the gh-9010 branch August 20, 2021 05:24
vkarpov15 added a commit that referenced this pull request Aug 23, 2021
vkarpov15 added a commit that referenced this pull request Aug 24, 2021
@steven266
Copy link

@AbdelrahmanHafez Thank you for your great work!

#10414 (comment)

Allowing schema.reserved.collection and using collection in schema results in an error that I yet haven't been able to resolve:

It seems like collection is still not usable, right? Is there any way I can help to make collection usable as a schema key?

@AbdelrahmanHafez
Copy link
Collaborator Author

Yeah, that's still one of the words that are unusable yet.
The way I would do it is pull the repository locally, start a new branch starting from master, go to the following line:

'emit', 'listeners', 'on', 'removeListener', /* 'collection', */ // TODO: add `collection`

Uncomment collection, and to run the tests:

$ npm i
$ npm run tdd

This test would fail, I'd see where it's failing and try to make it pass, we then need to make an alias for collection called $collection, and if anything internally in mongoose is using collection, we need to make it using $collection instead.

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.

None yet

3 participants