hyperjump-io / json-schema-bundle

A tool for bundling JSON Schema documents
MIT License
14 stars 1 forks source link

Collecting external ids throws type error #5

Closed jonaslagoni closed 2 years ago

jonaslagoni commented 2 years ago

The very minimal schema I could come up that throws an error is this:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "$id": "file://./asyncapi.json",
  "additionalProperties": true
}

Stack trace:

Exception has occurred: TypeError: Cannot read property '0' of undefined
  at Object.collectExternalIds (/Users/lagoni/Documents/AsyncAPI/spec-json-schemas/tools/bundler/node_modules/@hyperjump/json-schema-bundle/lib/core.js:6:35)
    at Object.collectExternalIds (/Users/lagoni/Documents/AsyncAPI/spec-json-schemas/tools/bundler/node_modules/@hyperjump/json-schema-bundle/lib/keywords.js:19:19)
    at Object.collectExternalIds (/Users/lagoni/Documents/AsyncAPI/spec-json-schemas/tools/bundler/node_modules/@hyperjump/json-schema-bundle/lib/core.js:9:30)
    at Object.bundle (/Users/lagoni/Documents/AsyncAPI/spec-json-schemas/tools/bundler/node_modules/@hyperjump/json-schema-bundle/lib/index.js:30:10)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at async /Users/lagoni/Documents/AsyncAPI/spec-json-schemas/tools/bundler/index.js:30:20

I know it does not have any external references, but with the original file, there is plenty to take from. The usage code can be found here.

I tried locally to change the line in question to:

const collectExternalIds = (schemaUri, externalIds, ast, dynamicAnchors) => {
  if(ast[schemaUri] === undefined) return;
  const keywordId = ast[schemaUri][0];
  const id = splitUri(schemaUri)[0];
  Core.getKeyword(keywordId).collectExternalIds(schemaUri, externalIds, ast, { ...ast.metaData[id].dynamicAnchors, ...dynamicAnchors });
};

And I did not find any side effects, but I don't have the larger picture of whether this is a problem.

jdesrosiers commented 2 years ago

file://./asyncapi.json is not a valid URI. I'll have to look closer later, but I'm sure that's causing the problem. I really need to improve error messaging when bad URIs are used because that trips people up quite a bit.

You can't use . to reference the current directory like that. It would have to be file:///path/to/schema/asyncapi.json or just ./asyncapi.json. Of course neither are good choices. The former ties the path to a local machine and the latter is just redundant. Any time your schemas are in a context where there is a retrieval URI (which is always the case with file://), it's better to leave out the $id and use the retrieval URI for any references.

jonaslagoni commented 2 years ago

I don't think it is the URI unfortunately, earlier I used URL's such as http://example.com/schemas/xxxx which still resulted in this error, so I am not sure they are related? 🤔

jdesrosiers commented 2 years ago

I took a quick look at this. The URI is invalid, but you are correct that that isn't the problem here. The problem is with the additionalProperties implementation for collectExternalIds. It's an quick fix, but I won't have time for it until tomorrow night.

jdesrosiers commented 2 years ago

I found bugs in a couple other applicators as well. All have been fixed and a new version published. Please give it another try.

gavinbarron commented 2 years ago

So, I was just trying v0.1.2 and ran into this same error message.

Doing a little debugging and found the piece triggering the error in my case to be the "additionalProperties": false line in a schema.

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "$id": "pageProperties.schema.json",
  "type": "object",
  "properties": {
    "blockedScope": {
      "type": "array",
      "items": {
        "$ref": "scope.schema.json#/$defs/Scope"
      }
    },
    "darkTheme": {
      "type": "boolean"
    },
    "additionalProperties": false
  }
}
{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "$id": "scope.schema.json",
  "$defs": {
    "Scope": {
      "type": "string",
      "enum": [
        "user",
        "tools",
        "admin",
        "editor"
      ]
    }
  }
}

I don't understand this enough to know why this is happening or how to approach a fix, but I hope that this data might be helpful

jdesrosiers commented 2 years ago

@gavinbarron Are you sure you're running v0.1.2? I tried your example and it worked fine.

jdesrosiers commented 2 years ago

@jonaslagoni I checked out the repo you're working with and found a couple more issues that I missed (fixed in v0.1.3). It's been extremely helpful to have a real and complex schema to shake out the issues.

I suggest you avoid using $ids and rely on the retrieval URI to identify your schemas. This allows you to skip the step of loading all the schemas manually. They will be loaded from the filesystem automatically. That and using relative URIs in your references will also allow you to avoid having to change all of your URIs when you create a new version.

gavinbarron commented 2 years ago

@jdesrosiers I'll double check the example in the morning, I may have given you the one that was working :(, and get back to you. Pretty sure I was on v0.1.2 as the semver in package.json was ^0.1.2

jonaslagoni commented 2 years ago

Works for me, thanks!

I am using additionalProperties: false as well @gavinbarron, so gonna close the issue, feel free to reopen it if required 🙂