Update CHANGELOG.md and Release Notes with 2.9's breaking changes

[nevans]

Quoting @anakinj in #616 (comment):

Im going to close this one as we try our best to follow sematic versioning to the documented public apis. If there are some suggestion on how to be better at communicating what are internal interfaces and what public. Please shout out...

So, I'm opening a new ticket for my suggestions. 😉

I'm personally okay with projects that don't strictly adhere to Semantic Versioning. I have issues with the way the standard was written--with a few (significant) tweaks, I'd probably love it. But I'll never commit to using it myself.

Regardless of whether or not the project uses Semantic Versioning or any other compatibility policy, what I really want to see is a list of any "Breaking Changes" in the CHANGELOG.md and Release Notes (for projects that use those). Bonus points (so few projects do this!): update the CHANGELOG and Release Notes with a "Known Issues" list for any significant bugs (aka: accidental breaking changes) that were fixed in a later release.

It's okay to update these docs months after the release was made. It'll be appreciated by someone... it may save them hours of debugging time! These should be living documents.

---

As for the actual release notes, quickly skimming through #605 and the bugfix since 2.9.0, I see the following:

### Breaking Changes
* `JWT::Verify` has been removed.
  * Replace `JWT::Verify.verify_claims`
    with `JWT::Claims.verify`.
* `JWT::ClaimsVerifier` has been removed.
  * Replace `ClaimsValidator.new(payload).validate!`
    with `Claims::Numeric.new(payload).verify!`.

### Known Issues
* `verify_iss` and `verify_aud` crash when no expected values are passed.
  _Fixed in v2.9.1 by [#619](https://github.com/jwt/ruby-jwt/pull/619)._

If you wanted to add something about "these constants were not intended to be used as part of the public API", that seems reasonable to me. But that's not really necessary, IMO.

[nevans]

Separately, to your concern that you expressed here: #616 (comment)

Thanks for pointing this out. We try to be as backwards compatible as possible and try to obey the documented behaviour. The challenge comes with the interfaces that is intended to be internal as the one you pointed out. As there are no ways to prevent the use of internals its a bit challenging to keept track of them. To preserve compatibility for all interfaces and classes in the gem would be a nightmare.

I think you're already halfway towards the solution: undocumented interfaces are already inherently a little bit unstable (IMO). I'd recommend one or more of the following:

• Completely remove them from rdoc and/or yard:
  • The simplest and safest way to do this is by adding :nodoc: (for rdoc) and/or @private (for yard).
  • But, in some cases, you can also use private_constant (for classes, modules, and other constants) or private (for methods). Both yard and rdoc will honor that and omit it the code from documentation.
• Add a documenting comment that explicitly calls out the interface as internal. Something like: "for internal use only. The API is subject to change without warning". Which won't be seen by rdoc or yard, if you also changed the visibility, but it should be seen by anyone who wants to use that code directly from their project. You might also add "Please submit an issue or PR if you think a publicly supported API would be useful for this." 🙂
• When changing APIs that should be private or internal, deprecate_constant and warn("DEPRECATED: Use ... instead.", category: :deprecated, uplevel: 1) are also great options. You can maintain a temporary compatibility shim/adapter for some number of versions (e.g until the next x.y.0 release) or time (e.g: for a year).

[nevans]

Also, for what it's worth, after looking through #605, I do like your refactoring, @anakinj. (Selfishly) I would have liked it better with a temporary backward compatibility layer (printing deprecation warnings). 😉 But we all have limited time and energy for this stuff! And adding deprecation adapters (and release notes) for every potential change can be draining.

[anakinj]

Lovely suggestions and appreciate you putting time on thinking and writing about this @nevans .

I think tagging the "internal" apis for rdoc/yard would be a nice way to more clearly document what are intended to be publicly used.

Would also be glad adding (also in retrospect) notes about the breaking changes in 2.9 to the internal APIs to the changelog.

As an addition to that I would also be happy to see a layer for keeping backwards compatibility.

[anakinj]

Im about to bring back the classes (probably as-is) and adding some deprecation warnings to those. Now Im a little puzzled on how to deal with suggesting alternatives.

Would not like to suggest anyone to use the internals until there is official support for those.

[nevans]

@anakinj I'm only able to put so much time into thinking and writing about this because I've already been dealing with it myself: I'm currently in the middle of trying to figure out a few final points before the net-imap 0.5.0 release. How much of a breaking change is too much? What should go through an extended deprecation period and what changes should just switch over? 🙂

And I'm sorry for prodding you into the extra work (bringing back the older API)! I'm am using JWT, but my code never used those APIs directly and I've already upgraded all of the gems that were using those APIs (this morning). As far as suggesting alternatives, honestly, I think that what you did in #624 is fine for now (not directly suggesting any alternatives).

On the other hand, I think that your Claims API is already simple (and tested) enough that I'd personally be comfortable declaring it as "officially supported". There's really just the following, right?

• JWT::Claims::Name.new(**claim_opts).verify!(context: in JWT::Claims::VerificationContext, **)
  • A set of classes, each with a small number of initializer kwargs, all implementing a simple single method interface.
• JWT::Claims.verify!(payload, **options)
  • And a single method to simplify calling most of the claims classes, with an almost one-to-one mapping between its verify_* kwargs and the claims classes.

That isn't too much to support, IMO. Since all of the JWT::Claims::#{name} classes can be accessed via JWT::Claims.verify!, you could even document that as the only public API and document all of the others as internal. Even if the JWT::Claims::* classes are documented as official, you could still recommend that people should prefer to use them via JWT::Claims.verify!.

I would suggest:

• adding JWT::Claims.private_constant :VERIFIERS
• changing the options param on JWT::Claims.verify! to **options (keyword params)

For me, the major sticking point would be if any of the class or keyword parameter names are ambiguous or misnamed. E.g: is there a better name (or maybe a good alias) for JWT::Claims::Numeric? But if the names aren't right and you find the right name: slap a deprecated_constant on the class, a warn in the VERIFIERS[name] proc, and remove it a year or so later. It's only a dozen or so lines of code to support until then. Same if you decide that the kwargs names are wrong, or have the wrong semantics.

[anakinj]

I actually managed to make most of the APIs backwards compatible with a bit of magic. Still a bit work to be done but doable.

About officially supporting claim validations I think is something that I could work. There are some changes in the horizon already for the current API so think I need to think of a way to keep it compatible The current vision is to drop the verify_* options and just activate the claim validation with the target claim, now the approach in #621 would break compatibility again, so think I need to iterate a bit more on that one.

So the API would be something like:

::JWT::Claims.verify!(token, :jti)
::JWT::Claims.verify!(token, sub: ['subject'])
::JWT::Claims.verify!(token, :jti, sub: ['subject'])

Would also be interesting to hear about those gems that were using those APIs? Would be valuable to know the users a bit better to understand the struggles.

Really appreciate your input, super motivating to react when there are some concrete suggestions to support you.

[anakinj]

Just a little update.

The removed APIs are back via #624 and #626 is aiming at a claims API that can be supported.

[anakinj]

Think I got the mess sorted now and released 2.9.2 with the old API compatibility for all the changed APIs. Added a list of deprecations that will be removed in 3.0 to the changelog.

For 2.10 there will be some improvements to the documentation based on your comments also in that version the deprecation warnings will be added to code, decided not to add them just yet to avoid spamming.

[nevans]

@anakinj Looking great! 👍🏻