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

Fix #74: add "deprecated" keyword #737

Merged
merged 3 commits into from
May 19, 2019
Merged

Fix #74: add "deprecated" keyword #737

merged 3 commits into from
May 19, 2019

Conversation

philsturgeon
Copy link
Collaborator

@philsturgeon philsturgeon commented May 15, 2019

There is often talk about getting OpenAPI in line with JSON Schema, but this is one instance where JSON Schema can be brought in line with OpenAPI, with a new"deprecated" keyword.

Screen Shot 2019-05-15 at 14 51 10

Just a simple true/false (default false) for this keyword, and it means the thing might be going away sometime. Easy.

Justification

There have been talks of all sorts of complex forms of deprecation, including deprecatedSince and deprecateVersion and deprecatedVersion over on OpenAPI, and this lead to all sorts of confusion for me in what JSON Schema should do.

I have thought through this on and off for months, and it's become clear to me that since JSON Schema currently has no concept of versions or dates or any sort of timeline, there is no reason to start trying to add a whole new dimension just for a deprecated keyword.

A lot of people want to know when something was deprecated, and a lot of other people want to know when it will go away. These are two incredibly valid use cases, and they do not need to be handled directly in the spec.

Let's take the example of API documentation as a use case. If you have API reference documentation up on your website, and you have made a general statement that deprecated things will stick around for 3 months, so you need the data to show when something was deprecated. If you are using evolution then you are marking properties deprecated over time, and when you publish a new version of your specs your documentation hub can notice the addition of the deprecated keyword, and the documentation hubs changelog will have this noticed added in.

Lots of folks are starting to implement changelogs in their docs for OpenAPI, for example my employer Stoplight.io, a competitor who I've actually forgotten about whilst writing this, and open-source tools like Azure/openapi-diff.

So, it seems like diffing is a good way to see a trail of history for those who are interested, and the future? If you aren't interested in generic statements like "Deprecations will last three months" then you can use runtime solutions like Deprecated / Sunset HTTP headers to mark whole URLs as deprecated in runtime.

If anyone is curious, here is how deprecation in general are handled in HTTP APIs using HTTP Sunset as a runtime option, and OpenAPI deprecated: trueas a build time option, for evolution and deprecation of whole global versions too

https://blog.apisyouwonthate.com/api-evolution-for-rest-http-apis-b4296519e564

Sunset lets you say when an entire URI is going away, but this does not help folks who want to deprecate a single property on a specific day. In my experience deprecation are rarely on the exact day anyway. People flop out a random day, try and pester people to stop using the thing, track the usage, then extend the date because people are still using the darn thing.

For this reason I don't think we need another property keyword deprecatedUntil or sunsetAt or future looking date. Put simply, if an application notices as deprecated keyword, warnings should go into bug trackers and the person investigating that bug can try to figure out how long they have to get
it done, if its a Priority 1 or Trivial.

Questions

  1. Do we want reason too? GraphQL has this, they do deprecated(reason: String), but this is mostly as they have nowhere lese to put text related to a property. JSON Schema has description and $comment, so maybe it is not so important.

  2. Do we want a date? Some folks want one or the other and so some other folks recommend having both, which makes either an object or a bunch of interdependent keywords which would be a pain in the butt.

Note: I'll improve this PR once I've got xml2rfc installed. Please give feedback on the writing so far, I dont think it needs too many words at it but I might be missing something.

@philsturgeon philsturgeon changed the title Fix \#74: add "deprecated" keyword Fix #74: add "deprecated" keyword May 15, 2019
@philsturgeon philsturgeon requested review from handrews, Relequestual and awwright and removed request for handrews May 15, 2019 13:05
@lidio601
Copy link

I wonder if it would be better to have a Javadoc-like or PHP doc-like @deprecated property.

If it's define as a String instead of boolean one can also specify the reason/migration specs.

It worked fine with my JSON Schema compiler to PHP classes as I exposed this string on top of a compiled PHP class like this, so that also the IDE recognized and outlined it and suggest the new class name.

/**
 * @deprecated reason blabla
 */
class SomeClass {
}

@philsturgeon
Copy link
Collaborator Author

@lidio601 hey, I hear ya, some people want reason and think thats more important than anything else, some folks want dates and think thats more important than anything else, some people think we should have a mixture and therefore it becomes an object to make room for any combination. Those are the known camps so far.

I usually just update the description field to let people know things are deprecated, just like in a CLI application:

Screen Shot 2019-05-15 at 22 31 39

GraphQL cannot not do this because their type system doesnt have a general description keyword, so they kinda need to put the deprecated reason in place. Seeing as we have a lot more options, I don't think we need to add even more options?

@Julian
Copy link
Member

Julian commented May 16, 2019

I am probably personally in the "object" camp, since yeah I think both date and reason are important, neither more than the other. Maybe we use true for the simple version but allow it to be an object as well.

But I also think you're right that we shouldn't bikeshed too hard, so if we start getting into a rabbithole in this thread or otherwise, I think more important is to just do the simple thing and I'd retract my above (and be fine with just a boolean).

Copy link
Contributor

@handrews handrews left a comment

Choose a reason for hiding this comment

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

Generally looks great.

@lidio601 @Julian we were all set to include this in draft-07 but disagreements of exactly this sort blocked it.

So the issue has sat open for the entire year and a half (sigh) since then, and no one would step up to figure out a better solution and build a case for it.

As far as I'm concerned, that ship has sailed. It's time to ship the next draft, it's advantageous to match OpenAPI in this particular case as it is well-established, and other things can be handled by future extensions.

@philsturgeon I would like to see that one detail tweaked that I commented on but otherwise I think this is good to go.

is going to be removed in the futuree.
</t>
<t>
An instance document that is marked as "deprecated" for the entire document
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add something about how this is done by setting "deprecated": true in the root schema. Just reading through this without thinking of the history, it sounds a bit like marking an instance document as deprecated is a rather separate thing.

@Julian
Copy link
Member

Julian commented May 16, 2019

I'm fine with that personally -- we can always make further changes later, I don't think this'd negatively affect that. Thumbs up.

@gregsdennis
Copy link
Member

Firstly, I agree with @handrews that we need to focus on getting draft-08 (or 2019-whatever) out. This isn't really the time to be proposing additions.

That said, I question the need for this within JSON schema in a general sense. This feels like an application-level concern, not something that should be put into the tool. If someone needs it, they can extend JSON schema with their own vocabulary that contains this keyword. Implementations should support extension vocabularies with this draft.

@handrews
Copy link
Contributor

@gregsdennis just as with readOnly and writeOnly this annotates data with information about its relationship to the data source.

I am also making a blanket proclamation: I stated in Issue #74 that this (the single boolean form) would be what we would implement in DECEMBER 2017 and asked for objections. It is now MAY 2019 with zero objections in the intervening time.

The time for objections came and went, several times over. I'm being a bit of an ass here but I am immensely frustrated at the amount of effort I have put into trying to get people to participate, bumping this out of a release when it was very much wanted by many people, having zero participation FOR A YEAR AND A HALF and suddenly everyone wants to talk about options and whether it's really needed. This issue was not hard to find. It gets brought up a lot, actually.

The only thing I'm interested in on this PR is whether @philsturgeon has correctly and clearly written a spec for the decision made back in 2017 that never received a single objection in the intervening time. As a boolean annotation, implementation support is about as trivial as it gets.

I let bikeshedding take this out of the last draft and it is one of my bigger regrets with draft-07. It gets requested regularly either on the slack or in comments on #74 or in duplicate issues filed. It's going in.

@Anthropic
Copy link
Collaborator

@handrews well said. I can agree with that.

@philsturgeon
Copy link
Collaborator Author

Alright! We are sticking to deprected: bool and now we are just chatting about wording.

@handrews great spot, I have updated in the latest commit. I had a typo and the word "operation" snuck in there too copying stuff from OpenAPI. 😅

@Relequestual can you rethumb if you like it?

The more thumbs the merrier.

@awwright
Copy link
Member

If "deprecated" has a value of boolean true, it indicates that applications SHOULD refrain from usage of the declared operation. It MAY mean the property SHOULD refrain from usage of the declared property.

"property" implies it exists in an object—while this is usually true, it might be misleading. Would "instance" work better?

A root schema containing "deprecated" with a value of true indicates the entire root schema MAY be removed in the future.

Why "entire root schema" over "entire document"?

The idea is that, in theory, implementations could keep track of which properties are deprecated, and if the client tries to read them, automatically generate a warning.

So if the root schema specifies "deprecated", then the client shouldn't be using the document at all.

@philsturgeon
Copy link
Collaborator Author

@awwright I'm trying to say both. If they mark a property as deprecated, dont use it. If an entire document is deprecated, dont use any properties. etc.

@philsturgeon philsturgeon merged commit 9b834f2 into master May 19, 2019
@philsturgeon philsturgeon deleted the deprecated branch May 19, 2019 11:23
@philsturgeon
Copy link
Collaborator Author

If we need to clarify wording it’s not going to break implementation at this point it’s judt helping users understand intent. Thanks for the feedback y’all!

@philsturgeon
Copy link
Collaborator Author

Also thanks to https://stoplight.io for letting me work on this!

@handrews
Copy link
Contributor

@philsturgeon Normally we leave non-trivial spec changes open for 14 days 😕 While I often find this frustrating, it is necessary for anyone who happens to be distracted for a few days. And if people with merge access skip the waiting period, it becomes difficult to justify our process to other contributors.

Also, I would have appreciated the chance to review the wording changes that I requested.

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

Successfully merging this pull request may close these issues.

8 participants