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

Add empty string check for collection name passed #14806

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

Shubham2552
Copy link

@Shubham2552 Shubham2552 commented Aug 14, 2024

@vkarpov15, I was unable to add reviewers directly and would appreciate your feedback on this PR. Please let me know if there are any additional steps I should follow or if you have any suggestions.

Feel free to reach out if you have any questions or need further information.

Summary

The motivation for me to make this pull request is that while we were working on our serverless project we used Mongoose but we got an error that said Cannot read properties of undefined (reading 'toLowerCase') we were not able to fix as on our local we could run and it worked fine so we scratched a lot of our head thinking we didn't use toString function anywhere where is the issue code, so we put consoles and then check on deployment logs and realized the issue was that the secret manager was not properly configured so let say we have process.env.COLLECTION_NAME will be undefined, so the problem was not handled and threw an error from the module where it tried to convert undefined to lowercase using the javascript method but it took a good amount of effort and time to fix it so I thought of understanding the flow and got to know the utils.js file has method that runs a function on collection name, that is toCollectionName so I put a string and empty string check here so meaningful message are thrown and error is immediately caught saving from a lot of frustration, effort and saving time, also I ran the test case to confirm nothing breaks but found some lines were not covered in the test so added some test cases to cover them as well.

Examples

The demonstration is passing values other than string will give Collection name must be a string error message and if it is a string but empty then it will say Collection name cannot be empty.

To test after making a successful mongoose connection:
const User = mongoose.model('', userSchema); or const User = mongoose.model(undefined, userSchema);

Earlier it used to throw a toLowerCase error but now with the above-referenced message will be thrown

Please do let me know if you have any queries regarding, suggestions, feedback, or enhancement to make.

@Shubham2552 Shubham2552 marked this pull request as draft August 16, 2024 08:37
@Shubham2552 Shubham2552 marked this pull request as ready for review August 16, 2024 08:37
@vkarpov15 vkarpov15 added this to the 8.5.4 milestone Aug 16, 2024
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.

Good find, thanks 👍 Empty collection name is a good case for us to handle, since MongoDB disallows empty collection names:

MongoDB Enterprise > db.createCollection('')
{
	"ok" : 0,
	"errmsg" : "Invalid namespace specified 'test.'",
	"code" : 73,
	"codeName" : "InvalidNamespace"
}

@vkarpov15 vkarpov15 merged commit 3e52119 into Automattic:master Aug 16, 2024
24 checks passed
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.

3 participants