fix(schema): validate extension files against their entity metaschema - #488
Conversation
The metaschema validation spec validated every .json file under schema/extensions/ against extension.schema.json. That metaschema requires description, caption, name and uid, and sets additionalProperties to false, so it only ever describes extension.json itself. An extension mirrors the layout of the core schema directory, so its dictionary.json, domains/, skills/, modules/, objects/ and profiles/ files are governed by the same metaschemas as their core counterparts. Validating them against extension.schema.json made every one of them fail, which meant task test:schema could not pass for any in-tree extension. Extension discovery now mirrors the schema server: a directory containing extension.json is an extension root and its subdirectories belong to it (server/lib/schema/json_reader.ex, find_extensions/2). Each file is then validated against the metaschema for its entity type. Files in locations with no defined metaschema are reported as warnings rather than failures. Repositories with no schema/extensions directory are unaffected. Signed-off-by: Narahara Chari Dingari <chari@sciencephalon.com>
ramizpolic
left a comment
There was a problem hiding this comment.
LGTM, thank you for a valuable contribution @ncdingari!
|
The test fix in commit 1 looks correct — discovery matches The example extension in commit 2 doesn't load cleanly, though. Baseline
oasf/schema/extensions/example/dictionary.json Lines 4 to 16 in 3f4695a
oasf/schema/extensions/example/README.md Lines 39 to 45 in 3f4695a Minor: a directory under Given the example needs two fixes plus the README correction, splitting the commits as you offered seems worthwhile. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
3f4695a to
7b2badb
Compare
|
@akijakya — all three reproduce, all three are fixed, and I've taken the split. This PR is now the test fix alone. Force-pushed to The example extension is on ReproductionFresh clone at
1. Missing
|
akijakya
left a comment
There was a problem hiding this comment.
Verified, and this is byte-identical to what I said stands on its own — 7b2badb1 is the same SHA as before the force-push, which is only possible if content, parent, message and timestamp are all unchanged. One file in the diff, and go test -count=1 at head gives ok schema.
I loaded the example branch and measured rather than reading the diff:
| Claim | Measured |
|---|---|
mix test with the extension |
366 passed, 0 failures |
| domain uid | 9980101 |
| category | example_verticals ("Example Verticals") |
| name enum | example/example_verticals/example_cold_chain_logistics |
module data |
object_t → example/example_telemetry_data |
All four match the new README exactly, and both failures are gone. You fixed the mechanism rather than the symptom, and documenting the two traps that made these fail quietly — the unqualified extends and the self-typed dictionary entry — is more than I asked for. Your read of the find_category_class/2 gap matches mine, and I agree it shouldn't ride along in either PR.
One consequence of the split I should have flagged when I suggested it: this PR's new spec is dormant on main. With no schema/extensions/ directory it takes the "does not exist" warning path and cannot fail, so CI can't demonstrate the fix — the evidence lives on a branch that isn't up for review. Not a defect, and the guard activates the moment anyone adds an extension, which is the point. But it does argue for your side-by-side option.
On your four questions:
- Open the example PR now. The duplicated commit collapses to nothing when this merges, and it closes the gap above.
find_category_class/2: separate issue please. It will hit every extension author and deserves its own visibility rather than being buried in an example.- The
SCHEMA_EXTENSIONCI run belongs in the example PR — here it would have nothing to load. - Take the undiscovered-directory warning, in this PR. Your reasoning about not diverging from the server's definition of an extension is sound, but a warning doesn't redefine anything; it just reports a directory no root claimed.
Approving. Happy to merge once you've decided on the warning — it's small enough that I'd rather it land here than as a follow-up.
…tensions A directory containing JSON files but no extension.json is not registered as an extension by the schema server, so nothing in it is read and nothing in it is validated. Misspelling the filename therefore produced a green suite with no indication that an entire directory had been skipped. Report those directories through the existing AddWarning mechanism. This does not change what counts as an extension -- discovery still mirrors find_extensions/2 in server/lib/schema/json_reader.ex -- it only reports a directory that no extension root claimed. Directories that merely group extensions hold no JSON of their own and are not reported. The check runs before the no-extensions-found early return, so it also covers a tree that has one valid extension alongside a misspelled one. Signed-off-by: Narahara Chari Dingari <chari@sciencephalon.com>
|
All four done. 1. Undiscovered-directory warning — taken, in this PR.
Two details worth flagging:
2. 3. I checked it can fail rather than assuming it — reverting the example's 4. Example PR opened now: #489. It's rebased onto On the dormant-spec point — you didn't need to flag it, but I'm glad you did, because it's the sharper version of what this PR is. Worth stating plainly: on Current state:
Both commits here signed off. |
fix(schema): validate extension files against their entity metaschema
Problem
schema/test/schema_test.govalidates every.jsonfile underschema/extensions/againstmetaschema/extension.schema.json:{Dir: filepath.Join(schemaDir, "extensions"), Schema: filepath.Join(metaschemaDir, "extension.schema.json")},The match is a recursive path-prefix match, so it reaches every file in every subdirectory of an extension. But
extension.schema.jsonrequiresdescription,caption,nameanduidand setsadditionalProperties: false— it describesextension.jsonand nothing else.An extension mirrors the layout of the core
schemadirectory, as described inCONTRIBUTING.md. Itsdictionary.json,domains/,skills/,modules/,objects/andprofiles/files are governed by the same metaschemas as their core counterparts. Validating them againstextension.schema.jsonmakes every one of them fail, sotask test:schemacannot pass for any in-tree extension.The bug is currently invisible because
schema/extensions/does not exist onmain. It surfaces as soon as anyone follows the documented extension workflow.Reproduced against
mainby dropping a minimal extension intoschema/extensions/example/:Change
extensionsis removed from the flat directory list and gets its own spec. Discovery mirrors the schema server: a directory containingextension.jsonis an extension root and its subdirectories belong to it — the same rule asfind_extensions/2inserver/lib/schema/json_reader.ex. Each file is then validated against the metaschema for its entity type:extension.jsonextension.schema.jsondictionary.jsondictionary.schema.jsonskills/,domains/,modules/class.schema.jsonobjects/object.schema.jsonprofiles/profile.schema.jsonFiles in locations with no defined metaschema are reported through the existing
AddWarningmechanism rather than failing the suite, so an extension carrying additional JSON does not break the build.Verification
Run from
schema/test:ok schema— 6 of 6 specs passmain's test file + the same extensionschema/extensions/directoryok schema— unchanged behaviour for the current treeThe Go diff introduces no
gofmtdrift beyond what is already onmain.Notes
This PR is the test fix alone. The reference example extension it was originally bundled with is now a separate PR, stacked on this one, since it is only mergeable once this lands.
One known gap, called out for the record: a directory under
extensions/that lacksextension.jsonis not discovered, so its files are neither validated nor warned about — a contributor who misspells the filename sees a green suite. That matches the schema server's own behaviour (it registers an extension only on findingextension.json), so this PR reproduces it deliberately rather than inventing a second discovery rule. Note that before this change such a directory was not validated either: every file in it hard-failed againstextension.schema.json. Happy to add a warning for undiscovered directories underextensions/if you would like the signal.The commit is signed off per the DCO.