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

bug(apidom-parser-adapter-yaml-1-2): Change message returned for syntax errors #2914

Closed

Conversation

damian-polewski-sb
Copy link
Contributor

@damian-polewski-sb damian-polewski-sb commented Jul 7, 2023

This PR changes the error message returned by apidom-parser-adapter-yaml-1-2 when it encounters a Syntax error.

Description

This PR does the following:

  • Changes error message returned by CstVisitor
  • Adds a condition to add the correct error message in ParseResult when it's YAML Syntax error
  • Adds unit tests for adapter-browser and adapter-node

Motivation and Context

closes #2889
Currently if apidom-parser-adapter-yaml-1-2 encounters Syntax errors in YAML definition it returns the ERROR node as an error message which is not useful information from user perspective, that's why it would be better to return the message "YAML Syntax error"
The initial idea to solve this issue was to check if the error message value is an ERROR node containing part of YAML definition and then replace the message with "YAML Syntax error" when processing an error found during parsing.
When investigating this issue and discussing it with Vladimir we decided that in terms of syntax errors, we do not need to check this in code because every error found on this level is a syntax error so we can just return a static error message there.
To make sure that there wasn't any useful information returned from the parser we are using I tested different structures used in YAML 1.2.2 (using swagger-editor 5.0.0-alpha.67) to check what error messages I get when introducing a syntax error.
Tested examples (found in https://yaml.org/spec/1.2.2):

  • Sequences
    • Sequence of Scalars
    • Mapping Scalars to Scalars
    • Mapping Scalars to Sequences
    • Sequence of Mappings
    • Sequence of Sequences
    • Mapping of Mappings
  • Structures
    • Two Documents in a Stream (in this case api-dom returns an error that only first document will be used)
    • Single Document with Two Comments
    • Node appears twice in this document
    • Mapping between Sequences
    • Compact Nested Mapping
  • Scalars
    • Literal style
    • Folded style
    • Quoted Scalars
    • Multi-line Flow Scalars
  • Tags
    • Integers
    • Floating Point
    • Miscellaneous
    • Timestamps
    • Various Explicit Tags
    • Global Tags
    • Unordered Sets
    • Ordered Mappings

In every scenario, I tried to cause a syntax error by removing indentation or part of correct YAML syntax (e.g. dash and space for block sequences or colon and space for mappings) which caused an ERROR node to be returned with part of YAML definition (elements with same scope/indentation above the part that causes the error or in some cases the same line e.g. when removing square brackets from flow sequence, curly braces from flow mapping or quote marks from quoted scalars).

How Has This Been Tested?

It has been tested by adding new unit tests in the package folder.

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@damian-polewski-sb damian-polewski-sb self-assigned this Jul 7, 2023
@damian-polewski-sb damian-polewski-sb changed the title Bug/change message returned for syntax errors bug(apidom-parser-adapter-yaml-1-2): Change message returned for syntax errors Jul 7, 2023
@damian-polewski-sb damian-polewski-sb force-pushed the bug/change-message-returned-for-syntax-errors branch from 8c40813 to 720559a Compare July 10, 2023 10:29
@char0n
Copy link
Member

char0n commented Jul 10, 2023

Hi @damian-polewski-sb,

Thanks for the PR! Looking in it now.

@char0n
Copy link
Member

char0n commented Jul 10, 2023

We seem to be having issue with tests. This happens in apidom-ls package, when certain tests fail, the language service never gets shut down and it handles indefinitely. I'll looks into it tommorow.

@damian-polewski-sb
Copy link
Contributor Author

@char0n Could you please let me know which tests are failing for you? I run unit tests for apidom-ls locally and they all passed but when I run test for the whole monorepo where was test failing for apidom-reference with duplicate parameters.

@char0n
Copy link
Member

char0n commented Jul 12, 2023

Closing in favor of #2931

@char0n char0n closed this Jul 12, 2023
char0n added a commit that referenced this pull request Jul 12, 2023
Co-authored-by: damian-polewski-sb <damian.polewski@smartbear.com>

Refs #2914
Refs #2889
@char0n char0n deleted the bug/change-message-returned-for-syntax-errors branch January 17, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better error messages for YAML syntax errors
2 participants