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

Added JSON Arrays Support #111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alfkonee
Copy link

Added Top Level JSON Arrays Support

@Choc13
Copy link
Collaborator

Choc13 commented Jul 16, 2020

Hi @alfkonee thanks for opening this. Could you give me some background and maybe some examples that highlight why this feature is needed? Just to help me put these changes in to context so I can better review them.

@alfkonee
Copy link
Author

This feature will allow you to use a consul Folder as the root for your config for Example with this tree.

- myApp/
    - data/
        [
          {
              Key: Value
          }
        ]
    - strings/
        [
            "string1"
            "string2"
        ]
    - auth/
        {
            "appId": "guid",
            "claims": [
                "email",
                "name"
            ]
        }
    - logging/
        {
            "level": "warn"
        }

In a Scenario like this you can add the whole tree by calling

var configuration = builder
    .AddConsul("myApp", cancellationToken)
    .Build();

The resultant configuration would contain array sections for data and strings. As a concrete example configuration.GetValue<string>("data:0:Key") would return "Value" and configuration.GetValue<string>("strings:1") would return "string2".

Hope this outlines the use cases for this.

@alfkonee
Copy link
Author

I've also made and API Proposal to the dotnet runtime dotnet/runtime#39429 to make this support or native Json not just Consul

@alfkonee
Copy link
Author

alfkonee commented Jul 20, 2020

Hello @Choc13 Please any Update ON this??

@Choc13
Copy link
Collaborator

Choc13 commented Jul 20, 2020

Hey, sorry I've been busy. My current thoughts on this are that it should really live upstream in the JsonParser. Partly because it's quite a lot duplicate functionality to include in here just to support a slight alterations to the way Json parsing works. Also, until recently we had this duplication in the library because the parsing in the JsonConfigurationSource wasn't usable outside of their library. However with the addition of the JsonStreamParser we were able to use that directly.

I am sympathetic to your requirements here and would like to do what I can to help. However I have to weigh up the increase in complexity for the gain in functionality. So I propose we do the following for now:

  1. Wait and see if your upstream changes are accepted and then just update the dependency here
  2. If they're rejected and depending on why we can see what other options are available.

For now you should be able to write this parser in your code base and override the default parser in the settings to point to your implementation that uses this code.

It would be great if you could link the upstream change request here so I can take a look at that too.

Thanks. Hi

@alfkonee
Copy link
Author

alfkonee commented Jul 20, 2020

Hello @Choc13 I agree that this implementation should live upstream in the DOTNET config Json Parser; as seen here dotnet/runtime#39429.
I have created the API Proposal that will make it possible for this here pending API proposal acceptance after which I'll create the Implementation that will enable it Upstream.
I made this PR with the aim that we can control our own parser here which can allow some extra features like a universal Parser for both scalar Values and Json Files. This is another feature I am planning to add to this Library using this inbuild Parser

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