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

Refactor AzureAppConfiguration with new interfaces #49

Merged
merged 8 commits into from
Mar 18, 2024

Conversation

Eskibear
Copy link
Member

@Eskibear Eskibear commented Feb 6, 2024

Design

We support both two approaches to access key-values, maximize compatibility with customer's application code. For easier adoption of Azure App Configuration, and less code change.

AzureAppConfiguration {
  get<T>(key: string): T | undefined;  // preferred method, e.g. config.get("app.settings.color")
    constructConfigurationObject(options);    // helper function to construct hierarchical object, compatible with chained dot notation, e.g. config.app.settings.color
}

Edge case (for using configuration object only):

Hierarchical key-value pairs with overlapped key prefix:
  key: "app3.settings" => value: "placeholder"
  key: "app3.settings.fontColor" => value: "yellow"

How to re-construct data object from key-values?
config.app3.settings = ? 

In this case, constructConfigurationObject() throws an error.

Mitigation approach

Add an option skipObjectValidataion: boolean, default to false.
- true. Throw an error when constructing the data object., because of ambiguity.
- false. Skip the validation, allow information lost in data object, as the preferred get() method can always have the expected results.
(propose to use constructConfigurationObject(options) as below, which would not affect the original load() call)

@Eskibear
Copy link
Member Author

Eskibear commented Feb 7, 2024

Offline synced with Zhenlan, two improvements

  • AppConfig impl Map interfaces, and throw exception in set(k, v) to enforce immutability.

  • Change data object to a helper function, e.g constructConfigurationObject(options), which constructs an object using KVs from the configMap. The options can include:

    • customized separator, if necessary. default is . but might be : if the config is shared with other languages.
    • - how to deal with ambiguity mentioned in above edge case, silently continue or throw exception.
    • - customized prefix, default to empty string, meaning to convert the whole map. It's useful if a only configuration section is wanted.

Updates from design review:

  • Remove prefix as we can get subset from the root object if necessary, the performance overhead is expected to be little.
  • The "behavior" should be always throwing error. Because silent fail doesn't ensure expected results. Customers should fix error in AppConfig store to get their app back to run.

@rossgrambo
Copy link

rossgrambo commented Feb 9, 2024

AppConfig impl Map interfaces

I don't think there is a Map interface. Javascript added Map, and Javascript doesn't have interfaces. Looks like Typescript didn't add an interface for it as far as I can tell.

Nvm- zhiyuan found it

@zhiyuanliang-ms
Copy link
Contributor

zhiyuanliang-ms commented Feb 9, 2024

For the edge case:

Edge case (for data property only):
Hierarchical key-value pairs with overlapped key prefix:
key: "app3.settings" => value: "placeholder"
key: "app3.settings.fontColor" => value: "yellow"

How to re-construct data object from key-values?
config.data.app3.settings = ?

Since the default shape is map and user needs to call constructDataObject(options) to use data object, is this a valid case?
If the user sets two key "app3.settings" and "app3.settings.fontColor", will he really use the data object instead of map to get configuration?

If the user wants to use data object, he should set the value as json shape and set the content type as json.

@zhiyuanliang-ms
Copy link
Contributor

zhiyuanliang-ms commented Feb 9, 2024

@rossgrambo There is Map interface in TypeScript.

My question is why to use a custom IGettable interface and throw exception when set is called. Currently, we are using ReadOnlyMap interface, this makes more sense to me.

Now I see the reason why we implement a custom IGettable as Get<T> will convert the value to the T type.

@Eskibear Eskibear force-pushed the yanzh/support-both-map-object branch from 4681bbf to 819c5fb Compare February 23, 2024 03:33
@Eskibear Eskibear changed the title [WIP] Refactor AzureAppConfiguration with new interfaces Refactor AzureAppConfiguration with new interfaces Mar 1, 2024
@Eskibear Eskibear marked this pull request as ready for review March 1, 2024 08:56
@Eskibear
Copy link
Member Author

Eskibear commented Mar 6, 2024

As the design is finalized, this PR is ready for review now. Please take a look.

Co-authored-by: Zhiyuan Liang <[email protected]>
src/AzureAppConfiguration.ts Show resolved Hide resolved
src/AzureAppConfigurationImpl.ts Show resolved Hide resolved
src/AzureAppConfigurationImpl.ts Show resolved Hide resolved
@Eskibear Eskibear force-pushed the yanzh/support-both-map-object branch from 91528d6 to f4953da Compare March 15, 2024 01:07
@Eskibear Eskibear merged commit 28a5c69 into main Mar 18, 2024
3 checks passed
@Eskibear Eskibear deleted the yanzh/support-both-map-object branch March 18, 2024 04:28
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.

None yet

5 participants