Skip to content

Adding seal endpoint for Append Blob issue #810#2553

Merged
blueww merged 5 commits intoAzure:mainfrom
matt-lethargic:seal-append-blob
Jun 18, 2025
Merged

Adding seal endpoint for Append Blob issue #810#2553
blueww merged 5 commits intoAzure:mainfrom
matt-lethargic:seal-append-blob

Conversation

@matt-lethargic
Copy link
Contributor

This pull request introduces support for sealing append blobs in the Azure Blob Storage emulator this should solve issue #810 . Key changes include implementing the sealBlob functionality across the metadata store layers, updating the AppendBlobHandler to handle the seal operation, and adding tests to validate the new behavior.

Append Blob Sealing Feature:

Testing Enhancements:

  • New Test Cases for Sealing:
    • Added tests to validate the sealing of append blobs, including scenarios for successful sealing, blob not found, invalid blob type, and verification of sealed properties. (tests/blob/apis/appendblob.test.ts, tests/blob/apis/appendblob.test.tsR682-R735)

Miscellaneous:

  • New NPM Script:
    • Added a new test script test:append to specifically run tests for append blob APIs. (package.json, package.jsonR317)

@matt-lethargic matt-lethargic changed the title Adding seal endpoint for Append Blob Adding seal endpoint for Append Blob issue #810 May 16, 2025
assert.deepStrictEqual(resultAfter.isSealed, true);
});

it("Seal append blob get blob properties @loki", async () => {
Copy link
Member

@blueww blueww May 20, 2025

Choose a reason for hiding this comment

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

Seal blob will make blob read-only, please add the implementation and test case, make the error aligned with product azure. (Or we will have customer issue.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should have addressed this, please detail anything that I have now missed :-)

@blueww
Copy link
Member

blueww commented May 20, 2025

@EmmaZhu
Would you please help to review this PR?

@blueww blueww requested a review from EmmaZhu May 20, 2025 03:24
* @throws StorageErrorFactory.getBlobNotFound
* @returns
*/
public async sealBlob(
Copy link
Member

@blueww blueww May 20, 2025

Choose a reason for hiding this comment

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

Append blob is actually not supported on SQL for azurite.
So implement SQL for this might won't take any effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove and throw a not implemented exception?

Copy link
Member

Choose a reason for hiding this comment

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

Please remove it to avoid confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now throw "NotImplementedinSQLError"

@matt-lethargic
Copy link
Contributor Author

I have now added all the functionality relating to sealing an append blob according to the documentation, especially the remarks section of the append blob docs. I've assumed the latest version of the API. If there is any functionality I've missed please feel free to detail and I'll amend.


const result_startcopy = await destBlobClient.beginCopyFromURL(
sourceBlobClient.url, {
sealBlob: true,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this test case and put a check in the copy to only copy isSealed for an appendblob

package.json Outdated
"test": "npm run lint && cross-env NODE_TLS_REJECT_UNAUTHORIZED=0 mocha --require ts-node/register --no-timeouts --grep @loki --recursive --exit tests/**/*.test.ts tests/**/**/*.test.ts",
"test:in-memory": "npm run lint && cross-env AZURITE_TEST_INMEMORYPERSISTENCE=1 NODE_TLS_REJECT_UNAUTHORIZED=0 mocha --require ts-node/register --no-timeouts --grep @loki --recursive --exit tests/**/*.test.ts tests/**/**/*.test.ts",
"test:blob": "npm run lint && cross-env NODE_TLS_REJECT_UNAUTHORIZED=0 mocha --require ts-node/register --no-timeouts --grep @loki --recursive --exit tests/blob/*.test.ts tests/blob/**/*.test.ts",
"test:append": "npm run lint && cross-env NODE_TLS_REJECT_UNAUTHORIZED=0 mocha --require ts-node/register --no-timeouts --grep @loki --recursive --exit tests/blob/apis/appendblob.test.ts",
Copy link
Member

@blueww blueww May 22, 2025

Choose a reason for hiding this comment

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

Why add a new test command? Would you like to remove it?
If you only run to run test in a single file, you can open the file , and run "Current Mocha TS File - Loki" as below picture.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, my bad didn't mean to commit this

}
});

it("should list append blobs in container with sealed property @loki @sql", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Please fix the test failure.
The append blob related case should not run on SQL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@matt-lethargic
Copy link
Contributor Author

Hey @blueww is there any movement on this? I should have addressed all of the issues now.

@blueww
Copy link
Member

blueww commented Jun 3, 2025

Thanks for your patient!

Just back from vacation. Has a lot of items to handle.
Will try to take a look this this or next week.

@matt-lethargic
Copy link
Contributor Author

@blueww can we get this sorted soon?

@blueww
Copy link
Member

blueww commented Jun 18, 2025

/azp run

@blueww blueww merged commit e20f867 into Azure:main Jun 18, 2025
35 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.

2 participants