Skip to content

fix: pass loadSchema callback to pickSchemaDialect#1176

Merged
datho7561 merged 4 commits intoredhat-developer:mainfrom
elasticjava:fix/pickSchemaDialect-this-undefined
Feb 13, 2026
Merged

fix: pass loadSchema callback to pickSchemaDialect#1176
datho7561 merged 4 commits intoredhat-developer:mainfrom
elasticjava:fix/pickSchemaDialect-this-undefined

Conversation

@elasticjava
Copy link
Contributor

Summary

pickSchemaDialect is a module-level function but references this.loadSchema(s) (line 1338), where this is undefined in strict mode. This causes a TypeError when resolving schemas with custom (non-standard) $schema URIs that require loading the meta-schema to infer the base dialect.

Root cause: The function was introduced as part of #1166 (Support JSON Schema 2019 and 2020). It lives at module scope but uses this as if it were a class method.

Fix: Add an optional loadSchema callback parameter. The caller in resolveSchemaContent passes this.loadSchema.bind(this) through a closure variable. When loadSchema is not provided, the function returns undefined for unknown dialects instead of crashing.

Changed lines: +9/-3 in yamlSchemaService.ts

Reproduction

Any schema with a custom $schema URI that is not one of the well-known drafts triggers the bug:

{
  "$schema": "https://example.com/my-custom-meta-schema/v1",
  "type": "object",
  "properties": { "name": { "type": "string" } }
}

Also affects schemas embedding K8s definitions with "$schema": "http://json-schema.org/schema#" in nested $defs.

Test plan

  • New test pickSchemaDialect.test.ts with 4 test cases:
    • Custom $schema URI does not throw TypeError
    • Known draft URIs still work (draft-07, 2020-12)
    • Nested $schema on sub-definitions does not crash
  • All 1229 existing + new tests pass (0 failing, 5 pending — all pre-existing Windows-only skips)

@datho7561
Copy link
Contributor

Confirmed this is a problem. There's a build error caused by formatting that prettier doesn't like. I'll fix that locally and make sure that this fix works.

@datho7561
Copy link
Contributor

Once the build error is fixed, I think this'll be good to merge.

@datho7561 datho7561 added this to the 1.20.0 milestone Feb 10, 2026
@datho7561 datho7561 added the bug label Feb 10, 2026
@coveralls
Copy link

coveralls commented Feb 12, 2026

Coverage Status

coverage: 84.996% (+0.3%) from 84.671%
when pulling 90d4063 on elasticjava:fix/pickSchemaDialect-this-undefined
into 2aca934 on redhat-developer:main.

@datho7561
Copy link
Contributor

The tests seem to pass even without the fix; I'll try to figure out what's going on.

Holger Bartnick and others added 2 commits February 12, 2026 15:00
pickSchemaDialect is a module-level function but used `this.loadSchema(s)`
on line 1338, where `this` is undefined (strict mode). This caused a
TypeError when resolving schemas with custom (non-standard) $schema URIs
that require loading the meta-schema to infer the dialect.

The fix adds an optional `loadSchema` callback parameter. The caller in
`resolveSchemaContent` (a class method with proper `this` context) passes
`this.loadSchema.bind(this)` via a closure variable.

When `loadSchema` is not provided, the function returns `undefined` for
unknown dialects instead of crashing.
Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561 datho7561 force-pushed the fix/pickSchemaDialect-this-undefined branch from f343c1a to 80a3500 Compare February 12, 2026 20:00
- Add an await for a promise that was left dangling in order to properly
propagate the error.
- Remove other test suite; the test was passing with and without the fix.

Signed-off-by: David Thompson <davthomp@redhat.com>
It's timing out on macOS

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561
Copy link
Contributor

I'll try to figure out what's going on.

That turned into a day long sidequest

Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

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

I've modified the test case so that it fails without the fix, and left just the case where we check the broken behaviour.

@elasticjava Sorry for modifying your PR so much. I'd love another look at my work if you have time. If not, I'll go ahead and merge.

@datho7561 datho7561 merged commit 49c1d5a into redhat-developer:main Feb 13, 2026
4 checks passed
@elasticjava
Copy link
Contributor Author

I've modified the test case so that it fails without the fix, and left just the case where we check the broken behaviour.

@elasticjava Sorry for modifying your PR so much. I'd love another look at my work if you have time. If not, I'll go ahead and merge.

Thank you @datho7561 — really appreciate the improvements.

The new invalid-metaschema.test.ts is spot on:
much closer to the real-life flow where this actually breaks.

Also great catch on the missing await
I guess I can’t await to validate my schema now. 🙂

Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants