-
Notifications
You must be signed in to change notification settings - Fork 130
refactor: Zod-based config loader #998
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #998 +/- ##
==========================================
- Coverage 48.51% 48.22% -0.29%
==========================================
Files 51 52 +1
Lines 2092 2117 +25
Branches 241 241
==========================================
+ Hits 1015 1021 +6
- Misses 1040 1052 +12
- Partials 37 44 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@JamieSlome @06kellyjac This is now ready for review, Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Would be great if you could add some background as to why we need to replace the existing json-schema
with zod
.
Merging with the latest main
and adding the latest config entries would be much appreciated too 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I like about using json-schema
is that it shows a description of each config entry, so you know what each option is supposed to toggle/modify. Apparently, zod
can do this by using .meta()
or .describe()
. source
Zod schema's aren't the easiest to read and duplicate the content of the config schema from which the config reference is generated... There are docs generators that will turn Zod schemas into markdown, e.g. https://github.com/matejchalk/zod2md (not used this - so not a recommendation), which might provide a solution. You can also generate zod schemas from JSON-schema (which would also negate the need to dual maintain the config schema and zod schemas) using quicktype: https://app.quicktype.io/#l=typescript-zod That would be worthwhile if the the zod validation provides an advantage over using ajv or similar to validate directly using the JSON schema - I don't know what that advantage would be... |
@kriswest I originally requested this change so that we could enforce stronger typing in some places like the database sink: let sink: any;
if (config.getDatabase().type === 'mongo') {
sink = require('./mongo');
} else if (config.getDatabase().type === 'fs') {
sink = require('./file');
} I also thought loading the config into something strongly-typed would be more reliable. An example is with the "authentication": [
{
"type": "local",
"enabled": true
},
{
"type": "ActiveDirectory",
"enabled": false,
"adminGroup": "",
"userGroup": "",
"domain": "",
"adConfig": {
"url": "",
"baseDN": "",
"searchBase": ""
}
},
{
"type": "openidconnect",
"enabled": false,
"oidcConfig": {
"issuer": "",
"clientID": "",
"clientSecret": "",
"callbackURL": "",
"scope": ""
}
}
], Strong typing would make sure that each value is truly valid during compilation, not runtime. Another use case for this is preventing things from blowing up given "certain" config rules. For example, prior to #963, enabling multiple auth methods would result in runtime errors. Other "unexpected" combinations or values will also cause runtime errors instead of failing fast.
I love the idea of generating documentation for the schema! It'd be great UX-wise, since the current |
A few more thoughts on alternatives:
Fair enough - what do you think about using zod schemas generated from the json-schema to achieve that? One reason to handle it that way would be to continue using the config reference docs generated from the schema (i.e. https://git-proxy.finos.org/docs/configuration/reference - note gaps in those docs are due to gaps in the schema itself). You can also generate standard Typescript types with quicktype, which come with convert functions to read the JSON and validate it. We use it extensively on the FDC3 project, e.g.:
FDC3s use of schemas is much more complex - with only one schema file here things would be much simpler to add. Something I'd be happy to contribute if it was desired. The same tooling can generate zod schemas instead of TS types and convert functions.
You can express some quite complex restrictions in json-schema, then validate at runtime with ajv or a generated convert class (as above) and fail fast (exit with a complaint and detail as to what failed validation).
The existing reference that gets generated: https://git-proxy.finos.org/docs/configuration/reference isn't using all the feature available (in json-schema / json-schema-for-humans) yet to due to a lack of descriptions and examples in the schemas (and has quite a few missing sub-property definitions). If you check out the preview on #972 you can see what I propose to add to the authentication schema and what that looks like: https://deploy-preview-972--endearing-brigadeiros-63f9d0.netlify.app/docs/configuration/reference |
Replace JSON-Schema with Zod for config validation and default injection
Add typed
config
export,loadConfig()
,validate()
andsetConfigFile()
Update
index.ts
to use the new loader and pass the validated config.