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(schema): avoid returning string 'nested' as schematype #14453

Merged
merged 6 commits into from Mar 21, 2024

Conversation

vkarpov15
Copy link
Collaborator

Fix #14435
Fix #14443

@FaizBShah

Summary

A more "root cause" fix for #14435 than #14443. I think it is incorrect for _getPath() to ever return a string: we only want SchemaType instance or nullish. The issue comes from us setting nested paths to 'nested' for nested paths underneath single nested subdocs - if we don't have that, then the test for #9459 starts failing.

Examples

@FaizBShah
Copy link
Contributor

@vkarpov15 This looks good to me. Just one query, currently according to the docs, function like schema.pathType() returns 'nested' for nested paths. So, will this change affect that or some other functions?

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

unless i missed something, arent the 2 added tests basically the same?

test/document.populate.test.js Outdated Show resolved Hide resolved
test/document.populate.test.js Outdated Show resolved Hide resolved
test/document.populate.test.js Outdated Show resolved Hide resolved
test/document.populate.test.js Outdated Show resolved Hide resolved
vkarpov15 and others added 4 commits March 21, 2024 17:39
Co-authored-by: hasezoey <hasezoey@gmail.com>
Co-authored-by: hasezoey <hasezoey@gmail.com>
Co-authored-by: hasezoey <hasezoey@gmail.com>
Co-authored-by: hasezoey <hasezoey@gmail.com>
@vkarpov15 vkarpov15 added this to the 8.2.3 milestone Mar 21, 2024
@vkarpov15
Copy link
Collaborator Author

Not quite, 2nd test is slightly different. But as far as I can tell, the 2nd test is also unnecessary, it passes both with and without the code changes in this PR.

@vkarpov15 vkarpov15 merged commit b51413c into master Mar 21, 2024
34 checks passed
@vkarpov15 vkarpov15 deleted the vkarpov15/gh-14435 branch March 21, 2024 21:47
@FaizBShah
Copy link
Contributor

FaizBShah commented Mar 22, 2024

unless i missed something, arent the 2 added tests basically the same?

@hasezoey No, in the first test the extras field was an array, while in the 2nd test, the extras field was a Schema field. The reason I wrote the 2 tests was because I saw that the issue was working when it was just a schema field, but failing when it was an array. So I wrote the tests to ensure both passes. And I didn't wrote .create() because the original issuer who gave the repro script didn't have it, but I think the changes done in this PR makes better sense

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.

Nested populate with nested paths and subdocuments is not working fine
3 participants