[ecmascript] (#311) Fix for Unknown error on VROES.import from a missing/invalid module path #345

bcpmihail · 2024-07-18T08:31:56Z

Description

Issue summary:
VROES.import(...).from(invalidModulePath) threw an Unknown error. Cause was calling System.getModule() with blank path.

Changes:
Added validation for module path and specifiers in ecmascript/Module, affecting for VROES.import().from().
Additional parameter for handling the validation errors in Module.import(), Module.load() with default value - function to log the error and return null (avoids breaking changes).
Removed unused path parameter and from() method from export.
Documentation (comments, README, release note)
Unit tests.

Checklist

I have added relevant error handling and logging messages to help troubleshooting
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation, relevant usage information (if applicable)
I have updated the PR title with affected component, related issue number and a short summary of the changes introduced
I have added labels for implementation kind (kind/) and version type (version/)
I have tested against live environment, if applicable
I have synced any structure and/or content vRA-NG improvements with vra-ng and ts-vra-ng archetypes (if applicable)
I have my changes rebased and squashed to the minimal number of relevant commits. Notice: don't squash all commits
I have added a descriptive commit message with a short title, including a Fixed #XXX - or Closed #XXX - prefix to auto-close the issue

Testing

Tested on a client environment with the same action as the issue and a library that searched through potential module paths.

…est indentation Signed-off-by: Mihail Penchev (c) <[email protected]>

Added more specific errors and default error handler (System.error). Removed unused Module.require, Module.export parameter, minor fixes Signed-off-by: Mihail Penchev (c) <[email protected]>

Renamed DEFAULT_ERR_HANDLER, minor formatting fix Signed-off-by: Mihail Penchev (c) <[email protected]>

Signed-off-by: Mihail Penchev (c) <[email protected]>

…ues in README Signed-off-by: Mihail Penchev (c) <[email protected]>

dimitar-nikolov-lazarov · 2024-07-18T09:10:22Z

packages/ecmascript/src/Module.ts

-			}
+		moduleInfo = !path ? null : System.getModule(path);
+		if (!moduleInfo) {
+			throw new Error(`No action or module found for paths: '${path}', '${path}/index'!`);


Maybe we could include moduleName, actionName as additional information to error message. In case of typos, strange chars, etc it could be helpful.

Path is included for that purpose. It is unknown whether an action or a module (namespace) is being imported.
The specifiers (elements to be imported from a module, or default/*) are validated separately

dimitar-nikolov-lazarov · 2024-07-18T09:10:45Z

packages/ecmascript/src/Module.ts

+					actionResult = invokeActionOrModule(path, onError);
+				}
+				catch (err) {
+					error = err?.message || err;


Best approach is always to store whole Exception object, instead of error message property only.
Small chance at TS/JS but whole exception/error object may contains some additional information such as stackTrace or internal error details.

Because error variable has initial value string then (something like this):
.... catch(err){ error = err.toString(); }...

…th Error objects Signed-off-by: Mihail Penchev (c) <[email protected]>

Michaelpalacce · 2024-07-19T05:59:39Z

You need to increase your test coverage for this. You've added functionality that was not tested.

Michaelpalacce · 2024-07-19T06:04:14Z

Passing the error handler for each function sounds like a bad idea. We won't really use Module directly in the first place either. We need to be opinionated with this class and handle errors our way.

Notes:

Each function having a different error handler sounds like a poor idea
Not apparent that it's an error handler for module import errors only
Consider giving people error handling options... for example: throw on error or log on error instead
Do global error handling, perhaps with some sort of a static setter

Michaelpalacce

more tests and move error handling away from each method

bcpmihail · 2024-08-01T12:28:16Z

Merged latest changes from main (as a "chore" squashed commit)
Addressed PR comments (not yet tested, pending new env. creation; added pr/wip label):

Error handling moved from each method, only Module.load and import have error handling.
Standard options for module error-handling provided in an enum, custom error handler still supported.
Module error handler is set globally for Module/VROES (new setter), by default is System.error (doesn't throw)
updated unit tests (no new tests because there are no new public methods apart from a setter)
Additional:
Added Regex validation for module/action path (w/ support for relative path in import)
minor refactoring - extracted methods
documentation updated and moved to interfaces

vmwclabot · 2024-08-01T14:27:59Z

@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit 89b654c must be signed by [email protected]

vmwclabot · 2024-08-01T14:27:59Z

@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit 89b654c must be signed by [email protected]

bcpmihail · 2024-08-01T14:32:44Z

Tested successfully on an environment, small fix was required.

…ng options, regex validation New enum with error handling options for the Module.load and import methods. New method setModuleErrorHandler in VROES and Module, accepting enum option or custom error handler Regex validation of action/module paths in Module.load/import Module error handling will only be invoked at the public method (load/import) level (not recursively) Signed-off-by: Mihail Penchev (c) <[email protected]>

Enum export was causing issues with generated JS Signed-off-by: Mihail Penchev (c) <[email protected]>

vmwclabot · 2024-08-01T14:38:33Z

@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit 4ba47d7 must be signed by [email protected]

vmwclabot · 2024-08-01T14:38:33Z

@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit 4ba47d7 must be signed by [email protected]

…ort-unknown-error Signed-off-by: Mihail Penchev (c) <[email protected]>

Signed-off-by: Mihail Penchev (c) <[email protected]>

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

bcpmihail · 2024-08-01T15:13:36Z

Conflicts resolved. @Michaelpalacce @dimitar-nikolov-lazarov please review.

VenelinBakalov · 2024-08-06T07:33:15Z

@dimitar-nikolov-lazarov @Michaelpalacce whenever you have the chance to check

…ix/issue-311-vroes-import-unknown-error Signed-off-by: Mihail Penchev (c) <[email protected]>

Michaelpalacce · 2024-08-12T12:41:18Z

packages/ecmascript/src/Module.ts

+	 * - one of the predefined {@link DefaultModuleErrorHandlers} options
+	 * Default is {@link DefaultModuleErrorHandlers.SYS_ERROR}
+	 */
+	setModuleErrorHandler(eh: DefaultModuleErrorHandlers | ModuleErrorHandler): void;


Do we really want to be accepting a string or a function? It's breaking the "S" in SOLID.

Michaelpalacce · 2024-08-12T12:48:46Z

packages/ecmascript/src/Module.ts

+/**
+ * Predefined handlers for errors on loading/importing a module or action - see {@link DefaultModuleErrorHandlers}
+ */
+const ModuleErrorHandlers = {


Honestly I don't think this will ever change.. we give them a default and they can overwrite it if they want, imho

Agreed. Keeping only the System.error option as default (will also address #345 (comment) )

Michaelpalacce · 2024-08-12T12:49:32Z

packages/ecmascript/src/Module.ts

+ * Note - use of '-' and capital letters is not limited, despite not being recommended.
+ * Using non-capturing group (?:) to reduce overhead.
+ */
+const IMPORT_BASE_REGEX = /^(?:[\w-]+\.)*[\w-]+$/g;


Lets give examples for these regexes?

Michaelpalacce · 2024-08-12T12:50:20Z

packages/ecmascript/src/Module.ts

@@ -94,6 +205,7 @@ export interface ModuleElementList {
 		return this;
 	};

+	/** @deprecated - unused and not part of {@link ModuleExport} */


Deprecations should happen after an internal triage, lets add this to the roadmap to discuss?

Michaelpalacce · 2024-08-12T12:52:15Z

packages/ecmascript/src/Module.ts

+	 * @throws Error if the relative path goes too many steps back (via ../) in the base path
+	 */
+
+	function constructAbsolutePath(path: string, base: string): string {


This needs to be extensively unit tested. What I mean here: ONLY this method needs to be unit tested. With a data provider that will have: path, base, expectedFailure, expected or sth like that

Michaelpalacce · 2024-08-12T12:53:51Z

packages/ecmascript/src/Module.ts

+	 * @throws Error if there are no specifiers
+	 * @throws Errir containing a list of invalid specifiers (blank, duplicate, missing from the provided module), if any.
+	 */
+	function extractImports(specifiers: string[], importedModule: any): any[] {


Same as previous comment about unit testing

…ault module error handler Addressing PR comments - removed enum with module error handler options Signed-off-by: Mihail Penchev (c) <[email protected]>

@deprecated

…emoved @deprecated Pending discussion to re-add @deprecated for Module.Export.from` Signed-off-by: Mihail Penchev (c) <[email protected]>

Michaelpalacce · 2024-08-19T08:02:46Z

LGTM

…odule.constructAbsolutePath Signed-off-by: Mihail Penchev (c) <[email protected]>

…odule.extractImports Signed-off-by: Mihail Penchev (c) <[email protected]>

bcpmihail added 6 commits July 17, 2024 13:02

style: [Issue 311] Validations in ecmascript/Module import - module t…

892ddf9

…est indentation Signed-off-by: Mihail Penchev (c) <[email protected]>

feat: [Issue 311] Validations in ecmascript/Module import, load

2063322

Added more specific errors and default error handler (System.error). Removed unused Module.require, Module.export parameter, minor fixes Signed-off-by: Mihail Penchev (c) <[email protected]>

docs: [Issue 311] Validations in ecmascript/Module - comments

677e45d

Renamed DEFAULT_ERR_HANDLER, minor formatting fix Signed-off-by: Mihail Penchev (c) <[email protected]>

test: [Issue 311] Validations in ecmascript/Module - unit tests

778920d

Signed-off-by: Mihail Penchev (c) <[email protected]>

docs: [Issue 311] Validations in ecmascript/Module - README

06e5447

Signed-off-by: Mihail Penchev (c) <[email protected]>

docs: [Issue 311] Validations in ecmascript/Module - release note

efd6150

Signed-off-by: Mihail Penchev (c) <[email protected]>

bcpmihail self-assigned this Jul 18, 2024

bcpmihail requested a review from a team as a code owner July 18, 2024 08:31

bcpmihail linked an issue Jul 18, 2024 that may be closed by this pull request

VROES.import from invalid package throws Unknown error #311

Closed

github-actions bot added the kind/bug Something isn't working label Jul 18, 2024

bcpmihail added lang/typescript Related to typescript code area/ecmascript Relates to ecmascript module labels Jul 18, 2024

bcpmihail requested a review from Michaelpalacce July 18, 2024 08:33

docs: [Issue 311] Validations in ecmascript/Module - fixed linter iss…

9299814

…ues in README Signed-off-by: Mihail Penchev (c) <[email protected]>

dimitar-nikolov-lazarov reviewed Jul 18, 2024

View reviewed changes

fix: [Issue 311] Validations in ecmascript/Module - error handling wi…

275e79c

…th Error objects Signed-off-by: Mihail Penchev (c) <[email protected]>

Michaelpalacce requested changes Jul 19, 2024

View reviewed changes

VenelinBakalov added the version/patch The change is a non-breaking bugfix label Jul 30, 2024

bcpmihail added the pr/wip This PR is still work in progress, added if we don't want the stale bot to auto close it label Aug 1, 2024

vmwclabot added the dco-required label Aug 1, 2024

bcpmihail force-pushed the bugfix/issue-311-vroes-import-unknown-error branch from 89b654c to 16c50af Compare August 1, 2024 14:30

vmwclabot removed the dco-required label Aug 1, 2024

bcpmihail added 2 commits August 1, 2024 17:36

fix: [Issue 311] Validations in ecmascript/Module - removed export

7a3d436

Enum export was causing issues with generated JS Signed-off-by: Mihail Penchev (c) <[email protected]>

bcpmihail force-pushed the bugfix/issue-311-vroes-import-unknown-error branch from 16c50af to 4ba47d7 Compare August 1, 2024 14:38

vmwclabot added dco-required labels Aug 1, 2024

Merge remote-tracking branch 'origin' into bugfix/issue-311-vroes-imp…

680b867

…ort-unknown-error Signed-off-by: Mihail Penchev (c) <[email protected]>

bcpmihail force-pushed the bugfix/issue-311-vroes-import-unknown-error branch from 4ba47d7 to 680b867 Compare August 1, 2024 14:40

vmwclabot removed the dco-required label Aug 1, 2024

bcpmihail added 2 commits August 1, 2024 18:03

docs: [Issue 311] Validations in ecmascript/Module - updated README

46a1048

Signed-off-by: Mihail Penchev (c) <[email protected]>

Merge branch 'main' into bugfix/issue-311-vroes-import-unknown-error

d8a48bc

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

VenelinBakalov requested review from dimitar-nikolov-lazarov and Michaelpalacce August 2, 2024 10:48

chore: Merge latest changes from the main branch (v.2.41.0) into bugf…

74cad6b

…ix/issue-311-vroes-import-unknown-error Signed-off-by: Mihail Penchev (c) <[email protected]>

Michaelpalacce requested changes Aug 12, 2024

View reviewed changes

bcpmihail added 2 commits August 19, 2024 09:21

fix: [Issue 311] Validations in ecmascript/Module - kept a single def…

def318b

…ault module error handler Addressing PR comments - removed enum with module error handler options Signed-off-by: Mihail Penchev (c) <[email protected]>

docs: [Issue 311] Validations in ecmascript/Module - regex example, r…

5687f7c

…emoved @deprecated Pending discussion to re-add @deprecated for Module.Export.from` Signed-off-by: Mihail Penchev (c) <[email protected]>

bcpmihail force-pushed the bugfix/issue-311-vroes-import-unknown-error branch from 60aec81 to 5687f7c Compare August 19, 2024 07:13

Michaelpalacce approved these changes Aug 19, 2024

View reviewed changes

bcpmihail added 2 commits August 19, 2024 11:56

test: [Issue 311] Validations in ecmascript/Module - unit tests for M…

67ededb

…odule.constructAbsolutePath Signed-off-by: Mihail Penchev (c) <[email protected]>

test: [Issue 311] Validations in ecmascript/Module - unit tests for M…

513d8c1

…odule.extractImports Signed-off-by: Mihail Penchev (c) <[email protected]>

Michaelpalacce approved these changes Aug 19, 2024

View reviewed changes

bcpmihail merged commit dd57325 into main Aug 19, 2024
14 checks passed

VenelinBakalov deleted the bugfix/issue-311-vroes-import-unknown-error branch August 19, 2024 11:08

VenelinBakalov changed the title ~~[ecmascript] (#311)Fix for Unknown error on VROES.import~~ [ecmascript] (#311) Fix for Unknown error on VROES.import Aug 19, 2024

VenelinBakalov changed the title ~~[ecmascript] (#311) Fix for Unknown error on VROES.import~~ [ecmascript] (#311) Fix for Unknown error on VROES.import from a missing/invalid module path Aug 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ecmascript] (#311) Fix for Unknown error on VROES.import from a missing/invalid module path #345

[ecmascript] (#311) Fix for Unknown error on VROES.import from a missing/invalid module path #345

bcpmihail commented Jul 18, 2024 •

edited

Loading

dimitar-nikolov-lazarov Jul 18, 2024

bcpmihail Jul 19, 2024 •

edited

Loading

dimitar-nikolov-lazarov Jul 18, 2024 •

edited

Loading

Michaelpalacce commented Jul 19, 2024

Michaelpalacce commented Jul 19, 2024

Michaelpalacce left a comment

bcpmihail commented Aug 1, 2024

vmwclabot commented Aug 1, 2024

vmwclabot commented Aug 1, 2024

bcpmihail commented Aug 1, 2024

vmwclabot commented Aug 1, 2024

vmwclabot commented Aug 1, 2024

bcpmihail commented Aug 1, 2024

VenelinBakalov commented Aug 6, 2024

Michaelpalacce Aug 12, 2024

Michaelpalacce Aug 12, 2024

bcpmihail Aug 19, 2024

Michaelpalacce Aug 12, 2024

Michaelpalacce Aug 12, 2024

Michaelpalacce Aug 12, 2024

Michaelpalacce Aug 12, 2024

Michaelpalacce commented Aug 19, 2024

[ecmascript] (#311) Fix for Unknown error on VROES.import from a missing/invalid module path #345

[ecmascript] (#311) Fix for Unknown error on VROES.import from a missing/invalid module path #345

Conversation

bcpmihail commented Jul 18, 2024 • edited Loading

Description

Checklist

Testing

Choose a reason for hiding this comment

bcpmihail Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

dimitar-nikolov-lazarov Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Michaelpalacce commented Jul 19, 2024

Michaelpalacce commented Jul 19, 2024

Michaelpalacce left a comment

Choose a reason for hiding this comment

bcpmihail commented Aug 1, 2024

vmwclabot commented Aug 1, 2024

vmwclabot commented Aug 1, 2024

bcpmihail commented Aug 1, 2024

vmwclabot commented Aug 1, 2024

vmwclabot commented Aug 1, 2024

bcpmihail commented Aug 1, 2024

VenelinBakalov commented Aug 6, 2024

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

Michaelpalacce commented Aug 19, 2024

bcpmihail commented Jul 18, 2024 •

edited

Loading

bcpmihail Jul 19, 2024 •

edited

Loading

dimitar-nikolov-lazarov Jul 18, 2024 •

edited

Loading