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 schema as a schema path name #8798

Closed
songguangyu opened this issue Apr 15, 2020 · 14 comments
Closed

Allow schema as a schema path name #8798

songguangyu opened this issue Apr 15, 2020 · 14 comments
Assignees
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@songguangyu
Copy link

songguangyu commented Apr 15, 2020

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
When i set schema name schema,
image

mongoose throw err
image

mongoose/lib/schema.js line 528

image

If the current behavior is a bug, please provide the steps to reproduce.

const schema = new Schema({ 'schema': { type: String, default: '{}'}, });
What is the expected behavior?
I can set name schema in Schema
What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.
Mongoose version 5.4.7
5.9.9 alse have this problem
https://github.com/Automattic/mongoose/blob/master/lib/schema.js#L645
Nodejs version 12

@AbdelrahmanHafez
Copy link
Collaborator

@songguangyu
Copy link
Author

@AbdelrahmanHafez Thanks for you answer, I am working on service migration. If the schema field already exists in the original database, how should I deal with this situation?

@AbdelrahmanHafez
Copy link
Collaborator

In that case I would rename the schema field into a different field name.
I've always found thesaurus quite helpful in finding synonyms.

@songguangyu
Copy link
Author

@AbdelrahmanHafez Maybe another mongo client is a better solution.

@AbdelrahmanHafez
Copy link
Collaborator

I can't make that choice for you, depending on the effort of renaming the field, and the features mongoose provides. See what outweighs what, and make your decision.

Perhaps @vkarpov15 has a better idea.

@AbdelrahmanHafez AbdelrahmanHafez added the help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary label Apr 20, 2020
@songguangyu
Copy link
Author

@AbdelrahmanHafez thanks!

@vkarpov15
Copy link
Collaborator

Some path names unfortunately do conflict with existing methods and properties. However, there are ways for us to work around this. I'll keep this open for a future release.

@vkarpov15 vkarpov15 changed the title Some path names conflict with document methods Allow schema as a schema path name Apr 23, 2020
@vkarpov15 vkarpov15 added this to the 5.x Unprioritized milestone Apr 23, 2020
@vkarpov15 vkarpov15 added enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature and removed help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary labels Apr 23, 2020
@vkarpov15 vkarpov15 modified the milestones: 5.x Unprioritized, 5.11 Aug 16, 2020
@vkarpov15 vkarpov15 added the good first issue This issue is a quick fix that would be a good one for a first-time contributor. label Aug 16, 2020
@JatinRanka
Copy link

@vkarpov15 Can I take up this issue to work on?

@vkarpov15
Copy link
Collaborator

@JatinRanka sure 👍

@vkarpov15 vkarpov15 modified the milestones: 5.11, 5.12 Nov 6, 2020
@Edu93Jer
Copy link

Edu93Jer commented Dec 1, 2020

Hi, @vkarpov15 I am a newbie to this project and would like to contribute it; do you think you can put me in the right direction, please?

@vkarpov15
Copy link
Collaborator

@Edu93Jer sure. The general idea is that several methods in lib/document.js relies on this.schema to get the document's schema, which is why schema isn't allowed as a path name. We would need to refactor lib/document.js to ideally rely on a symbol instead of 'schema', but we could get the same result by changing the name of the 'schema' property on documents to '$__schema' or something similar.

@Edu93Jer
Copy link

Edu93Jer commented Dec 2, 2020

@vkarpov15 when you say "changing the name of the 'schema' property on documents to '$__schema' " you mean in all the documents or just where the property is added, because I just did this, and the issue works well. (Line 592)
Capture12

@vkarpov15
Copy link
Collaborator

That's one step. But the reason why reserved.schema is set is that Mongoose documents assume that this.schema is the schema belonging to the model that this document is an instance of. For example, here:

const schema = this.schema;
.

Any change to reserved.schema also means we need to change all the places in lib/document.js where we use this.schema.

Edu93Jer added a commit to Edu93Jer/mongoose that referenced this issue Dec 22, 2020
@vkarpov15 vkarpov15 removed the good first issue This issue is a quick fix that would be a good one for a first-time contributor. label Dec 27, 2020
@vkarpov15
Copy link
Collaborator

In lib/model.js and lib/document.js there's a lot of places we use this.schema. We should use this.$__schema instead internally.

IslandRhythms added a commit that referenced this issue Jan 25, 2021
adding $__ in front of schema at line 1671 let all the toObject tests pass and fixed the maximum stack error
vkarpov15 added a commit that referenced this issue Jan 26, 2021
This was referenced Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
Development

No branches or pull requests

6 participants