Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.09k stars 1.2k forks source link

Metadata Property key always lowercase #8117

Closed Mansi1 closed 7 months ago

Mansi1 commented 4 years ago

Maybe its not a bug here in the sdk, but wenn i add some metadata in kamelcase and i get it back it will be in lowercase

image

-->

image

xirzec commented 4 years ago

According to this: https://docs.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-containers--blobs--and-metadata#metadata-names

Metadata is supposed to preserve its case. I don't see anything in storage-blob that would make this be an intentional choice.

I think it may be caused by this line: https://github.com/Azure/azure-sdk-for-js/blob/4555a454b5effde0d7e52b39e6b4996861c4d8d1/sdk/core/core-http/src/httpHeaders.ts#L170

It seems odd that rawHeaders mutates the casing of headers. I'm not sure why this was done or if it would break anything to change it.

/cc @daviwil @bterlson @jeremymeng

jeremymeng commented 4 years ago

This is a known issue due to the node-fetch behavior

jeremymeng commented 4 years ago

Refer to https://github.com/Azure/azure-sdk-for-js/issues/4966 for past discussions.

xirzec commented 4 years ago

@jeremymeng I see we added doc comments, but perhaps some additional docs in the README would help? It doesn't seem like we have a section/example on metadata at all.

jeremymeng commented 4 years ago

Yes, I agree more samples would help. I am not sure README is the proper place though as there are too many features/topics in storage blobs to fit there. When storage is in its own repo there is wiki page. In mono-repo wiki page might not scale well.

It would be great to have a place to put FAQ/quirks/etc.

xirzec commented 4 years ago

Quirks.md?

jongio commented 4 years ago

@XiaoningLiu - Do you have an update on this? It is currently 140 days old. Thanks, Jon

XiaoningLiu commented 4 years ago

Thanks! @jongio The limitation is from HTTP clients (both node-fetch or previously axios) in Azure Core. One of steps in decision is to update JS doc about a warning for casing differences for Get Properties API. (see https://github.com/Azure/azure-sdk-for-js/pull/6308/files).

@jiacfan can you help check if further document work needed? Added you per CRI schedule.

jiacfan commented 4 years ago

Thanks for adding me.

In addition to "HTTP client's limitation" mentioned above, this is also related to HTTP's RFC, where header is declared to consists of a case-insensitive field name.

An optimization in service side is that blob tag is introduced, where both tag key&value are saved in case-sensitive pattern which is achieved through using headers' field value to save both tag's key&value, e.g. in PutBlob. Customer may choose to use metadata or tag according to scenarios. (Here is the section on choosing between metadata and blob indexed tag).

@bterlson @jongio @jeremymeng @xirzec to suggest if we need doc this in Readme or an additional .md file, besides the existing doc(https://github.com/Azure/azure-sdk-for-js/pull/6308/files).

xirzec commented 4 years ago

I'm curious how the C# client does it - do they use a case insensitive dictionary type?

I'm fine with the doc comments as they are today and don't feel a strong need to do more at the moment from that angle.

jiacfan commented 4 years ago

.Net also has case insensitive support for metadata, we can find test code here and case insensitive dictionary comparison logic here. Close the issue as the API doc is clear, and we can further optimize if there is further customer requirements.

pharring commented 3 years ago

@xirzec @jiacfan I know this is an old issue (and I don't have permissions to reactivate it), but I just want to express how frustrating this issue is for me and, I suspect other users of Azure Storage Explorer which, as you know, uses this SDK.

I work on an Azure service that uses the Storage SDK for .NET. The service writes blob metadata using mixed-case keys. For the metadata names, we use common two letter prefixes (e.g. "ds", "sp") followed by Pascal-cased symbols. e.g. "spTraceFormat". I often find myself investigating issues where I use Azure Storage Explorer to check the metadata on these blobs. I find it frustrating that the Storage Explorer displays the metadata names all in lower-case when I know that my .NET code wrote them using a specific case and the casing can round trip. It also means, if I choose to edit the metadata using Storage Explorer, then casing is lost.

I totally understand that one shouldn't rely on the casing of metadata. Yes, I get it: Lookups should be OrdinalIgnoreCase. But the displaying of metadata should preserve the casing that was returned from the storage service.

When I worked on the Visual Studio IDE, we had an issue where the Solution Explorer did not preserve the casing of files on disk -- e.g. you renamed one of your source files, changing only the casing, but Solution Explorer didn't update. Of course, it was a usability bug and we fixed it.

Imagine if Windows Explorer one day decided to list all your file and directory names in lower-case! Of course the Windows file systems (NTFS/DOS) doesn't care about casing, but users do. That would also be a bug.

Azure Storage Explorer is the odd one out. Apparently, it can't be fixed there because the casing is lost in the underlying Storage SDK for JS.

The Azure Portal preserves the casing and so does the .NET SDK, so the fault is not with the underlying REST API. This appears to be squarely an "SDK for JS" issue.

jeremymeng commented 3 years ago

It is due to the behavior of our underlying http client dependencies. It looks that with core v2 moving to use NodeJS built-in https module, we should be able to preserve the casing. However, there isn't an ETA yet on when Storage will finish moving to core v2.

image

pharring commented 3 years ago

Hi, @jeremymeng (long time!) Thanks for re-opening this for consideration. Much appreciated.

EmmaZhu commented 3 years ago

Hi @pharring

Which interface do you use to get blob's metadata? Do you use the listing method, or the GetProperties method?

Thanks Emma

MatejSkrbis commented 3 years ago

I'm also using GetProperties to get metadata.

With azure-storage it was possible to set and retrieve upper-case words. https://www.npmjs.com/package/azure-storage

Here we need to search for workarounds to use this functionality.

jeremymeng commented 3 years ago

I have a workaround that does not need changes in the core library. For *Client.getProperties() we can use a custom http client to retrieve the metadata while preserving their casing.

  const containerClient = serviceClient.getContainerClient(containerName);
  const properties1 = await containerClient.getProperties();
  console.log("metadata retrieved:");
  console.log(properties1.metadata);

  const getPropertiesClient = new ContainerClient(
    containerClient.url,
    sharedKeyCredential,
    {
      httpClient: new ExampleHttpClient(),
    }
  );
  const properties2 = await getPropertiesClient.getProperties();
  console.log("x-ms-meta retrieved with custom http client:");
  console.log((properties2._response as any).xMsMeta);

Output:

metadata retrieved:
{ _: 'underscore', camelcase: 'camelCaseValue', v: 'value' }
x-ms-meta retrieved with custom http client:
{ camelCase: 'camelCaseValue', v: 'value', _: 'underscore' }

Complete code: https://gist.github.com/jeremymeng/36c8162b65fe8945c8dae59061d9ce1d

MRayermannMSFT commented 3 years ago

@jeremymeng what about for setting properties? I think we might be able to pull off the get scenario, and for setting, the networking stack we are using will respect the header casing. But, when setting, my http client gets headers which are lowercased. Any thoughts?

jeremymeng commented 3 years ago

@MRayermannMSFT yes, unfortunately setting metadata has issue too. We are lower-casing the header key in core-http when building up the request: https://github.com/Azure/azure-sdk-for-js/blob/1c04b202f507af6297d00318aa06f9663f15108e/sdk/core/core-http/src/httpHeaders.ts#L140

The metadata can be set via setMetadata() call, or when uploading/copying blobs, thus it would be more involved to use a custom http client even if we expose the original raw headers in the request.

I will give it a try to see the impact of keeping the original header key casings. /cc @xirzec @deyaaeldeen

xirzec commented 3 years ago

@jeremymeng Perhaps this is another good time to ask about the timeline of migrating Storage to the new core? 😄

jeremymeng commented 3 years ago

@jeremymeng Perhaps this is another good time to ask about the timeline of migrating Storage to the new core? 😄

@xirzec we normalize the header name too in core-rest-pipeline...

xirzec commented 3 years ago

@xirzec we normalize the header name too in core-rest-pipeline...

Oh no, you're right! I wonder if we changed the implementation to preserve the first casing we see (while still doing comparisons correctly) if that would break anyone.

jeremymeng commented 3 years ago

in core-http we store the original header name. However I don't understand why we would use lowercased key when returning the raw headers: https://github.com/Azure/azure-sdk-for-js/blob/d79c90f49312851d798de610d3c0eb481b66e25c/sdk/core/core-http/src/httpHeaders.ts#L181

I will make a draft PR and see how many tests are failing.

MRayermannMSFT commented 3 years ago

@jeremymeng

The metadata can be set via setMetadata() call, or when uploading/copying blobs, thus it would be more involved to use a custom http client even if we expose the original raw headers in the request.

We aren't doing any metadata setting during upload or copy with the JS SDKs, so no worries on the complexity there. Just need it for setMetadata().

jeremymeng commented 3 years ago

@MRayermannMSFT after fixing an issue in core-http with #18348, I was able to update my custom http client to use the original header names. setMetadata() then worked as expected keeping the casing. https://gist.github.com/jeremymeng/36c8162b65fe8945c8dae59061d9ce1d#file-examplehttpclient-ts-L83

https://gist.github.com/jeremymeng/36c8162b65fe8945c8dae59061d9ce1d#file-examplehttpclient-ts-L170

MRayermannMSFT commented 3 years ago

@jeremymeng cool, looks like we can take advantage of that. Just waiting on Electron to ok adding raw headers on incoming http messages!

DadvDadv commented 2 years ago

Any news on this ? 2 years now and this affect Azure Storage Explorer dramaticaly

pharring commented 2 years ago

@DadvDadv yes. See https://github.com/microsoft/AzureStorageExplorer/issues/2573#issuecomment-1097402892

EmmaZhu commented 2 years ago

https://github.com/Azure/azure-sdk-for-js/issues/15594

ttaylor29 commented 2 years ago

Hello,

I'm seeing this issue and I'm making sure I understand it properly.

I have uploaded a lot of files via C# SDK into our Azure Blob Storage.

Here is the code I'm using to obtain the Metadata properties on a blob:

BlobProperties properties = await blobItem.GetPropertiesAsync().ConfigureAwait(false);

Our tester reported an issue with one file that it would not download. I realized this is a file I've managed directly via Azure Storage Explorer. I then realized the metadata key is coming back ask 'moduleid', when we saved it as 'moduleId'. We are expecting it to come back as 'moduleId' (upper case I), so code on up the chain will find it in our metadata list etc.

After reading this GitHub Issue, it seems like there is a setting on my desktop's Azure Storage Explorer app that is causing this?

Is this correct?

MRayermannMSFT commented 2 years ago

@ttaylor29 I would recommend not having your code be case sensitive with regards to checking metadata keys. Given that metadata are simply HTTP headers, there really is nothing in the HTTP spec that is ensuring you will get the same casing you used originally.

After reading this GitHub Issue, it seems like there is a setting on my desktop's Azure Storage Explorer app that is causing this?

No. More like: there is now a setting you can enable that will help prevent issues, but things still aren't perfect because AzCopy still does not maintain HTTP header casing.

github-actions[bot] commented 7 months ago

Hi @Mansi1, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.