fix(error): adding type to error in string validator in introspect #773

santanu8961 · 2023-12-15T15:02:24Z

Closes #772

Changes

Adds type to validator error in introspect validator.

added a new optional parameter to class BaseException which can take errorType as well as errorMessage which helps to determine what kind of error is getting thrown from the validator itself. and using that to throw error in validator.
also added this in NotFoundException for better complience

Author Checklist

Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
Vital features and changes captured in unit and/or integration tests
Commits messages follow AP format
Extend the documentation, if necessary
Merging to main from fork:branchname

Signed-off-by: Santanu Roy <edu.santanu12@gmail.com>

packages/concerto-core/lib/introspect/stringvalidatorexception.js

sanketshevkar · 2023-12-18T08:23:05Z

Should we have a new folder for error classes and not keep it in introspect?

ekarademir · 2023-12-18T11:11:13Z

packages/concerto-core/lib/introspect/stringvalidatorexception.js

+* @class
+* @memberof module:concerto-core
+*/
+class StringValidationException extends Error {


I think this can be named just ValidationException. If not, name of the class should be the same as the file name.

File name is stringvalidaTORexception and name of the class is StringValidaTIONException.

It should be Validator, not Validation. Internally the regex keyword is called a StringValidator.

Yes, we should ensure we can use similar approach with NumberValidator as well.

Hi @mttrbrts , @ekarademir I am keeping the name of the class as StringValidatiorException and the default error as ValidatorError and the regex error as RegexValidatorException.
let me know if this works fine . and this is what you meant.

ekarademir · 2023-12-18T11:15:37Z

packages/concerto-core/lib/introspect/validator.js

     * @throws {Error} throws an error to report the message
     */
-    reportError(id, msg) {
-        throw new Error( 'Validator error for field `' + id + '`. ' + this.getFieldOrScalarDeclaration().getFullyQualifiedName() + ': ' + msg );
+    reportError(id, msg, errorType='ValidatorError') {


What is the use of this additional argument? How is it different than checking the type of the error that's been thrown?

Let's avoid having to use instanceof checks - those make assumptions about how packages are required (instanceof can fail if the same code is loaded at different version) which have bitten us in the past.

Hi @ekarademir I have used the additional argument errorType so that it can be propagated through till StringValidationException where we can use it to denote what kind of error it is , which can be very useful to do any kind of operation as only error string is quick tricky to detect what error it is trying to throw

Hi @dselman , can you please clarify what do you mean by the instance of checks?
Thanks.

I mean client code doing things like:

try { ... } catch(err) { if(err instanceof StringValidatorException) { ... } }

dselman

I like the motivation for this. I just needs a bit of refactoring to ensure it works with NumberValidator and can be cleanly consumed upstream.

dselman · 2023-12-18T13:29:47Z

packages/concerto-core/lib/introspect/stringvalidatorexception.js

+* @class
+* @memberof module:concerto-core
+*/
+class StringValidationException extends Error {


Yes, we should ensure we can use similar approach with NumberValidator as well.

dselman · 2023-12-18T13:31:52Z

packages/concerto-core/lib/introspect/validator.js

     * @throws {Error} throws an error to report the message
     */
-    reportError(id, msg) {
-        throw new Error( 'Validator error for field `' + id + '`. ' + this.getFieldOrScalarDeclaration().getFullyQualifiedName() + ': ' + msg );
+    reportError(id, msg, errorType='ValidatorError') {


Let's avoid having to use instanceof checks - those make assumptions about how packages are required (instanceof can fail if the same code is loaded at different version) which have bitten us in the past.

dselman

I would prefer to see generic Error Code functionality moved into concerto-util/BaseException, and to refactor code to throw Errors that derive from BaseException.

Error codes should be defined constants so that code is type safe.

dselman · 2023-12-21T11:24:06Z

packages/concerto-core/lib/introspect/validator.js

     * @throws {Error} throws an error to report the message
     */
-    reportError(id, msg) {
-        throw new Error( 'Validator error for field `' + id + '`. ' + this.getFieldOrScalarDeclaration().getFullyQualifiedName() + ': ' + msg );
+    reportError(id, msg, errorType='ValidatorError') {


I mean client code doing things like:

try { ... } catch(err) { if(err instanceof StringValidatorException) { ... } }

dselman · 2023-12-21T11:33:25Z

packages/concerto-core/lib/introspect/stringvalidator.js

@@ -70,7 +70,7 @@ class StringValidator extends Validator{
                this.regex = new CustomRegExp(validator.pattern, validator.flags);
            }
            catch(exception) {
-                this.reportError(field.getName(), exception.message);
+                this.reportError(field.getName(), exception.message,'RegexValidatorException');


I think if we want to go this route then the error codes should be defined and exported and referenced.

E.g. something like:

ErrorCodes.js export const INVALID_REGEX = 1;

Somewhere we want to report an error:

this.reportError(field.getName(), exception.message, ErrorCodes.INVALID_REGEX);

This allows a client to do something like:

import {ErrorCodes} from '@accordproject/concerto-core'; try { ... } catch(err) { const errorCode = err?.getErrorCode.?(); switch(errorCode) { case ErrorCodes.INVALID_REGEX: // do something specific here break; default: // general case }

We should have in mind that concerto-core will be moving to TypeScript and we want these error codes to be type safe.

Hi @dselman , as you requested I have changed the structure by doing the following changes to the structure:

I no longer am using the StringValidatorException class instead I have added the errorType as an optional param in BaseException and using that.

added an errorCodes.js file where I am storing the error type constants and using them directly , these are exposed so the client will also be able to use these , as discussed in the PR

dselman · 2023-12-21T11:36:24Z

packages/concerto-core/lib/introspect/stringvalidatorexception.js

+* @class
+* @memberof module:concerto-core
+*/
+class StringValidatorException extends Error {


This could be more generic - it extends Error and adds an Error Code. We could add an optional error code to concerto-util/BaseException, so that all exceptions can reference an error code.

santanu8961 · 2023-12-28T10:05:21Z

packages/concerto-core/lib/introspect/stringvalidator.js

@@ -70,7 +71,7 @@ class StringValidator extends Validator{
                this.regex = new CustomRegExp(validator.pattern, validator.flags);
            }
            catch(exception) {
-                this.reportError(field.getName(), exception.message);
+                this.reportError(field.getName(), exception.message,ErrorCodes.REGEX_VALIDATOR_EXCEPTION);


Here I have used error codes as constants instead of inline strings for consistency

santanu8961 · 2023-12-28T10:05:38Z

packages/concerto-core/lib/introspect/validator.js

     * @throws {Error} throws an error to report the message
     */
-    reportError(id, msg) {
-        throw new Error( 'Validator error for field `' + id + '`. ' + this.getFieldOrScalarDeclaration().getFullyQualifiedName() + ': ' + msg );
+    reportError(id, msg, errorType=ErrorCodes.DEFAULT_VALIDATOR_EXCEPTION) {


Here I have used error codes as constants instead of inline strings for consistency

santanu8961 · 2023-12-28T10:06:32Z

packages/concerto-core/lib/introspect/validator.js

-    reportError(id, msg) {
-        throw new Error( 'Validator error for field `' + id + '`. ' + this.getFieldOrScalarDeclaration().getFullyQualifiedName() + ': ' + msg );
+    reportError(id, msg, errorType=ErrorCodes.DEFAULT_VALIDATOR_EXCEPTION) {
+        throw new BaseException('Validator error for field `' + id + '`. ' + this.getFieldOrScalarDeclaration().getFullyQualifiedName() + ': ' + msg ,errorType);


removed StringValidatorExceptionClass as its not needed anymore I have added a optional param errorType in BaseException which solves the purpose

santanu8961 · 2023-12-28T10:07:25Z

packages/concerto-util/lib/errorcodes.js

+ */
+
+'use strict';
+


error codes are defined in this file which can be used to implement different errors when ever needed

santanu8961 · 2023-12-28T11:49:51Z

packages/concerto-util/lib/baseexception.js

     */
-    constructor(message, component) {
+    constructor(message, component, errorType) {


added a errortype parameter which is optional and provides the errorcode related to the error

santanu8961 · 2023-12-28T11:53:54Z

Hi @dselman , as you requested I have changed the structure by doing the following changes to the structure:

I no longer am using the StringValidatorException class instead I have added the errorType as an optional param in BaseException and using that.
added an errorCodes.js file where I am storing the error type constants and using them directly , these are exposed so the client will also be able to use these , as discussed in the PR .

Signed-off-by: Santanu Roy <edu.santanu12@gmail.com> Signed-off-by: Santanu Roy <santanu.roy@docusign.com>

sanketshevkar · 2023-12-29T07:51:54Z

packages/concerto-core/lib/introspect/stringvalidator.js

@@ -70,7 +71,7 @@ class StringValidator extends Validator{
                this.regex = new CustomRegExp(validator.pattern, validator.flags);
            }
            catch(exception) {
-                this.reportError(field.getName(), exception.message);
+                this.reportError(field.getName(), exception.message,ErrorCodes.REGEX_VALIDATOR_EXCEPTION);


nit: lint check in pipeline could throw error here

sanketshevkar · 2023-12-29T08:08:47Z

packages/concerto-core/types/lib/typenotfoundexception.d.ts

-     * @param {string|undefined} message - error message.
-     * @param {string} component - the optional component which throws this error
-     */
-    constructor(typeName: string, message: string | undefined, component: string);


On the main I still see constructor on TypeNotFoundException.js but why is this getting deleted here? Is this auto generated?

Yes this is actually auto generated after I run build command

sanketshevkar · 2023-12-29T08:25:37Z

packages/concerto-core/lib/introspect/validator.js

-    reportError(id, msg) {
-        throw new Error( 'Validator error for field `' + id + '`. ' + this.getFieldOrScalarDeclaration().getFullyQualifiedName() + ': ' + msg );
+    reportError(id, msg, errorType=ErrorCodes.DEFAULT_VALIDATOR_EXCEPTION) {
+        throw new BaseException('Validator error for field `' + id + '`. ' + this.getFieldOrScalarDeclaration().getFullyQualifiedName() + ': ' + msg ,undefined,errorType);


nit: Space linting, same as above.

Signed-off-by: Santanu Roy <edu.santanu12@gmail.com>

dselman

Good work. Thanks!

…ccordproject#773) * fix(error): adding type to error in string validator in introspect Signed-off-by: Santanu Roy <edu.santanu12@gmail.com> Co-authored-by: Santanu Roy <santanu.roy@docusign.com> Signed-off-by: Angel Mendez Cano <amendez1988@gmail.com>

…ccordproject#773) * fix(error): adding type to error in string validator in introspect Signed-off-by: Santanu Roy <edu.santanu12@gmail.com> Co-authored-by: Santanu Roy <santanu.roy@docusign.com>

* fix(build): include unions in index Signed-off-by: Matt Roberts <code@rbrts.uk> * chore(deps): upgrade codegen to latest release Signed-off-by: Matt Roberts <code@rbrts.uk> --------- Signed-off-by: Matt Roberts <code@rbrts.uk> * fix(class): throw error when class is extending itself (#767) * fix(parser): throw error when concept is extending itself in CTO Signed-off-by: Stefan Blaginov <stefanblaginov@gmail.com> Signed-off-by: Stefan Blaginov <Stefan.Blaginov@docusign.com> * fix(parser): throw error when concept is extending itself in JSON metamodel form Signed-off-by: Stefan Blaginov <stefanblaginov@gmail.com> Signed-off-by: Stefan Blaginov <Stefan.Blaginov@docusign.com> * fix(parser): throw error when concept is extending itself in the AST Signed-off-by: Stefan Blaginov <stefanblaginov@gmail.com> Signed-off-by: Stefan Blaginov <Stefan.Blaginov@docusign.com> * refactor(validation): alphabetical rearrangement Signed-off-by: Stefan Blaginov <stefanblaginov@gmail.com> Signed-off-by: Stefan Blaginov <Stefan.Blaginov@docusign.com> * test(self-extending): remove redundant tests (codepath covered in concerto-cto) Signed-off-by: Stefan Blaginov <stefanblaginov@gmail.com> Signed-off-by: Stefan Blaginov <Stefan.Blaginov@docusign.com> * test(fix): remove unneeded import Signed-off-by: Stefan Blaginov <stefanblaginov@gmail.com> Signed-off-by: Stefan Blaginov <Stefan.Blaginov@docusign.com> --------- Signed-off-by: Stefan Blaginov <stefanblaginov@gmail.com> Signed-off-by: Stefan Blaginov <Stefan.Blaginov@docusign.com> Co-authored-by: Stefan Blaginov <Stefan.Blaginov@docusign.com> * fix(class-declaration): throw with undefined ast properties (#771) Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> * fix(error): adding type to error in string validator in introspect (#773) * fix(error): adding type to error in string validator in introspect Signed-off-by: Santanu Roy <edu.santanu12@gmail.com> Co-authored-by: Santanu Roy <santanu.roy@docusign.com> * refactor(declarations): move declaration uniqueness check to model file (#794) * refactor(declarations): Move unique name check to model file. Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> * refactor(test): move validation checks for duplicate class to model file Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> * test: empty commit to trigger test Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> --------- Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> * perf(core): don't use arrays to check uniqueness (#802) refactor: don't use arrays to check uniqueness Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> * perf(core): remove usage of arrays while forming duplicate item errors (#804) * refactor: don't use arrays to check uniqueness Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> * refactor: also refactor unique property name check Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> * refactor: remove array for decorator uniqueness check Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> * refactor: remove uniqueness check from scalar declarations as well Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> --------- Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> --------- Signed-off-by: Matt Roberts <code@rbrts.uk> Signed-off-by: Stefan Blaginov <stefanblaginov@gmail.com> Signed-off-by: Stefan Blaginov <Stefan.Blaginov@docusign.com> Signed-off-by: Ertugrul Karademir <ekarademir@gmail.com> Signed-off-by: Santanu Roy <edu.santanu12@gmail.com> Co-authored-by: Stefan Blaginov <stefanblaginov@gmail.com> Co-authored-by: Stefan Blaginov <Stefan.Blaginov@docusign.com> Co-authored-by: Ertugrul Karademir <ekarademir@gmail.com> Co-authored-by: Santanu Roy <43335319+santanu8961@users.noreply.github.com> Co-authored-by: Santanu Roy <santanu.roy@docusign.com>

fix(error): adding type to error in string validator in introspect

f4ab97b

Signed-off-by: Santanu Roy <edu.santanu12@gmail.com>

sanketshevkar reviewed Dec 18, 2023

View reviewed changes

packages/concerto-core/lib/introspect/stringvalidatorexception.js Outdated Show resolved Hide resolved

sanketshevkar reviewed Dec 18, 2023

View reviewed changes

packages/concerto-core/lib/introspect/stringvalidatorexception.js Outdated Show resolved Hide resolved

ekarademir reviewed Dec 18, 2023

View reviewed changes

dselman requested changes Dec 18, 2023

View reviewed changes

santanu8961 requested review from dselman and mttrbrts December 20, 2023 12:20

mttrbrts approved these changes Dec 20, 2023

View reviewed changes

dselman requested changes Dec 21, 2023

View reviewed changes

santanu8961 force-pushed the santanur/i772/provide-proper-error-object-instead-of-message-for-string-error-introspect branch from a3549e8 to 846fff2 Compare December 28, 2023 10:03

santanu8961 commented Dec 28, 2023

View reviewed changes

santanu8961 force-pushed the santanur/i772/provide-proper-error-object-instead-of-message-for-string-error-introspect branch from 846fff2 to 1139dce Compare December 28, 2023 11:46

santanu8961 commented Dec 28, 2023

View reviewed changes

santanu8961 requested a review from dselman December 28, 2023 11:50

Santanu Roy added 2 commits December 28, 2023 23:46

fix(error): addressed pr comments & error in test run

3b6180a

Signed-off-by: Santanu Roy <edu.santanu12@gmail.com> Signed-off-by: Santanu Roy <santanu.roy@docusign.com>

feat(error): addressed PR comments and restructured the code

6fce3db

Signed-off-by: Santanu Roy <edu.santanu12@gmail.com> Signed-off-by: Santanu Roy <santanu.roy@docusign.com>

santanu8961 force-pushed the santanur/i772/provide-proper-error-object-instead-of-message-for-string-error-introspect branch from 1139dce to 6fce3db Compare December 28, 2023 18:18

sanketshevkar reviewed Dec 29, 2023

View reviewed changes

feat(error): resolved pr comments

18bd20f

Signed-off-by: Santanu Roy <edu.santanu12@gmail.com>

sanketshevkar approved these changes Jan 2, 2024

View reviewed changes

dselman approved these changes Jan 2, 2024

View reviewed changes

dselman enabled auto-merge (squash) January 2, 2024 14:16

dselman merged commit 769baa0 into accordproject:main Jan 2, 2024
11 checks passed

This was referenced Mar 21, 2024

feat: Added "status" and "code" to errors #812

Closed

Add code and status properties to produced errors #337

Closed

ekarademir mentioned this pull request Apr 5, 2024

Add error codes to property validation errors #834

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(error): adding type to error in string validator in introspect #773

fix(error): adding type to error in string validator in introspect #773

santanu8961 commented Dec 15, 2023 •

edited

sanketshevkar commented Dec 18, 2023

ekarademir Dec 18, 2023

mttrbrts Dec 18, 2023

dselman Dec 18, 2023

santanu8961 Dec 19, 2023

ekarademir Dec 18, 2023

dselman Dec 18, 2023

santanu8961 Dec 19, 2023

santanu8961 Dec 19, 2023

dselman Dec 21, 2023

dselman left a comment

dselman Dec 18, 2023

dselman Dec 18, 2023

dselman left a comment

dselman Dec 21, 2023

dselman Dec 21, 2023

santanu8961 Dec 28, 2023

dselman Dec 21, 2023

santanu8961 Dec 28, 2023

santanu8961 Dec 28, 2023

santanu8961 Dec 28, 2023

santanu8961 Dec 28, 2023

santanu8961 Dec 28, 2023

santanu8961 commented Dec 28, 2023 •

edited

sanketshevkar Dec 29, 2023

santanu8961 Dec 29, 2023

sanketshevkar Dec 29, 2023

santanu8961 Dec 29, 2023

sanketshevkar Dec 29, 2023

santanu8961 Dec 29, 2023

dselman left a comment

		*/

		'use strict';

fix(error): adding type to error in string validator in introspect #773

fix(error): adding type to error in string validator in introspect #773

Conversation

santanu8961 commented Dec 15, 2023 • edited

Closes #772

Changes

Author Checklist

sanketshevkar commented Dec 18, 2023

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

dselman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dselman 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

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

santanu8961 commented Dec 28, 2023 • edited

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

dselman left a comment

Choose a reason for hiding this comment

santanu8961 commented Dec 15, 2023 •

edited

santanu8961 commented Dec 28, 2023 •

edited