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

fix: ignore zod errors for json schema validations #5461

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

casoetan
Copy link

WHY are these changes introduced?

JSON schema extension contracts validations were being ignored. Custom CLI validations were still being used to validate json-schemas from the server. As we move to the CLI being a dumb client, respecting the server schema file will help make moving to the new world faster

WHAT is this pull request doing?

Switching from zod validations to json-schema validations for extensions with json-schemas

How to test your changes?

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor

github-actions bot commented Feb 25, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
76.01% (+0.03% 🔼)
9180/12078
🟡 Branches
71.24% (+0.12% 🔼)
4493/6307
🟡 Functions 75.54% 2393/3168
🟡 Lines
76.57% (+0.04% 🔼)
8671/11324
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
96.39% (-1.2% 🔻)
88.57% (-2.86% 🔻)
95.45% 100%

Test suite run success

2084 tests passing in 924 suites.

Report generated by 🧪jest coverage report action from 130aebc

@@ -56,7 +56,7 @@ export async function unifiedConfigurationParserFactory(
errorSet.add(key)
return true
})
if (zodParse.state !== 'ok' || errors.length > 0) {
if (jsonSchemaParse.state !== 'ok') {

Choose a reason for hiding this comment

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

shouldn't this just look at the length of errors? In this case if zodParse had errors, but jsonSchemaParse didn't it wouldn't return the error state?

Copy link
Author

Choose a reason for hiding this comment

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

We technically should not be using zodParse for json schema. I would like to remove it from this file, but I feel I may be missing some edge cases and reasoning behind why it's still used here.

Copy link

@toddjeff toddjeff left a comment

Choose a reason for hiding this comment

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

Seems simple enough, but calling out not having tests (I realize it's draft).

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.

2 participants