Fix: Resolve Type Warnings for ConfigService.get() #3350

belloibrahv · 2024-12-21T02:42:19Z

Description:
This PR addresses type inconsistencies in the test specifications related to the ConfigService.get(<envName>) method. The changes ensure type safety and consistency across all affected test cases by aligning with the expected types defined in the ConfigService.get() method.

Key Changes:

Created a new file named envName.ts to define and manage environment variable types.
Refactored test cases to use the envName file, ensuring type consistency with ConfigService.get().
Removed unnecessary type casting where applicable and added necessary casting to eliminate TypeScript warnings and errors.
Improved overall type safety in the codebase.

Related issue(s):
Fixes #3142

Notes for Reviewer:

Type mismatches were prevalent in various test cases, leading to TypeScript warnings/errors. This PR systematically addresses those issues by aligning the test code with the ConfigService.get() method's expected types.
Refactoring also included ensuring that variable types are explicitly defined and consistent across the codebase.
Suggestions from the issue discussion (e.g., leveraging a ConfigName enum) were incorporated where feasible.

Checklist:

Documented: Added/updated relevant code comments and documentation.
Tested: Verified changes with updated and new unit tests to ensure correctness.

- Ensured type consistency in all affected test cases. - Removed unnecessary type casting or added necessary casting. Signed-off-by: belloibrahv <[email protected]>

belloibrahv · 2024-12-21T03:04:43Z

@Nana-EC @ebadiere @quiet-node I've submitted a PR addressing issue #3142 to resolve type warnings for ConfigService.get(). It ensures type consistency across test cases by refining type usage and adding/removing the necessary casting. Let me know if further changes are needed. Thank you for reviewing!

acuarica · 2024-12-21T03:49:23Z

packages/config-service/src/services/globalConfig.ts

@@ -26,7 +28,7 @@ export interface ConfigProperty {
 }

 export class GlobalConfig {
-  public static readonly ENTRIES: Record<string, ConfigProperty> = {
+  public static readonly ENTRIES: Record<ConfigName, ConfigProperty> = {


Instead of enumerating the keys manually, can we do something like

const _CONFIG = { BATCH_REQUESTS_ENABLED: { envName: 'BATCH_REQUESTS_ENABLED', type: 'boolean', required: false, defaultValue: null, }, //... WS_SUBSCRIPTION_LIMIT: { envName: 'WS_SUBSCRIPTION_LIMIT', type: 'number', required: false, defaultValue: null, }, } satisfies { [key: string]: ConfigProperty }; export type ConfigKey = keyof typeof _CONFIG; export class GlobalConfig { public static readonly ENTRIES: Record<ConfigKey, ConfigProperty> = _CONFIG; }

?

So we retain type safety for the keys (even if they're strings).

Note that we can use satisfies because we're using TypeScript 4.9.5 as defined in package-lock.json.

Ohh I like the suggestion! Looks cleaner for sure 👏🏼

Nana-EC

Thanks for the contribution.

Nana-EC · 2024-12-21T03:37:44Z

packages/relay/tests/lib/config/hbarSpendingPlanConfigService.spec.ts

 *
 * Hedera JSON RPC Relay
 *
- * Copyright (C) 2022-2024 Hedera Hashgraph, LLC
+ * Copyright (C) 2024 Hedera Hashgraph, LLC


nit: revert

Nana-EC · 2024-12-21T03:41:31Z

packages/server/tests/acceptance/conformityTests.spec.ts

 *
 * Hedera JSON RPC Relay
 *
- * Copyright (C) 2022-2024 Hedera Hashgraph, LLC
+ * Copyright (C) 2024 Hedera Hashgraph, LLC


revert this and any other license header changes where you're changing an old header date into this year.
Might be a bug in an automated script

Nana-EC · 2024-12-21T03:44:18Z

packages/server/tests/acceptance/serverConfig.spec.ts


 describe('@server-config Server Configuration Options Coverage', function () {
  describe('Koa Server Timeout', () => {
    it('should timeout a request after the specified time', async () => {
-      const requestTimeoutMs: number = parseInt(ConfigService.get('SERVER_REQUEST_TIMEOUT_MS') || '3000');
+      const requestTimeoutMs: number = parseInt(ConfigService.get(ConfigName.SERVER_REQUEST_TIMEOUT_MS) as string || '3000');


I wonder if we should add a getString and do the casting in the ConfigService to make this easier on devs.
Thinking out loud and getting thoughts.
No need to change in this PR

Thank you for the suggestion about ConfigService.getString. I agree that it could streamline type handling. I suggest we open a separate issue to implement and test this change comprehensively.

codecov · 2024-12-23T05:07:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.70%. Comparing base (13d09e9) to head (a6285b0).

❗ There is a different number of reports uploaded between BASE (13d09e9) and HEAD (a6285b0). Click for more details.

HEAD has 18 uploads less than BASE

Flag BASE (13d09e9) HEAD (a6285b0)

config-service 1 0

relay 1 0

17 1

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3350       +/-   ##
===========================================
- Coverage   85.04%   49.70%   -35.35%     
===========================================
  Files          65       66        +1     
  Lines        4508     4601       +93     
  Branches     1023     1023               
===========================================
- Hits         3834     2287     -1547     
- Misses        330     1926     +1596     
- Partials      344      388       +44

Flag	Coverage Δ
config-service	`?`
relay	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
packages/config-service/src/services/configName.ts	`100.00% <100.00%> (ø)`
...ckages/config-service/src/services/globalConfig.ts	`100.00% <100.00%> (ø)`

... and 45 files with indirect coverage changes

belloibrahv · 2024-12-25T08:56:34Z

@Nana-EC @acuarica @quiet-node, Could you please review my updates and let me know if further adjustments are needed?

acuarica

Hi @belloibrahv, thanks for taking the time to send this, it's looking good. Left a couple of suggestions we can continue iterating on to improve the config.

acuarica · 2024-12-26T15:50:18Z

packages/config-service/tests/src/services/configService.spec.ts

@@ -49,13 +50,13 @@ describe('ConfigService tests', async function () {
  });

  it('should be able to get existing env var', async () => {
-    const res = ConfigService.get('CHAIN_ID');
+    const res = ConfigService.get('CHAIN_ID' as ConfigKey);


Can we change the definition of ConfigService.get to get(name: ConfigKey)? So we can then avoid all these type assertions, i.e., as ConfigKey.

Not related with your PR, but I tried this change locally and I saw there was a configuration entry missing, SERVER_HOST. If we change the definition of get as mentioned above, we might want add a SERVER_HOST entry.

packages/config-service/tests/src/services/configService.spec.ts

acuarica · 2024-12-26T16:02:06Z

Also @belloibrahv please take a look at #3350 (comment).

quiet-node · 2024-12-26T16:57:22Z

@belloibrahv DCO is failing https://github.com/hashgraph/hedera-json-rpc-relay/pull/3350/checks?check_run_id=34857409075

Signed-off-by: belloibrahv <[email protected]>

belloibrahv · 2024-12-26T23:32:37Z

@quiet-node @acuarica @Nana-EC @shemnon , thank you for the suggestion. I've implemented the following changes based on your feedback:

Updated ConfigService.get to accept name: ConfigKey, removing the need for as ConfigKey assertions.
Added a missing configuration entry for SERVER_HOST in _CONFIG.
Adjusted test cases to align with the updated method definition.

Let me know if there's anything else you'd like me to address!

belloibrahv · 2024-12-26T23:36:46Z

Also @belloibrahv please take a look at #3350 (comment).

@acuarica, Thank you for the suggestion! I think adding a getString (or similar methods like getNumber, getBoolean) in ConfigService could significantly simplify casting and improve developer experience. It could also centralize type handling and reduce repetitive casting logic.

Would you suggest we implement these methods for this task or address it as a follow-up? Also, should these methods throw errors for mismatched types, or return defaults like null? Happy to explore this further and incorporate it if needed!

Nana-EC · 2024-12-27T04:54:23Z

Also @belloibrahv please take a look at #3350 (comment).

@acuarica, Thank you for the suggestion! I think adding a getString (or similar methods like getNumber, getBoolean) in ConfigService could significantly simplify casting and improve developer experience. It could also centralize type handling and reduce repetitive casting logic.

Would you suggest we implement these methods for this task or address it as a follow-up? Also, should these methods throw errors for mismatched types, or return defaults like null? Happy to explore this further and incorporate it if needed!

Let's do it in a separate PR after this.

Also appreciate the patience.
The team is on holiday for the Christmas season so things are a bit slow.

belloibrahv · 2025-01-03T03:02:32Z

Hi @quiet-node, @acuarica, @Nana-EC, @shemnon, Hope you all had a wonderful holiday! When you have a moment, please review and provide feedback on the requested changes to the PR. Thank you for your understanding!

acuarica

Hi @belloibrahv, hope you had a wonderful holiday too! It's looking good, left some comments.

We appreciate your patience on this. Next week the full team will be back from holidays, so you'll get more reviews.

packages/config-service/src/services/index.ts

packages/config-service/tests/src/services/configService.spec.ts

packages/relay/tests/lib/config/hbarSpendingPlanConfigService.spec.ts

packages/relay/tests/lib/eth/eth-config.ts

packages/relay/tests/lib/ethGetBlockBy.spec.ts

packages/ws-server/tests/acceptance/subscribeNewHeads.spec.ts

tools/brownie-example/LICENSE

acuarica · 2025-01-04T01:04:25Z

Another change we could implement to improve type safety, could be to avoid casting the return type of ConfigService.get.

~~Probably better off in another PR, but I wanted to leave the general idea here for discussion.~~ As pointed out by @quiet-node, the scope of the original issue is to type check the return type.

We need some type definitions

type TypeStrToType<Tstr extends string> = Tstr extends 'string'
  ? string
  : Tstr extends 'boolean'
    ? boolean
    : Tstr extends 'number'
      ? number
      : Tstr extends 'array'
        ? unknown[] // could be also `any[]`
        : never;

type GetTypeStrOfKey<K extends string> = K extends keyof typeof _CONFIG ? typeof _CONFIG[K]['type'] : never;
export type TypeOfKey<K extends string> = TypeStrToType<GetTypeStrOfKey<K>>;

These type definitions would allows to get right type based on the config key. For example

Then, we can update our ConfigService.get definition to something like

  public static get<K extends ConfigKey>(name: K): TypeOfKey<K> | undefined

So now the correct return type is inferred based on the config key

Nana-EC · 2025-01-07T06:44:37Z

Hi @quiet-node, @acuarica, @Nana-EC, @shemnon, Hope you all had a wonderful holiday! When you have a moment, please review and provide feedback on the requested changes to the PR. Thank you for your understanding!

Thanks @belloibrahv , hope you had a good one too.
Changes look good and it seems we all agree there's some follow ups post this PR.
@acuarica and @quiet-node will be reviewing this to help you get this through.
Thanks again for the contributions

quiet-node

Great work! Please revert the unnecessary changes and additions. I’ve added a suggestion on the ConfigService.get() method.

packages/config-service/src/commands/printEnvs.ts

packages/config-service/src/services/globalConfig.ts

quiet-node · 2025-01-07T17:55:21Z

packages/config-service/src/services/globalConfig.ts

@@ -26,7 +28,7 @@ export interface ConfigProperty {
 }

 export class GlobalConfig {
-  public static readonly ENTRIES: Record<string, ConfigProperty> = {
+  public static readonly ENTRIES: Record<ConfigName, ConfigProperty> = {


Ohh I like the suggestion! Looks cleaner for sure 👏🏼

packages/config-service/src/services/index.ts

quiet-node · 2025-01-07T18:00:33Z

packages/config-service/src/services/loggerService.ts

@@ -2,7 +2,7 @@
 *
 * Hedera JSON RPC Relay
 *
- * Copyright (C) 2024 Hedera Hashgraph, LLC
+ * Copyright (C) 2022-2024 Hedera Hashgraph, LLC


Let's revert back to original

packages/config-service/tests/src/services/configService.spec.ts

packages/config-service/tests/src/services/loggerService.spec.ts

packages/config-service/tests/src/services/validationService.spec.ts

packages/relay/tests/lib/eth/eth-helpers.ts

quiet-node · 2025-01-07T19:51:58Z

packages/config-service/src/services/index.ts

+  public static get(name: ConfigKey): string | number | boolean | null | undefined {
    return this.getInstance().envs[name];
  }


Hmm, this still doesn’t resolve the primary issue with the .get() method. There are still instances across the codebase where the error “Argument of type ‘string | number | boolean’ is not assignable to parameter of type…” persists, as shown in the example below. This remains an unresolved task that the ticket is meant to address.

Luis adds a very good solution which can fix the issue, please refer to his solution here #3350 (comment)

…asting Signed-off-by: belloibrahv <[email protected]>

sonarqubecloud · 2025-01-08T11:48:08Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.1% Duplication on New Code

See analysis details on SonarQube Cloud

belloibrahv · 2025-01-08T11:50:03Z

Hello @acuarica and @quiet-node, I’ve addressed the recent feedback, incorporating the suggested changes to enhance type safety and eliminate manual casting in ConfigService.get(). Please review the updates at your earliest convenience and let me know if there’s anything else to adjust. Thank you

belloibrahv added 2 commits December 21, 2024 03:20

fix: resolve type warnings for ConfigService.get()

df2f57f

- Ensured type consistency in all affected test cases. - Removed unnecessary type casting or added necessary casting. Signed-off-by: belloibrahv <[email protected]>

fix: resolve type warnings for ConfigService.get()

a6285b0

- Ensured type consistency in all affected test cases. - Removed unnecessary type casting or added necessary casting. Signed-off-by: belloibrahv <[email protected]>

belloibrahv requested review from georgi-l95, Ivo-Yankov, a team and Nana-EC as code owners December 21, 2024 02:42

belloibrahv changed the title ~~fix: resolve type warnings for ConfigService.get()~~ Fix: Resolve Type Warnings for ConfigService.get() Dec 21, 2024

belloibrahv mentioned this pull request Dec 21, 2024

Fix all typescript warnings and errors generated by type mismatching on the ConfigService.get(<envName>) method in test specs #3142

Open

Nana-EC requested review from acuarica, konstantinabl and quiet-node December 21, 2024 03:34

Nana-EC assigned belloibrahv Dec 21, 2024

acuarica reviewed Dec 21, 2024

View reviewed changes

Nana-EC reviewed Dec 21, 2024

View reviewed changes

quiet-node added this to the 0.64.0 milestone Dec 23, 2024

quiet-node added the internal For changes that affect the project's internal workings but not its outward-facing functionality. label Dec 23, 2024

belloibrahv requested review from a team as code owners December 25, 2024 07:55

belloibrahv requested a review from jeromy-cannon December 25, 2024 07:55

belloibrahv force-pushed the 3142-fix-configservice-get-type-warnings branch from 27253f5 to 7f440e4 Compare December 25, 2024 08:54

acuarica requested changes Dec 26, 2024

View reviewed changes

belloibrahv added 3 commits December 26, 2024 23:20

Refactor GlobalConfig and revert header changes as per feedback

0f0377e

Signed-off-by: belloibrahv <[email protected]>

Reverting License Header Changes

f62379c

Signed-off-by: belloibrahv <[email protected]>

Refinment of ConfigService.get() (to conform to newly suggested changes)

c7e6a08

Signed-off-by: belloibrahv <[email protected]>

belloibrahv force-pushed the 3142-fix-configservice-get-type-warnings branch from 7f440e4 to c7e6a08 Compare December 26, 2024 23:23

acuarica reviewed Jan 3, 2025

View reviewed changes

quiet-node requested changes Jan 7, 2025

View reviewed changes

Refactor ConfigService.get to enhance type safety and remove manual c…

7f3c824

…asting Signed-off-by: belloibrahv <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Resolve Type Warnings for ConfigService.get() #3350

Fix: Resolve Type Warnings for ConfigService.get() #3350

belloibrahv commented Dec 21, 2024 •

edited by Nana-EC

Loading

belloibrahv commented Dec 21, 2024

acuarica Dec 21, 2024 •

edited

Loading

quiet-node Jan 7, 2025

Nana-EC left a comment

Nana-EC Dec 21, 2024

Nana-EC Dec 21, 2024

Nana-EC Dec 21, 2024

belloibrahv Dec 24, 2024

codecov bot commented Dec 23, 2024

belloibrahv commented Dec 25, 2024

acuarica left a comment

acuarica Dec 26, 2024

acuarica commented Dec 26, 2024

quiet-node commented Dec 26, 2024

belloibrahv commented Dec 26, 2024

belloibrahv commented Dec 26, 2024

Nana-EC commented Dec 27, 2024

belloibrahv commented Jan 3, 2025

acuarica left a comment •

edited

Loading

acuarica commented Jan 4, 2025 •

edited

Loading

Nana-EC commented Jan 7, 2025

quiet-node left a comment

quiet-node Jan 7, 2025

quiet-node Jan 7, 2025

quiet-node Jan 7, 2025

quiet-node Jan 7, 2025

sonarqubecloud bot commented Jan 8, 2025

belloibrahv commented Jan 8, 2025

Fix: Resolve Type Warnings for ConfigService.get() #3350

Are you sure you want to change the base?

Fix: Resolve Type Warnings for ConfigService.get() #3350

Conversation

belloibrahv commented Dec 21, 2024 • edited by Nana-EC Loading

Key Changes:

belloibrahv commented Dec 21, 2024

acuarica Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nana-EC 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

codecov bot commented Dec 23, 2024

Codecov Report

belloibrahv commented Dec 25, 2024

acuarica left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acuarica commented Dec 26, 2024

quiet-node commented Dec 26, 2024

belloibrahv commented Dec 26, 2024

belloibrahv commented Dec 26, 2024

Nana-EC commented Dec 27, 2024

belloibrahv commented Jan 3, 2025

acuarica left a comment • edited Loading

Choose a reason for hiding this comment

acuarica commented Jan 4, 2025 • edited Loading

Nana-EC commented Jan 7, 2025

quiet-node 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

sonarqubecloud bot commented Jan 8, 2025

Quality Gate passed

belloibrahv commented Jan 8, 2025

belloibrahv commented Dec 21, 2024 •

edited by Nana-EC

Loading

acuarica Dec 21, 2024 •

edited

Loading

acuarica left a comment •

edited

Loading

acuarica commented Jan 4, 2025 •

edited

Loading