feat(parameters): added `BaseProvider` class #1168

dreamorosi · 2022-11-16T00:46:09Z

Description of your changes

This PR introduces a new package (parameters) in the npm workspace as well as the BaseProvider for the new utility and its unit tests.

The changes related to the new workspace package include:

New pacakges/parameters folder with package.json
TSConfig files
Eslint file
Jest test harness, and jestconfig file
README file

The changes related to the BaseProvider include the content of the src and tests directories:

New BaseProvider class
New types and exceptions to support the class
Unit tests for BaseProvider

Note
I have left a few comments/questions that I would appreciate reviewers to engage with during the review

Please don't take in account the content of the README.md file nor the docstrings (or lack thereof) for the various features, both items will be addressed in their own PRs.

How to verify this change

See unit tests added as part of this PR.

Related issues, RFCs

Issue number: #1173

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

Breaking change checklist

I have documented the migration process
I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

dreamorosi · 2022-11-17T16:42:53Z

packages/parameters/src/BaseProvider.ts

+}
+
+abstract class BaseProvider implements BaseProviderInterface {
+  public store: Map<string, ExpirableValue> = new Map;


Question: Should this be public or private?

I'll go with private first and broaden later if needed.

If I use private I'm not sure how to test around it. So I went with private and extended the class in the tests, similar to what done in the Idempotency utility.

If you have any better way to do it that also allows a pure private let me know.

That should be good for now. I would expect that the tests cover all privates variable from the public methods.

dreamorosi · 2022-11-17T16:49:33Z

packages/parameters/src/BaseProvider.ts

+   */
+  public async get(name: string, options?: GetOptionsInterface): Promise<undefined | string | Record<string, unknown>> {
+    const configs = new GetOptions(options);
+    const key = [ name, configs.transform ].toString();


I couldn't think of a more elegant way of constructing this key.

For context:

Python uses a Tuple, which are not supported in JS (only stage 2)

I was initially using objects like { name: string, transform: TransformOptions } since we are using a Map, but given that Map.has and Map.get use object identity to compare the key it would never return a match - also it's not possible to extend/customize the equality function used by a Map

Set have the same limitations described above and that apply to Map

I briefly considered using external modules like immutable-js or custom implementations of Set, but opted not to in order to avoid introducing a dependency over this otherwise limited usage.

As long as the test cases are rigorous enough, this should be ok. I cannot see how we can run into having a duplicated here, can we?

Agree.

I wasn't worried about duplicates as much as special characters and such.

dreamorosi · 2022-11-17T16:57:26Z

packages/parameters/src/BaseProvider.ts

+    }
+
+    // TODO: revisit return type once providers are implemented, it might be missing binary when not transformed
+    return value;


As mentioned in the comment here, I stayed generic with types because at this stage I'm not yet sure what each provider/AWS SDK (from _get) will return.

I have opened #1172 to track this work.

dreamorosi · 2022-11-17T16:57:54Z

packages/parameters/src/BaseProvider.ts

+      this.addToCache(key, values, configs.maxAge);
+    }
+
+    // TODO: revisit return type once providers are implemented, it might be missing something


I have opened #1172 to track this work.

dreamorosi · 2022-11-17T16:58:20Z

packages/parameters/src/BaseProvider.ts

+
+}
+
+// TODO: revisit `value` type once we are clearer on the types returned by the various SDKs


I have opened #1172 to track this work.

dreamorosi · 2022-11-17T16:59:03Z

packages/parameters/src/BaseProvider.ts

+    ) {
+      return new TextDecoder('utf-8').decode(fromBase64(value));
+    } else {
+      // TODO: revisit this type once we are clearer on types returned by SDKs


I have opened #1172 to track this work.

dreamorosi · 2022-11-17T17:00:08Z

packages/parameters/src/BaseProvider.ts

+  }
+}
+
+class GetMultipleOptions implements GetMultipleOptionsInterface {


Should these classes be in separate files?

Given this file's size, I would say so. But it is not critical.

Addressed in f9259eb

dreamorosi · 2022-11-17T17:02:17Z

packages/parameters/src/BaseProvider.ts

+    const value = this.store.get(key);
+    if (value) {
+      if (value.isExpired()) {
+        this.store.delete(key);


As far as I can tell, the Python implementation doesn't clear expired items from the cache.

Can you think of any case in which doing that might become an issue?

No. We just trade time for space here. The operation is O(1) so I don't think there is any issue to delete it now.

The problem is more on doing more than one thing in a single method. It is not obvious to me that hasKeyExpiredInCache() will prune the this.store. The new maintainer may use this function and didn't know that he's pruned the cache. Thus, I'm incline to remove this.

If we want to prune expired items, I would expect a public/protected method to do this explicitly. But that will cause O(n) and I am not sure if it's useful. Map has O(1) access so we are only saving the memory consumption here, which I expect it to be very small gain.

Thus, I'm incline to remove this and keep the expired item.

That said. It's open to debate. I don't think that it's bad enough to block this PR. So I would give an approval. You can decide in later commit if you want to remove this.

Good points, I didn't think about the operation costs and I agree that the memory savings are marginal.

I have removed it.

ijemmy

One comment on prunning expire key

ijemmy · 2022-11-18T13:39:37Z

packages/parameters/src/BaseProvider.ts

+}
+
+abstract class BaseProvider implements BaseProviderInterface {
+  public store: Map<string, ExpirableValue> = new Map;


I'll go with private first and broaden later if needed.

packages/parameters/src/BaseProvider.ts

ijemmy · 2022-11-18T13:46:29Z

packages/parameters/src/BaseProvider.ts

+  }
+}
+
+class GetMultipleOptions implements GetMultipleOptionsInterface {


Given this file's size, I would say so. But it is not critical.

ijemmy · 2022-11-18T13:50:00Z

packages/parameters/src/BaseProvider.ts

+   */
+  public async get(name: string, options?: GetOptionsInterface): Promise<undefined | string | Record<string, unknown>> {
+    const configs = new GetOptions(options);
+    const key = [ name, configs.transform ].toString();


As long as the test cases are rigorous enough, this should be ok. I cannot see how we can run into having a duplicated here, can we?

ijemmy · 2022-11-18T13:52:53Z

packages/parameters/src/BaseProvider.ts

+    const value = this.store.get(key);
+    if (value) {
+      if (value.isExpired()) {
+        this.store.delete(key);


No. We just trade time for space here. The operation is O(1) so I don't think there is any issue to delete it now.

The problem is more on doing more than one thing in a single method. It is not obvious to me that hasKeyExpiredInCache() will prune the this.store. The new maintainer may use this function and didn't know that he's pruned the cache. Thus, I'm incline to remove this.

If we want to prune expired items, I would expect a public/protected method to do this explicitly. But that will cause O(n) and I am not sure if it's useful. Map has O(1) access so we are only saving the memory consumption here, which I expect it to be very small gain.

Thus, I'm incline to remove this and keep the expired item.

ijemmy · 2022-11-18T13:59:12Z

packages/parameters/src/BaseProvider.ts

+    const value = this.store.get(key);
+    if (value) {
+      if (value.isExpired()) {
+        this.store.delete(key);


That said. It's open to debate. I don't think that it's bad enough to block this PR. So I would give an approval. You can decide in later commit if you want to remove this.

shdq · 2022-12-16T12:50:32Z

packages/parameters/src/types/BaseProvider.ts

+interface GetMultipleOptionsInterface {
+  maxAge?: number
+  forceFetch?: boolean
+  sdkOptions?: unknown
+  transform?: string
+  throwOnTransformError?: boolean
+}


Just to reduce boilerplate:

interface GetMultipleOptionsInterface extends GetOptionsInterface { throwOnTransformError?: boolean }

Good point, please feel free to address this in your PR for AppConfigprovider

pull-request-size bot added the size/L PRs between 100-499 LOC label Nov 16, 2022

github-actions bot added the feature label Nov 16, 2022

dreamorosi added feature PRs that introduce new features or minor changes and removed feature labels Nov 16, 2022

dreamorosi self-assigned this Nov 16, 2022

pull-request-size bot added size/XL PRs between 500-999 LOC, often PRs that grown with feedback and removed size/L PRs between 100-499 LOC labels Nov 17, 2022

dreamorosi commented Nov 17, 2022

View reviewed changes

dreamorosi linked an issue Nov 17, 2022 that may be closed by this pull request

Feature request: implement BaseProvider #1173

Closed

4 tasks

dreamorosi changed the title ~~feat(parameters): added BaseProvider class~~ feat(parameters): added BaseProvider class Nov 17, 2022

github-actions bot added the feature label Nov 17, 2022

dreamorosi marked this pull request as ready for review November 17, 2022 17:43

dreamorosi requested review from flochaz and ijemmy November 17, 2022 17:43

dreamorosi added parameters This item relates to the Parameters Utility and removed feature labels Nov 17, 2022

ijemmy previously approved these changes Nov 18, 2022

View reviewed changes

dreamorosi added 6 commits November 19, 2022 13:14

feat: added baseprovider class

a2de862

tests: added unit tests

e52c416

chore: added package to unit tests run

bc0a5f9

chore: added comments

de13951

chore: break down classes in files

f9259eb

chore: remove redundant store init value

2a294ef

dreamorosi added 3 commits November 19, 2022 13:23

refactor: moved constants in separate file

734c98f

chore: made store protected

82fa784

fix: removed cache pruning

3ef5990

dreamorosi dismissed ijemmy’s stale review via 128ed70 November 19, 2022 12:55

pull-request-size bot added size/XXL PRs with 1K+ LOC, largely documentation related and removed size/XL PRs between 500-999 LOC, often PRs that grown with feedback labels Nov 19, 2022

feat: added baseprovider class

14f8b81

dreamorosi force-pushed the feat/parameters/base branch from 128ed70 to 14f8b81 Compare November 19, 2022 12:58

pull-request-size bot added size/XL PRs between 500-999 LOC, often PRs that grown with feedback and removed size/XXL PRs with 1K+ LOC, largely documentation related labels Nov 19, 2022

dreamorosi requested a review from ijemmy November 19, 2022 12:59

ijemmy approved these changes Nov 21, 2022

View reviewed changes

dreamorosi merged commit d717a26 into main Nov 21, 2022

dreamorosi deleted the feat/parameters/base branch November 21, 2022 10:07

shdq mentioned this pull request Dec 14, 2022

Feature request: implement AppConfigProvider #1177

Closed

4 tasks

shdq reviewed Dec 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(parameters): added `BaseProvider` class #1168

feat(parameters): added `BaseProvider` class #1168

dreamorosi commented Nov 16, 2022 •

edited

Loading

dreamorosi Nov 17, 2022

ijemmy Nov 18, 2022

dreamorosi Nov 19, 2022

ijemmy Nov 21, 2022

dreamorosi Nov 17, 2022

ijemmy Nov 18, 2022

dreamorosi Nov 19, 2022

dreamorosi Nov 17, 2022

dreamorosi Nov 17, 2022

dreamorosi Nov 17, 2022

dreamorosi Nov 17, 2022

dreamorosi Nov 17, 2022

ijemmy Nov 18, 2022

dreamorosi Nov 19, 2022

dreamorosi Nov 17, 2022

ijemmy Nov 18, 2022

ijemmy Nov 18, 2022 •

edited

Loading

dreamorosi Nov 19, 2022

ijemmy left a comment

ijemmy Nov 18, 2022

ijemmy Nov 18, 2022

ijemmy Nov 18, 2022

ijemmy Nov 18, 2022

ijemmy Nov 18, 2022 •

edited

Loading

shdq Dec 16, 2022

dreamorosi Dec 16, 2022


		}

		// TODO: revisit `value` type once we are clearer on the types returned by the various SDKs

feat(parameters): added BaseProvider class #1168

feat(parameters): added BaseProvider class #1168

Conversation

dreamorosi commented Nov 16, 2022 • edited Loading

Description of your changes

How to verify this change

Related issues, RFCs

PR status

Checklist

Breaking change checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijemmy Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijemmy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijemmy Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(parameters): added `BaseProvider` class #1168

feat(parameters): added `BaseProvider` class #1168

dreamorosi commented Nov 16, 2022 •

edited

Loading

ijemmy Nov 18, 2022 •

edited

Loading

ijemmy Nov 18, 2022 •

edited

Loading