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

Consider not using $id in every property #72

Open
Relequestual opened this issue Aug 13, 2020 · 14 comments
Open

Consider not using $id in every property #72

Relequestual opened this issue Aug 13, 2020 · 14 comments
Labels
help wanted Extra attention is needed

Comments

@Relequestual
Copy link

I'm assuming this config option results in the use of $id for every property.
This is considered not best pratice... it's not required either.

@bravo-kernel
Copy link
Collaborator

bravo-kernel commented Oct 2, 2020

The absolutePaths option is used to switch between absolute baseURI and relative baseURI as can be seen in this test so:

  • https://alt3.io
  • vs /

@Relequestual Relequestual changed the title Consider defaulting absolutePaths to false Consider not using $id in every property Oct 2, 2020
@Relequestual
Copy link
Author

OK, so it doesn't disable it completely.
In which case, I've amended the issue title.

I don't know why schema generators DO this. It's not needed.
What do you see people gain from this?

@bravo-kernel
Copy link
Collaborator

Thanks for the follow-up. I'm open for all suggestions but not sure that I understand the issue yet. Are you talking about the $id field below, as taken from this generated full example at
https://github.com/alt3/sequelize-to-json-schemas/blob/master/examples/json-schema-v7.md ?

    "STRING": {
      "$id": "https://api.example.com/properties/STRING",
      "type": "string",
      "default": "Default value for STRING"
    },

@bravo-kernel
Copy link
Collaborator

bravo-kernel commented Oct 2, 2020

I'm guessing here but I see (and empathize) with two approaches:

  1. responding with $id: makes it easier for the consuming clients as they don't have to concatenate/construct the $id themselves
  2. responding without $id: for the developer that does not want to provide redundant information/want to keep his response size as small as possible

If you are indeed after number 2... I'm perfectly fine with a PR that adds that as a config option.

@Relequestual
Copy link
Author

responding with $id: makes it easier for the consuming clients as they don't have to concatenate/construct the $id themselves

Why do you believe they WANT an $id for each property value?

If you are indeed after number 2... I'm perfectly fine with a PR that adds that as a config option.

😅Sure. I have the JSON Schema spec to work on though. I'm suggesting it should be the default.
This is actually invalid for draft 2019-09 and newer.

@bravo-kernel bravo-kernel added the help wanted Extra attention is needed label Oct 2, 2020
@bravo-kernel
Copy link
Collaborator

Since we're all busy let's agree to keep this ticket open so it does not get lost. I've added the Help Wanted label, perhaps someone is willing to jump in.

Again, really appreciate your input and was not aware that we were generating invalid schema. Any chance you can provide use with a link to some information about that? Might help motivate.

@Relequestual
Copy link
Author

Ha. Sure!

It's valid for draft-07, which is what (hopefully) most people will be using now.

Draft 2019-09 and newer don't allow $id to be used in a subschema*.
The new $anchor keyword allows for providing a NAME for a subschema which can then be referenced, much like an anchor in HTML.

In terms of information, we haven't got much beyond the spec itself: http://json-schema.org/draft/2019-09/json-schema-core.html

Appendix A might help: http://json-schema.org/draft/2019-09/json-schema-core.html#rfc.appendix.A

  • We have a notion of embedded schema resources for a bundled schema document. When a schema references multiple resources, sometimes it's helpful to bundle them together into one schema. This is the only time when a schema should use $id, to identify a individual schema resource embedded within another document. This has a clearer explanation in the upcoming draft 2020-NN version of JSON Schema.

@bravo-kernel
Copy link
Collaborator

bravo-kernel commented Oct 2, 2020

Awesome, much appreciated. Hope this does not sound silly but is there a change that that 2019-09 version will eventually turn into a new separate draft name like draft-0x ?

If so, I think:

I would be willing to put in the effort to get that new strategy up and running.

@Relequestual
Copy link
Author

Draft 2019-09 is effectivly draft-08.

We decided to change the naming convention because using draft-NUMBER confused people when we publish IETF drafts under different names.

You could do that. Again, my advice is DO NOT include $id in subschemas.

I go back to my question: Why do you believe they WANT an $id for each property value?
What purupose does it serve? Was it just becasue another popular library did this?

@bravo-kernel
Copy link
Collaborator

Was it just becasue another popular library did this?

Yes, 100% true. I made this lib because the alternatives, at the time, were either outdated, missing functionality or did not support Sequelize 6. Since I'm no schema specialist (at all) I took them for inspiration and used it as the basis to study the strategy pattern.

Why do you believe they WANT an $id for each property value?

I do not believe anything but can imagine they would appreciate that. I could think up of more reasoning but IMO that's not really relevant. Your point makes sense. I'm just saying I understand the current approach as well.

Anyway, I really appreciate your input, it shows you actually care about what's out there in the wild which is great by itself 💯 .

I'm about to jump into the weekend now so will not be back until tomorrow. I still have a tiny doubt about whether I truly understand which exact points would require changing but perhaps you can bring up the patience to explain it to me some day, perhaps even via a discord chat or sth.

@Relequestual
Copy link
Author

Yeah, I am familiar with several libraries which do similar... it's a pain.
I don't understand WHY they do that, but hey.

Yeah, I have a vested interest in this project specifically, potentially. Now maybe not.

If you want to drop by our slack server, it's pretty friendly, and covered in most timezones.
I'm likely going to afk next week for a while, but others there are knowledgeable too!

@bravo-kernel
Copy link
Collaborator

@Relequestual I am preparing a major update and have just added the draft 2019-09 strategy here #88.

To my surprise, running the generated schema with the $id field still passes the AJV validation. You mentioned this would no longer be supported in 2019-09. Has something changed or can you confirm that $id is still allowed?

ps: I am still going to make the $id field optional, just want to make sure I understand.

@bravo-kernel
Copy link
Collaborator

@Relequestual the 2019-09 strategy now generates a schema:

  1. with the $id field for only the schema itself
  2. without the $id field for any of the properties

Sample output can be seen here. Does this match what you were after? https://gist.github.com/bravo-kernel/b07ffcf120a5543e26e531de58c389cb

@bravo-kernel
Copy link
Collaborator

bravo-kernel commented Dec 18, 2021

Added configuration option renderIdField, defaulting to false meaning all schemas will now be rendered without $id by default. Details in this commit.

Exception.. the topmost $id field for the schema itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants