Switch to AJV for validating JSON Schema #9191
Conversation
somebody1234
added
CI: No changelog needed
Well I thought it was just a suggestion 😅 but thanks for implementing this, I think that will indeed be right approach! |
| @@ -148,7 +156,9 @@ export default function DataLinkInput(props: DataLinkInputProps) { | |||
| value={typeof value === 'string' ? value : ''} | |||
| size={1} | |||
| className={`rounded-full w-40 px-2 bg-transparent border leading-170 h-6 py-px disabled:opacity-50 read-only:opacity-75 read-only:cursor-not-allowed ${ | |||
| jsonSchema.isMatch(DEFS, schema, value) ? 'border-black/10' : 'border-red-700/60' | |||
| error.assert<ajv.ValidateFunction>(() => AJV.getSchema(path))(value) | |||
There was a problem hiding this comment.
I think I'd refactor this error.assert<...>(() => AJV.getSchema(path)) pattern into some helper function, as it is used all over the place.
e.g. function getKnownSchema
The last remaining task is to fix `setValue` (if needed)
| /** Return a human-readable name representing a schema. */ | ||
| function getSchemaNameHelper(schema: object): string { | ||
| if ('title' in schema) { | ||
| return String(schema.title) | ||
| } else if ('type' in schema) { | ||
| return String(schema.type) | ||
| } else if ('$ref' in schema) { | ||
| const referencedSchema = jsonSchema.lookupDef(DEFS, schema) | ||
| return referencedSchema == null ? '(unknown)' : getSchemaName(referencedSchema) | ||
| } else if ('anyOf' in schema) { | ||
| const members = Array.isArray(schema.anyOf) ? schema.anyOf : [] | ||
| return ( | ||
| members.flatMap(object.singletonObjectOrNull).map(getSchemaName).join(' | ') || '(unknown)' | ||
| ) | ||
| } else if ('allOf' in schema) { | ||
| const members = Array.isArray(schema.allOf) ? schema.allOf : [] | ||
| return members.flatMap(object.singletonObjectOrNull).join(' & ') || '(unknown)' | ||
| } else { | ||
| return '(unknown)' | ||
| } | ||
| } |
There was a problem hiding this comment.
This function seems copy-pasted, can we get it just in one place?
Screen.Recording.2024-02-28.at.12.20.39.mov
|
|
wow that is funky. i'll have to look into it |
|
@PabloBuchu weird, i can't repro |
| @@ -193,7 +197,7 @@ export default function AssetSearchBar(props: AssetSearchBarProps) { | |||
| document.removeEventListener('keydown', onKeyDown) | |||
| document.removeEventListener('keyup', onKeyUp) | |||
| } | |||
| }, [setQuery]) | |||
| }, [setQuery, /* should never change */ modalRef]) | |||
There was a problem hiding this comment.
I'm not sure if I understand this comment, but it may be just because I'm not familiar enough with the codebase.
There was a problem hiding this comment.
yeah the comment is a little awkward, not sure how useful it is in practice. the array here is a dependency list, so the function is re-run when any of the dependencies change. the comment indicates that it is likely a mistake if th value changes - it is required to be in the dependency list because of our linter setup.
There was a problem hiding this comment.
Looks good to me, but I did not review JSONSchemaInput.tsx very deeply as I'm not familiar enough with the framework to provide constructive review, at least not yet.
|
@somebody1234 weird I am also no longer able to reproduce 🤷 qa 🟢 |
Pull Request Description
As
requestedsuggested by @radeusgdImportant Notes
None
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
The documentation has been updated, if necessary.Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.If GUI codebase was changed, the GUI was tested when built using./run ide build.