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

table store batch to reject batches with different partitionkeys #2333

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kentis
Copy link

@kentis kentis commented Jan 4, 2024

Thanks for contribution! Please go through following checklist before sending PR.

This PR fixes issue #1215

PR Branch Destination

  • For Azurite V3, please send PR to main branch.
  • For legacy Azurite V2, please send PR to legacy-dev branch.

Always Add Test Cases

Test case is added but skipped since it requires modifications to the azure table client dependency to run properly. This is because the client perform the same check. This is, howevernot true for all clients.

Add Change Log

Add change log for the code change in Upcoming Release section in ChangeLog.md.

Development Guideline

Please go to CONTRIBUTION.md for steps about setting up development environment and recommended Visual Studio Code extensions.

@edwin-huber
Copy link
Collaborator

Reviewing

@edwin-huber edwin-huber self-requested a review January 8, 2024 07:54
@edwin-huber edwin-huber added the table-storage Relating to Azurite table storage implementation label Jan 8, 2024
@blueww
Copy link
Member

blueww commented Jan 18, 2024

@edwin-huber

Feel free to let me know if you need any assistance on this PR reviewing.

batchContextClone: any
) {
if (this.partitionKeyMap.size > 0) {
console.log("checking for differing partition keys");
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove console.log calls

@@ -604,4 +604,46 @@ describe("table Entity APIs test", () => {

await tableClientrollback.deleteTable();
});

//Skips this test because it requires modifying the Azure Data Tables client so that it does not perform a check for partition key consistency before submitting the batch.
it.skip("10. Batch API should fail when attempting to insert elements with different partitionkeys in same batch, @loki", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please look at the table.entity.rest.test.ts file, there is an example of a batch request in there, which you can use to simulate this request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am just updating the tests, which should make this easier for you

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is taking a while to convert all the REST tests to the new method and remove dependencies, hope to be done by the end of the week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
table-storage Relating to Azurite table storage implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants