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

Metadata Property key always lowercase #8117

Closed
Mansi1 opened this issue Mar 31, 2020 · 32 comments
Closed

Metadata Property key always lowercase #8117

Mansi1 opened this issue Mar 31, 2020 · 32 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved. Storage Storage Service (Queues, Blobs, Files)

Comments

@Mansi1
Copy link

Mansi1 commented Mar 31, 2020

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 xirzec added Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Storage Storage Service (Queues, Blobs, Files) labels Mar 31, 2020
@xirzec
Copy link
Member

xirzec commented Mar 31, 2020

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:

result[header.name.toLowerCase()] = header.value;

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
Copy link
Member

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

@jeremymeng
Copy link
Member

Refer to #4966 for past discussions.

@xirzec
Copy link
Member

xirzec commented Mar 31, 2020

@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
Copy link
Member

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
Copy link
Member

xirzec commented Mar 31, 2020

Quirks.md?

@ramya-rao-a ramya-rao-a added bug This issue requires a change to an existing behavior in the product in order to be resolved. Docs labels Jul 20, 2020
@ramya-rao-a ramya-rao-a added this to the Backlog milestone Jul 20, 2020
@jongio
Copy link
Member

jongio commented Aug 18, 2020

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

@XiaoningLiu XiaoningLiu assigned jiacfan and unassigned XiaoningLiu Aug 20, 2020
@XiaoningLiu
Copy link
Member

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 jiacfan removed the bug This issue requires a change to an existing behavior in the product in order to be resolved. label Aug 20, 2020
@jiacfan
Copy link
Member

jiacfan commented Aug 20, 2020

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
Copy link
Member

xirzec commented Aug 21, 2020

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
Copy link
Member

jiacfan commented Aug 27, 2020

.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
Copy link

pharring commented May 4, 2021

@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
Copy link
Member

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

@jeremymeng jeremymeng reopened this May 4, 2021
@MatejSkrbis
Copy link

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
Copy link
Member

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
Copy link

@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
Copy link
Member

@MRayermannMSFT yes, unfortunately setting metadata has issue too. We are lower-casing the header key in core-http when building up the request:

this._headersMap[getHeaderKey(headerName)] = {

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
Copy link
Member

xirzec commented Oct 22, 2021

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

@jeremymeng
Copy link
Member

@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
Copy link
Member

xirzec commented Oct 22, 2021

@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
Copy link
Member

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:

result[header.name.toLowerCase()] = header.value;

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

@MRayermannMSFT
Copy link

MRayermannMSFT commented Oct 22, 2021

@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
Copy link
Member

@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
Copy link

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

@DadvDadv
Copy link

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

@pharring
Copy link

@DadvDadv yes. See microsoft/AzureStorageExplorer#2573 (comment)

@EmmaZhu
Copy link
Member

EmmaZhu commented May 5, 2022

#15594

@ttaylor29
Copy link

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
Copy link

@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.

Copy link

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.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 22, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2024
@xirzec xirzec removed this from the Backlog milestone May 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

No branches or pull requests