Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: extend bundling to create another schemas without $id #298

Merged
merged 25 commits into from
Apr 25, 2023

Conversation

derberg
Copy link
Member

@derberg derberg commented Dec 13, 2022

⏰ BREAKING CHANGE ⏰

@asyncapi/specs package has now different structure. You no longer access specs directly by version name but there are 2 properties:

  • schemas - with previous schemas object
  • schemasWithoutIds - with new set of schemas that do not contain $id

For more details check out readme updates about $id and examples on how to use the package


Fixes #247
Fixes #367

This is a workaround implementation. I'm not proud of it but it is better than the current situation.

Our AsyncAPI JSON Schema is created following JSON Schema best practices and also requirements of https://github.com/hyperjump-io/json-schema-bundle - which means we have beautiful $id and our references are based on these, which makes schema unreadable for humans (like me that completely do not understand $id and $ref concept, but my issues could be ignored and I could learn) and are not understood by some tools (like https://github.com/redhat-developer/vscode-yaml that VSCode YAML autocompletion and validation is based on, and this is a problem for us).


  • This PR also contains additional generated schemas, under schemas dir, that are alternative, without $id
  • New schemas are now used in "All schemas file" that we use in schema store.
  • New schemas are exposed on JS/GO packages level
  • I tested newly generated schemas with asyncapi file on local, and autocompletion and validation works

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

Just small suggestions.

tools/bundler/index.js Outdated Show resolved Hide resolved
tools/bundler/index.js Outdated Show resolved Hide resolved
tools/bundler/index.js Outdated Show resolved Hide resolved
jonaslagoni
jonaslagoni previously approved these changes Dec 19, 2022
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

LGTM 👍

So my thoughts the current $id and $ref situation is that we have solved the issue in the bundled schemas now, which is perfect 👌

If however, people want to use a single schema, instead of the bundled one, we still support dynamically fetching those schemas remotely. Which I technically think is good, whether there is a use-case for it is another thing 😆 Cause I don't see it, just from a technical standpoint.

So if we don't see that as a "thing", we could remove the $id yea, but I would probably wait and see tbh 🙂

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

Two suggestions :)

package.json Outdated Show resolved Hide resolved
tools/bundler/index.js Show resolved Hide resolved
@magicmatatjahu
Copy link
Member

So if we don't see that as a "thing", we could remove the $id yea, but I would probably wait and see tbh 🙂

I would leave it at that, even if we currently have no use case for that.

@sonarcloud
Copy link

sonarcloud bot commented Dec 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@derberg
Copy link
Member Author

derberg commented Dec 19, 2022

@magicmatatjahu @jonaslagoni the problem is that once I leave $id then parser will not work with my changes because of errors like:

{
message: "can't resolve reference #/definitions/contact from…d http://asyncapi.com/definitions/2.5.0/info.json", 
missingRef: 'http://asyncapi.com/definitions/2.5.0/info.json#/definitions/contact', 
missingSchema: 'http://asyncapi.com/definitions/2.5.0/info.json'
}

because ajv, when it sees $id, wants to resolve $refs against it.

If however, people want to use a single schema, instead of the bundled one, we still support dynamically fetching those schemas remotely

what do you mean by single schema? like https://github.com/asyncapi/spec-json-schemas/blob/master/definitions/2.5.0/channels.json ? the $ref that we use there is absolute so I guess $id is not necessary. Unless you meant something different

omg I don't think I will ever like $id

@jonaslagoni
Copy link
Member

@derberg yea you're right, forgot that the $id changes where relative references are looking. For the bundled schemas you need to remove them 🙂

Thought you meant to remove them from the individual definitions.

@derberg
Copy link
Member Author

derberg commented Dec 20, 2022

Thought you meant to remove them from the individual definitions.

Actually, why not? I seriously do not get why we need it there 😆
example:

{
  "type": "object",
  "propertyNames": {
    "type": "string",
    "format": "uri-template",
    "minLength": 1
  },
  "additionalProperties": {
    "$ref": "http://asyncapi.com/definitions/2.5.0/channelItem.json"
  },
  "$schema": "http://json-schema.org/draft-07/schema#",
  "$id": "http://asyncapi.com/definitions/2.5.0/channels.json"
}

if $ref is anyway an absolut URL, what is $id for....asking for a friend 🙏🏼

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Dec 20, 2022

If the schema is single, the $id is not needed, but if it is standalone, it is better to leave it as it determines the id of this single schema.

EDIT: That $id is used to bundle schemas in proper way.

@char0n
Copy link
Collaborator

char0n commented Dec 20, 2022

Actually, why not? I seriously do not get why we need it there

$id keyword primary role is to establish JSON Schema identity. It's an important and sadly, very complex concept.

There are three concepts in play:

base URI - is established by resolving resolved $id against retrieval URI
retrieval URI - is URI where the JSON Schema has been retrieved (protocol based or local file system)
$id - is established by resolving the current $id with all parents $id, up to root JSON Schema $id

EDIT: That $id is used to bundle JSON Schemas in proper way.

Exactly. It's only RECOMMENDED practice to use $id when no referencing of JSON Schemas happens, but it's NEEDED to have when referencing JSON Schemas. Simply said, $id gives us ability to uniquely identify the JSON Schema, even if the retrieval URI of the JSON Schema is different from the $id. Example: JSON Schema can be physically served from different HTTP URLs, but if it contains the same $id it's considered to be the same JSON Schema. You want to bundle it just once.

@derberg
Copy link
Member Author

derberg commented Dec 21, 2022

@char0n thanks a lot for detailed explanation

Example: JSON Schema can be physically served from different HTTP URLs, but if it contains the same $id it's considered to be the same JSON Schema. You want to bundle it just once.

now looking at my example above, reference points to http://asyncapi.com/definitions/2.5.0/channelItem.json where $id is different that the $id of schema that there reference comes from

...

ok, I leave $id in source files and remove it from bundled schema

@jonaslagoni
Copy link
Member

@derberg any update here, or are you just waiting for reviews? 🙂

@derberg
Copy link
Member Author

derberg commented Feb 9, 2023

ok, so now $id is removed from resulting schema and autocompletion works like a charm when I check manually:

Screenshot 2023-02-09 at 18 17 31
Screenshot 2023-02-09 at 18 17 46

but in Parser it fails with:

Error: can't resolve reference http://asyncapi.com/definitions/2.6.0/schema.json from id #

but....but there is no http://asyncapi.com/definitions/2.6.0/schema.json reference 😭

I thought I need "$id": "http://asyncapi.com/schema-store/2.6.0.json" id in schema root, but that was not a lucky guess

I ran out of ideas, need a break

@derberg
Copy link
Member Author

derberg commented Mar 29, 2023

@jonaslagoni @smoya I changed the package exports, have a look:

  • prefix for major release updated
  • description of PR updated

jonaslagoni
jonaslagoni previously approved these changes Mar 29, 2023
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

If you dont want 2 major version changes within 3 months, you could merge this into next-major-spec. But just a suggestion 🙂

Copy link
Member Author

derberg commented Mar 29, 2023

I do not mind having major every month if that is required 😃 we should not be afraid of major releases in libraries

@smoya
Copy link
Member

smoya commented Mar 29, 2023

If you dont want 2 major version changes within 3 months, you could merge this into next-major-spec. But just a suggestion 🙂

I agree. And keep advocating for that over long living PR's targeting master

@smoya
Copy link
Member

smoya commented Mar 29, 2023

I do not mind having major every month if that is required 😃 we should not be afraid of major releases in libraries

I'm sure releasing few many major versions in a short period of time can be taken as negative because few reasons. Completely valid for a new discussion but still worth to share IMHO:

  • Can create the perception of instability, like the package is not mature enough.
  • Contributors can feel disconnected from the repo when realizing few major versions happened without them to even realize.
  • Annoying users to update too many times in order to keep up with big changes.

Having said that, its up to you to do one thing or another atm

smoya
smoya previously approved these changes Mar 30, 2023
@derberg
Copy link
Member Author

derberg commented Mar 30, 2023

We have March now, releasing major. Next maybe, I say maybe, will be in June. So we have at least 3 months different. This is not often. Let us not forget it is not a big project. This is super simple package, that only exposes json files, not parser or any complicated beast.

Can create the perception of instability, like the package is not mature enough

the person who has no sins should be the first to throw a stone 😄

have you ever "rejected" to use a package that has many major releases?

  • we use Jest everywhere (current release - 29)
  • not to mention react
  • I can go one and one

anyway, thanks for approval 🙏🏼 😄

@derberg
Copy link
Member Author

derberg commented Mar 30, 2023

@fmvilas I need your approval as you requested changes in the past

@dalelane @char0n have a look as we will release major here

1 similar comment
@derberg
Copy link
Member Author

derberg commented Apr 11, 2023

@fmvilas I need your approval as you requested changes in the past

@dalelane @char0n have a look as we will release major here

fmvilas
fmvilas previously approved these changes Apr 18, 2023
@derberg derberg dismissed stale reviews from fmvilas and smoya via a270f3f April 19, 2023 09:00
@derberg derberg requested review from fmvilas and smoya April 19, 2023 09:01
@derberg
Copy link
Member Author

derberg commented Apr 19, 2023

@smoya please approve again, I had to solve merge conflict with readme cause by new breaking changes section. No more changes

@derberg
Copy link
Member Author

derberg commented Apr 24, 2023

@smoya @fmvilas one approve needed

@sonarcloud
Copy link

sonarcloud bot commented Apr 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@derberg
Copy link
Member Author

derberg commented Apr 25, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit d950c38 into asyncapi:master Apr 25, 2023
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 4.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Change the way schemas are exported Vscode unable to resolve parts of the schema
8 participants