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

Add yaml support #381

Open
carlin-q-scott opened this issue Feb 10, 2023 · 11 comments
Open

Add yaml support #381

carlin-q-scott opened this issue Feb 10, 2023 · 11 comments

Comments

@carlin-q-scott
Copy link

I'd like to be able to store application/yaml content in my app config keys and have this library deserialize it.

I found the json key value adaptor in src/Microsoft.Extensions.Configuration.AzureAppConfiguration/JsonKeyValueAdapter.cs. I don't think it would take much work to implement that for yaml. But it would add a dependency on another nuget package.

@amerjusupovic
Copy link
Member

Hi @carlin-q-scott , as of now we recommend using the application/json content type and converting yaml data to json format when necessary. Please let us know if you have any questions or issues you feel aren't addressed by this approach.

@goldsam
Copy link

goldsam commented Jun 10, 2024

The issue is having to convert back and forth between JSON and YAML. azure App Configuration is targeting administrators who enjoy simplicity. YAML is simplicity - there's a reason it's achieved nearly ubiquitous adoption. Sorry to sound snarky, but I feel like this was self-evident in the OP's request and the response seemed very dismissive.

@amerjusupovic
Copy link
Member

I never intended to be dismissive of the original question, so I'm sorry if I came across that way. From what I understand, yaml can be converted to json fairly easily so I thought it wouldn't introduce a large hurdle for anyone who wants to parse a yaml file. I wanted to know if anyone's aware of an issue with this, or a use case that I might not have considered, because then I would appreciate them bringing up any issues with my original understanding.

I agree that having YAML parsing could make things a bit easier for some users. The nuget package for parsing yaml that was mentioned does mean we have to add a new dependency, which we usually try to avoid. We didn't think there was a strong enough reason to add this capability, so we recommended the approach that I mentioned. We're of course open to reconsidering this if it's something that people feel strongly about.

@goldsam
Copy link

goldsam commented Jun 10, 2024

@amerjusupovic You could instead make IKeyValueAdapter a part of the public API to allow parsing extensions. A separate package such as Microsoft.Extensions.Configuration.AzureAppConfiguration.Yaml could be provided to add the request YAML support.

@fvidesf
Copy link

fvidesf commented Oct 23, 2024

@amerjusupovic You could instead make IKeyValueAdapter a part of the public API to allow parsing extensions. A separate package such as Microsoft.Extensions.Configuration.AzureAppConfiguration.Yaml could be provided to add the request YAML support.

And here I was thinking we could do this on my project (implementing our own IKeyValueAdapters). Are there any plans for this? or any workarounds

@zhenlan
Copy link
Contributor

zhenlan commented Nov 9, 2024

@amerjusupovic can this be done by user code through the Map API?

@amerjusupovic
Copy link
Member

@zhenlan I had a quick discussion with @samsadsam about this, and the only issue we discovered is that IKeyValueAdapter can return an IEnumerable<KeyValuePair<string, string>>, which means a json/yaml object can be flattened and add multiple keys to the configuration like in the JsonKeyValueAdapter with the JsonFlattener class. Because Map just returns a ConfigurationSetting, it doesn't seem to be the same functionality. Map could just aid in converting the yaml key-values to json before being processed at the moment, which doesn't match what's being requested in this issue.

Making IKeyValueAdapter public would require some changes since currently it has functionalities that weren't intended to be public (I think only ProcessKeyValue and CanProcess could be public).

@zhenlan
Copy link
Contributor

zhenlan commented Nov 11, 2024

Thanks @amerjusupovic.

Because Map just returns a ConfigurationSetting

Will it work if we change the Map API to return enumerable of ConfigurationSetting?

Map could just aid in converting the yaml key-values to json before being processed at the moment, which doesn't match what's being requested in this issue.

Why not? It sounds like a reasonable workaround for user code to handle YAML. Even if the Map API worked, the user code would still need to deal with YAML.

@amerjusupovic
Copy link
Member

Will it work if we change the Map API to return enumerable of ConfigurationSetting?

I think that's possible, that would mean you wouldn't have to convert to json, but you'd have to parse yaml and flatten it within a call to Map still.

Why not? It sounds like a reasonable workaround for user code to handle YAML. Even if the Map API worked, the user code would still need to deal with YAML.

I didn't mention Map API as part of the solution with converting to json originally, so that's a fair point. I would think it's less work to implement for users too. It still requires some work to implement a custom IKeyValueAdapter for yaml if the class was public.

@amerjusupovic
Copy link
Member

@fvidesf To clarify, the workaround is to use the Map API to convert the any ConfigurationSetting with a yaml value to a json string, and set the content type of the setting to "application/json" to allow the provider's json adapter to process and add all values from the yaml to the configuration. You can denote which values are yaml using your own custom content type (I chose application/yaml).

It would look something like this, where options is your AzureAppConfigurationOptions:

using YamlDotNet.Serialization;
using System.Text.Json;

// other code

options.Map(setting =>
{
    if (setting.ContentType == "application/yaml")
    {
        var deserializer = new DeserializerBuilder().Build();

        var yamlObject = deserializer.Deserialize<object>(setting.Value);

        string newSettingJsonValue = JsonSerializer.Serialize(yamlObject);

        ConfigurationSetting newSetting = new ConfigurationSetting(setting.Key, newSettingJsonValue, setting.Label, setting.ETag);

        newSetting.ContentType = "application/json";

        return new ValueTask<ConfigurationSetting>(newSetting);
    }

    return new ValueTask<ConfigurationSetting>(setting);
});

@goldsam
Copy link

goldsam commented Nov 14, 2024

So the suggestion is to parse and map YAML to JSON only for that JSON to be parsed yet again to key-value pairs? That seems like a very ugly hack. The better solution would be a proper public API to define the policy for mapping configuration content to configuration key/value pairs such as IKeyValueAdapter (albeit with some modifications).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

5 participants