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(go): Improve dynamic snippets error reporting #5099

Merged
merged 11 commits into from
Nov 5, 2024

Conversation

amckinney
Copy link
Contributor

@amckinney amckinney commented Nov 4, 2024

This adds the ErrorReporter into the dynamic snippet generator's core flow. With this, we collect all errors that occur during snippet generation, but continue to compose the snippet as best we can (leveraging go.TypeInstantiation.nop as needed).

In the future, we plan to supplement this validation with the help of JSON schema, but this at least builds out the hand-crafted solution that will be used by the regions of the snippet that won't be covered by JSON schema (e.g. query parameters, headers, etc).

Copy link

github-actions bot commented Nov 4, 2024

throw new Error(`Expected discriminant value to be a string but got: ${JSON.stringify(discriminantValue)}`);
this.errors.add({
severity: Severity.Critical,
message: `Expected discriminant value to be a string but got: ${JSON.stringify(discriminantValue)}`
Copy link
Member

Choose a reason for hiding this comment

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

${JSON.stringify(discriminantValue)} this might be a little tough for anyone to render in a pretty way -- maybe we just add the type of discriminantValue ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, happy to adjust the message here.

Copy link
Member

@dsinghvi dsinghvi left a comment

Choose a reason for hiding this comment

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

Approving to unblock, couple things crossed my mind:

  1. If we add JSON.stringify() in the error messages, its going to be hard for a consumer to render in a pretty way.
  2. It would be nice to decouple validation from generator specific code (i.e. python/typescript should not have to worry about dealing with validation)
  3. Reimplementing validation will never be as fully-featured as a third party lib that'll take things like min, max, regex into account as well (so still pushing json schema here)

Still great to have this since we are going from a world with no validation to real validation. Curious for your thoughts on the above!

@amckinney
Copy link
Contributor Author

... couple things crossed my mind:

  1. Good call - i'll see what I can do to remove all instances of JSON.stringify in the error messages before merging.
  2. Another good call - we should be able to do this by pushing more logic into the to-be-named AbstractDynamicSnippetsGeneratorContext that will be shared between all dynamic snippet generators. I'll keep this in mind once we start rolling out the next dynamic snippets generator.
  3. 100% agree, this won't replace the JSON schema validation we have planned.
    • I imagine the final flow will be receive request -> validate against JSON schema -> generate snippet and collect warnings along the way.
    • Many (if not all) of the "critical" errors we define here can be factored out and only warnings that can't be easily captured by JSON schema will remain from this abstraction.

@amckinney amckinney enabled auto-merge (squash) November 5, 2024 00:50
@amckinney amckinney merged commit b98d1e8 into main Nov 5, 2024
56 of 58 checks passed
@amckinney amckinney deleted the amckinney/dynamic/errors branch November 5, 2024 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants