Block blob StageBlockWithURL isn't implemented

radekg commented 3 months ago

Which service(blob, file, queue, table) does this issue concern?

This ticket is for the blob service.

Which version of the Azurite was used?

Version: 2024.04 Version 3.30.0.

Where do you get Azurite? (npm, DockerHub, NuGet, Visual Studio Code Extension)

DockerHub

What's the Node.js version?

Running in Docker, irrelevant.

What problem was encountered?

RESPONSE 500: 500 Current API is not implemented yet. Please vote your wanted features to https://github.com/azure/azurite/issues
ERROR CODE: APINotImplemented
--------------------------------------------------------------------------------
<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"yes\"?>
<Error>
  <Code>APINotImplemented</Code>
  <Message>Current API is not implemented yet. Please vote your wanted features to https://github.com/azure/azurite/issues\nRequestId:8acf1d5b-511f-4c13-8044-af31e423b2ae\nTime:2024-07-30T08:03:49.048Z</Message>
</Error>
--------------------------------------------------------------------------------

Steps to reproduce the issue?

Issue a request to blockblob client StageBlockWithURL(...).

https://github.com/Azure/Azurite/blob/2945f6b2a3964362d2f417f17e3a33e1bb9388d0/src/blob/handlers/BlockBlobHandler.ts#L266-L274

Have you found a mitigation/solution?

There's no solution.

radekg commented 3 months ago

This is how I'd Implement it: https://github.com/Azure/Azurite/compare/main...radekg:Azurite:blockBlobClient-stageBlockFromURL.

Some work would have to be done to extract those functions copied straight from BlobHandler.ts. But the supplied test passes. Would my branch be a good starting point, or am way off regarding how it should be implemented?

radekg commented 3 months ago

The Docker image built from my branch does what it supposed to do. I can put more work in this to have it merged but I'd need guidance. Thank you.

blueww commented 3 months ago

@radekg

Thanks for the contribution! Would you please indicate the detail support you need from us to prepare this PR to implement stageblockfromURL?

BTW, Azurite blob service support 2 kinds of data location (loki and SQL), not sure if you would like also support SQL? If so, have you tested both work? If not, Readme.md should be updated for the support limitation. And test case should not cover sql.

Besides that, you might can follow up rest API doc , and make sure the rest API doc mentioned error / details behavior is aligned with Azurite implementation, and add enough test cases if necessary.
For some details scenarios, sometime we need to test on public Azure server to get the real server behavior and make Azurite aligned.

radekg commented 3 months ago

@blueww I have opened the PR. Regarding the guidance I need:

https://github.com/Azure/Azurite/pull/2442/files#diff-007d907654bdb4b8a5e6ac396dd017e7c93004c1462d5e387d46297bb934f9f6R575-R801, these functions are a carbon copy of the ones from BlobHandler.ts. I had to copy them because they're declared private to the BlobHandler; they could be extracted to a utility and used from both places, if we could simply pass a logger into them (logger is the only thing referencing this in those functions; would that be acceptable?
I can have a look as SQL, but I never used that product so maybe I will have more questions.

Shall we continue the discussion on the PR?

blueww commented 3 months ago

@radekg

Thanks for the contribution!

It's not good to make have duplicated copy of almost same code. It will make the maintenance of the code be difficult. For the common Util function used in several place in blob handler, we put it into this file: https://github.com/Azure/Azurite/blob/main/src/blob/utils/utils.ts Not sure if you have move the function to this file and make both handler use the function in this file?

It's OK if SQL will not be supported at the first version of the API implementation. You just need update Readme.md to add the limitation like : https://github.com/Azure/Azurite#:~:text=Copy%20Blob%20From%20URL%20(Only%20supports%20copy%20within%20same%20Azurite%20instance%2C%20only%20on%20Loki)

radekg commented 3 months ago

Hey @blueww, I have updated the PR. I moved those shared functions to blob/utils/utils.go. Not sure how you'd go about the logger and loggerPrefix arguments, there's never a good way to generalize such code with caller-dependent log messages. I have also updated the readme and tagged the test for @loki only.

Azure / Azurite