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

Use PartitonKey In update/Delete queries #60

Merged
merged 10 commits into from
May 2, 2022

Conversation

aspladypfl
Copy link
Contributor

@aspladypfl aspladypfl commented Oct 19, 2021

Currently, Updates/deletes only work if the partionkey and the id are the same value. This change will now respect the requestOptions.partitionKey optional value

https://docs.microsoft.com/en-us/javascript/api/%40azure/cosmos/feedoptions?view=azure-node-latest#partitionKey

@aspladypfl aspladypfl mentioned this pull request Oct 29, 2021
@andrejpk
Copy link
Owner

andrejpk commented Nov 3, 2021

Hey this looks good.. a few suggestions that would speed up merging:

  • Add new test cases that validate a full CRUD cycle

You can copy the existing integration test file and add partition keys for those operations. It would be great if they also covered correct handling of a non-happy scenario (for example, excluding a required value or passing an invalid value). These test run locally using the Vercel local CosmosDb-Server so you should easily be able to run them using npm test.

  • Remove the changes on package.json and package-lock.json

I don't think you changed anything in these so this keeps the PR cleaner and less worry that anything broke by mistake.. I'll update those in a separate PR (you can probably see I'm a bit behind on catching up on dependancies.. I need to get to that soon)

With these two things, I think we'd be in good shape to merge. Thanks for taking the time to submit this!

@aspladypfl
Copy link
Contributor Author

Alright I have updated the PR to add more testing infrastructure and PR comments

@andrejpk andrejpk merged commit 6019818 into andrejpk:main May 2, 2022
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.

2 participants