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

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

Closed
wants to merge 25 commits into from

Conversation

kailash360
Copy link

@kailash360 kailash360 commented Mar 3, 2024

I added status and code to the errors.

Closes #337

Changes

  • Adds status and code to the different exceptions
  • New exception types added as constant

Flags

N/A

Screenshots or Video

N/A

Related Issues

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

mttrbrts and others added 23 commits November 24, 2023 13:26
* feat(types): build uniontypes too

Signed-off-by: Matt Roberts <[email protected]>

* fix(build): include unions in index

Signed-off-by: Matt Roberts <[email protected]>

* chore(deps): upgrade codegen to latest release

Signed-off-by: Matt Roberts <[email protected]>

---------

Signed-off-by: Matt Roberts <[email protected]>
…#767)

* fix(parser): throw error when concept is extending itself in CTO

Signed-off-by: Stefan Blaginov <[email protected]>
Signed-off-by: Stefan Blaginov <[email protected]>

* fix(parser): throw error when concept is extending itself in JSON metamodel form

Signed-off-by: Stefan Blaginov <[email protected]>
Signed-off-by: Stefan Blaginov <[email protected]>

* fix(parser): throw error when concept is extending itself in the AST

Signed-off-by: Stefan Blaginov <[email protected]>
Signed-off-by: Stefan Blaginov <[email protected]>

* refactor(validation): alphabetical rearrangement

Signed-off-by: Stefan Blaginov <[email protected]>
Signed-off-by: Stefan Blaginov <[email protected]>

* test(self-extending): remove redundant tests (codepath covered in concerto-cto)

Signed-off-by: Stefan Blaginov <[email protected]>
Signed-off-by: Stefan Blaginov <[email protected]>

* test(fix): remove unneeded import

Signed-off-by: Stefan Blaginov <[email protected]>
Signed-off-by: Stefan Blaginov <[email protected]>

---------

Signed-off-by: Stefan Blaginov <[email protected]>
Signed-off-by: Stefan Blaginov <[email protected]>
Co-authored-by: Stefan Blaginov <[email protected]>
…ccordproject#773)

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

Signed-off-by: Santanu Roy <[email protected]>
Co-authored-by: Santanu Roy <[email protected]>
…/concerto-util (accordproject#780)

chore(deps): bump follow-redirects in /packages/concerto-util

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.0 to 1.15.4.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.15.0...v1.15.4)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ages/concerto-types (accordproject#783)

chore(deps-dev): bump follow-redirects in /packages/concerto-types

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.3 to 1.15.4.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.15.3...v1.15.4)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…/concerto-vocabulary (accordproject#782)

chore(deps): bump follow-redirects in /packages/concerto-vocabulary

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.3 to 1.15.4.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.15.3...v1.15.4)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…dproject/concerto-util in /packages/concerto-cto (accordproject#786)

chore(deps): bump axios, @accordproject/concerto-metamodel and @accordproject/concerto-util

Bumps [axios](https://github.com/axios/axios) to 1.6.0 and updates ancestor dependencies [axios](https://github.com/axios/axios), [@accordproject/concerto-metamodel](https://github.com/accordproject/concerto/tree/HEAD/packages/concerto-metamodel) and [@accordproject/concerto-util](https://github.com/accordproject/concerto/tree/HEAD/packages/concerto-util). These dependencies need to be updated together.


Updates `axios` from 0.23.0 to 1.6.0
- [Release notes](https://github.com/axios/axios/releases)
- [Changelog](https://github.com/axios/axios/blob/v1.x/CHANGELOG.md)
- [Commits](axios/axios@v0.23.0...v1.6.0)

Updates `@accordproject/concerto-metamodel` from 3.8.1 to 3.9.1
- [Release notes](https://github.com/accordproject/concerto/releases)
- [Commits](https://github.com/accordproject/concerto/commits/v3.9.1/packages/concerto-metamodel)

Updates `@accordproject/concerto-util` from 3.13.0 to 3.16.1
- [Release notes](https://github.com/accordproject/concerto/releases)
- [Changelog](https://github.com/accordproject/concerto/blob/main/packages/concerto-util/changelog.txt)
- [Commits](https://github.com/accordproject/concerto/commits/v3.16.1/packages/concerto-util)

---
updated-dependencies:
- dependency-name: axios
  dependency-type: indirect
- dependency-name: "@accordproject/concerto-metamodel"
  dependency-type: direct:production
- dependency-name: "@accordproject/concerto-util"
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…kages/concerto-core (accordproject#789)

chore(deps): bump axios and @accordproject/concerto-metamodel

Bumps [axios](https://github.com/axios/axios) to 1.6.7 and updates ancestor dependency [@accordproject/concerto-metamodel](https://github.com/accordproject/concerto/tree/HEAD/packages/concerto-metamodel). These dependencies need to be updated together.


Updates `axios` from 0.23.0 to 1.6.7
- [Release notes](https://github.com/axios/axios/releases)
- [Changelog](https://github.com/axios/axios/blob/v1.x/CHANGELOG.md)
- [Commits](axios/axios@v0.23.0...v1.6.7)

Updates `@accordproject/concerto-metamodel` from 3.9.0 to 3.9.1
- [Release notes](https://github.com/accordproject/concerto/releases)
- [Commits](https://github.com/accordproject/concerto/commits/v3.9.1/packages/concerto-metamodel)

---
updated-dependencies:
- dependency-name: axios
  dependency-type: indirect
- dependency-name: "@accordproject/concerto-metamodel"
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…roject#790)

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.2 to 1.15.5.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.15.2...v1.15.5)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…o-core (accordproject#792)

chore(deps-dev): bump axios in /packages/concerto-core

Bumps [axios](https://github.com/axios/axios) from 0.23.0 to 1.6.0.
- [Release notes](https://github.com/axios/axios/releases)
- [Changelog](https://github.com/axios/axios/blob/v1.x/CHANGELOG.md)
- [Commits](axios/axios@v0.23.0...v1.6.0)

---
updated-dependencies:
- dependency-name: axios
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…le (accordproject#794)

* refactor(declarations): Move unique name check to model file.

Signed-off-by: Ertugrul Karademir <[email protected]>

* refactor(test): move validation checks for duplicate class to model file

Signed-off-by: Ertugrul Karademir <[email protected]>

* test: empty commit to trigger test

Signed-off-by: Ertugrul Karademir <[email protected]>

---------

Signed-off-by: Ertugrul Karademir <[email protected]>
refactor: don't use arrays to check uniqueness

Signed-off-by: Ertugrul Karademir <[email protected]>
accordproject#804)

* refactor: don't use arrays to check uniqueness

Signed-off-by: Ertugrul Karademir <[email protected]>

* refactor: also refactor unique property name check

Signed-off-by: Ertugrul Karademir <[email protected]>

* refactor: remove array for decorator uniqueness check

Signed-off-by: Ertugrul Karademir <[email protected]>

* refactor: remove uniqueness check from scalar declarations as well

Signed-off-by: Ertugrul Karademir <[email protected]>

---------

Signed-off-by: Ertugrul Karademir <[email protected]>
Signed-off-by: Sanket Shevkar <[email protected]>
Co-authored-by: Sanket Shevkar <[email protected]>
@kailash360 kailash360 changed the title feat: Added "status" and "code`to errors feat: Added "status" and "code" to errors Mar 3, 2024
@kailash360
Copy link
Author

@mttrbrts

I have created this PR to add status and code in the exceptions so that they can be accessed easily.

@mttrbrts mttrbrts self-requested a review March 5, 2024 21:05
Copy link
Member

@mttrbrts mttrbrts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start, thank you @kailash360.

I'd like to start a discussion about the design of the error codes before we fix them. For example, I think that there are semantic groups that we should use to categorise the similar codes (e.g. taking inspiration from the HTTP Status codes, e.g. https://www.npmjs.com/package/http-status-codes).

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit here of splitting the code and status into two parameters? Would one suffice?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can surely reduce it to the {code,status} object that we want and pass it directly, instead of spreading it. This approach was made keeping in mind that there wasn't any grouping of errors defined explicitly. So the user can perhaps use a separate error code and status to define in an exception. I will reduce it to a single object.

Comment on lines +1 to +46
export namespace DEFAULT_BASE_EXCEPTION {
const status: string;
const code: string;
}
export namespace DEFAULT_VALIDATOR_EXCEPTION {
const status_1: string;
export { status_1 as status };
const code_1: string;
export { code_1 as code };
}
export namespace REGEX_VALIDATOR_EXCEPTION {
const status_2: string;
export { status_2 as status };
const code_2: string;
export { code_2 as code };
}
export namespace TYPE_NOT_FOUND_EXCEPTION {
const status_3: string;
export { status_3 as status };
const code_3: string;
export { code_3 as code };
}
export namespace CONCERTO_SEMANTIC_EXCEPTION {
const status_4: string;
export { status_4 as status };
const code_4: string;
export { code_4 as code };
}
export namespace SECURITY_EXCEPTION {
const status_5: string;
export { status_5 as status };
const code_5: string;
export { code_5 as code };
}
export namespace METAMODEL_EXCEPTION {
const status_6: string;
export { status_6 as status };
const code_6: string;
export { code_6 as code };
}
export namespace ILLEGAL_MODEL_EXCEPTION {
const status_7: string;
export { status_7 as status };
const code_7: string;
export { code_7 as code };
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change, unfortunately. Is there a way to make this backwards compatible? If not, we should target the v4.0 branch

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, will rebase the PR to v4.0

@kailash360
Copy link
Author

This is a great start, thank you @kailash360.

I'd like to start a discussion about the design of the error codes before we fix them. For example, I think that there are semantic groups that we should use to categorise the similar codes (e.g. taking inspiration from the HTTP Status codes, e.g. https://www.npmjs.com/package/http-status-codes).

Considering the HTTP Status codes, we can obviously define our own error codes. I referred to this resource explaining the context of numbering the HTTP Status codes. On a higher ground, they are categorized based on the nature or aspect of the HTTP request. In our case, we need to define the depth that we need to cover.

For example, we can define status codes for TypeNotFoundException or SecurityException and use them anytime we get these errors. But for a detailed explanation, we can also define the family of TypeNotFoundException errors as 3xx and specify the error code for each of the sub-types, and follow this for the other exceptions as well. I personally feel the latter approach will add a big overhead.

If you recommend, I can group the existing exceptions in the below format:

Syntax Errors
Starts with 3 and follows 3x format

  • 31 TypeNotFoundException
  • 32 ValidationException
  • 33 MetaModelException

Semantic Errors
Starts with 4 and follows 4x format

  • 41 BaseFileException
  • 42 IllegalModelException
  • 43 SecurityException

Let me know if this sounds good to proceed.

@kailash360 kailash360 changed the base branch from main to v4.0.0 March 6, 2024 13:05
Signed-off-by: Kailash Kejriwal <[email protected]>
@mttrbrts
Copy link
Member

Yes, this is the right direction. I recommend that we discuss this design live on a Working Group call. They run weekly at 8am EST

Also, can you rebase you PR on top of the v4.0.0 branch, please? If you look at the commit log, your PR also includes lots of unrelated changes.

@mttrbrts
Copy link
Member

@kailash360

We reviewed this change in the working group call on 13 March (https://vimeo.com/922895729). The consensus was that a previous change has already addressed this issue sufficiently by adding types to Errors. We haven't solved the categorisation problem but we should first come up with a design for that in an issue, before writing code.

I'm sorry for any wasted time here on your part.

@mttrbrts mttrbrts closed this Mar 21, 2024
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.

Add code and status properties to produced errors
5 participants