fix: correct return type of getSchemaType

[mlewando]

I noted that the return type of getSchemaType is not correct. It looks to be derived from

export const SCHEMA_TYPES = ["string", "number", "integer", "boolean", "null", "array", "object"]

but the actual declaration was

export function getSchemaType(node: SchemaNode, data: unknown): keyof typeof SCHEMA_TYPES | undefined

The keyof typeof SCHEMA_TYPES is actually "length" | "push" | "pop" | "map" ... which is wrong.

I made the SCHEMA_TYPES a const array and created new simple type:

export type SchemaType = (typeof SCHEMA_TYPES)[number];

to fix that issue.

[sagold]

This is out of scope of this merge request. I am wondering, why are you adding ref-resolution here?

If we were to add this:

• getNode would need data as second argument to fully use its api
• I would prefer to use node.if.resolveRef here to be explicit

[mlewando]

the short story here is that first I realised that types of getSchemaType are off, so I fixed them and created this PR, later while working on my product I've seen that the function is also not working correctly on oneOf nodes with refs, so I thought this PR is close enough to include this as well... If you prefer I can split this into two PRs (or we can keep it as single one if that's ok)

as for node.if.resolveRef - I think it should work... I pushed the changes about that, are those ok?
and if you won't mind - what's the real benefit of using resolveRef here? I think that getNode looks to be way more generic way of "get me whatever there is underneath" right? under the hood it's also looks to handle more edge cases than resolveRef 🤔

[mlewando-cp]

there is one more issue here:

• with resolveRef I have errors in current tests

 Exception during run: TypeError: Cannot read properties of undefined (reading 'length')
    at resolveRecursiveRef (/projects/json-schema-library/src/draft2019-09/keywords/$ref.ts:108:26)
    at Object.resolveRef (/projects/json-schema-library/src/draft2019-09/keywords/$ref.ts:75:26)
    at getSchemaType (/projects/json-schema-library/src/utils/getSchemaType.ts:132:66)
    at Object.getData (/projects/json-schema-library/src/draft2019-09/methods/getData.ts:174:31)
    at Object.getData (/projects/json-schema-library/src/SchemaNode.ts:305:37)
    at /projects/json-schema-library/src/draft2019-09/methods/getData.ts:200:56
    at Array.forEach (<anonymous>)
    at Object.object (/projects/json-schema-library/src/draft2019-09/methods/getData.ts:193:42)
    at Object.getData (/projects/json-schema-library/src/draft2019-09/methods/getData.ts:175:38)
    at Object.getData (/projects/json-schema-library/src/SchemaNode.ts:305:37)
    at Object.getData (/projects/json-schema-library/src/draft2019-09/methods/getData.ts:161:36)
    at Object.getData (/projects/json-schema-library/src/SchemaNode.ts:305:37)
    at /projects/json-schema-library/src/draft2019-09/methods/getData.ts:126:39
    at Array.forEach (<anonymous>)
    at Object.getData (/projects/json-schema-library/src/draft2019-09/methods/getData.ts:125:27)
    at Object.getData (/projects/json-schema-library/src/SchemaNode.ts:305:37)
    at compileSchema (/projects/json-schema-library/src/compileSchema.ts:74:58)
    at /projects/json-schema-library/src/tests/utils/runTestCases.ts:26:39
    at Array.forEach (<anonymous>)
    at Suite.<anonymous> (/projects/json-schema-library/src/tests/utils/runTestCases.ts:22:22)
    at Object.create (/projects/json-schema-library/node_modules/mocha/lib/interfaces/common.js:148:19)
    at context.describe.context.context (/projects/json-schema-library/node_modules/mocha/lib/interfaces/bdd.js:42:27)
    at runTestCase (/projects/json-schema-library/src/tests/utils/runTestCases.ts:21:5)
    at /projects/json-schema-library/src/tests/utils/runTestCases.ts:103:30
    at Array.forEach (<anonymous>)
    at Suite.<anonymous> (/projects/json-schema-library/src/tests/utils/runTestCases.ts:103:14)
    at Object.create (/projects/json-schema-library/node_modules/mocha/lib/interfaces/common.js:148:19)
    at context.describe.context.context (/projects/json-schema-library/node_modules/mocha/lib/interfaces/bdd.js:42:27)
    at runAllTestCases (/projects/json-schema-library/src/tests/utils/runTestCases.ts:89:5)
    at Object.<anonymous> (/projects/json-schema-library/src/tests/spec/draft2019-09.spec.ts:10:16)

• with getNode everything works correctly

I don't have a root cause of the error yet

[sagold]

I am unable to reproduce issues based your tests:
On your branch, using

const type = getSchemaType(node.allOf[i].resolveRef(), data);
if (type) {
    return type;
}

works just fine on my end.

getNode does a couple of things in addition to resolving refs, like

• parsing the json-pointer to find the node we are looking for and
• creating additional an extended response type for consumption.

We can save unnecessary work and simply use resolveRef. Especially since this is a core function we should be explicit on the actions we use and work on the simpler api.

[mlewando]

hmmm... maybe I'm running tests somehow wrongly... I'm just executing yarn test

but I don't know how it could work tbh...
I see quite clear bug in src/draft2019-09/keywords/$ref.ts in resolveRef function (line 72). It has optional path param, that I'm not providing to it (as it's optional and I don't need it). Then on line 75 it's calling resolveRecursiveRef with that optional path as second parameter, which is not optional in resolveRecursiveRef declaration (src/draft2019-09/keywords/$ref.ts:102-103), which obviously results in error on line 108 where it tries to do history.length with history being undefined.
I fixed that bug by providing empty path in src/draft2019-09/keywords/$ref.ts and then I used resolveRef in getSchemaType. Now it should all be good

[sagold]

Hey there, thank you for your merge request. Highly appreciated!

[mlewando]

hi @sagold - any chance to merge this?

[sagold]

Hey mlewando.

Yes, we need some tests to check if it behaves as expected and keep it working through future changes.

I can help you with that next week, but maybe you can post some of your use cases?

Cheers,
Sascha

[mlewando]

@sagold I've added test cases and created an issue #90 to keep things documented

[sagold]

Hey Mateusz,

thank you so much for your work and your contribution to this library. I think this feature will save some headaches for other users. I suggest we merge this request!

I will make some changes after merging and before publishing them

• move tests from issues to getSchemaType.test.ts as this is then tracked as a tested feature
• replace getNode by resolveRef and commit the changes unless I hit issues

[mlewando]

@sagold - thanks a lot :)

• move tests from issues to getSchemaType.test.ts as this is then tracked as a tested feature
• replace getNode by resolveRef and commit the changes unless I hit issues

those I just did, so you should be free to just merge it 🤞