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

Adds Deprecation Support #57

Merged
merged 7 commits into from
Jun 15, 2018
Merged

Adds Deprecation Support #57

merged 7 commits into from
Jun 15, 2018

Conversation

davetron5000
Copy link
Contributor

@davetron5000 davetron5000 commented Jun 1, 2018

This allows developers to indicate if an endopint is deprecated, when it will be gone, and to then make it explicitly gone.

Suppose you have a kittens-service and its endpoint GET /api/floof_monsters is going to be deprecated on May 4, 2019 (but you still want to keep GET /api/floof_monsters/:id). You'd use deprecated method from Stitches::Deprecation like so:

class Api::V1::FloofMonstersController << ApiController
  # ApiController includes the Stitches::Deprecation module already

  def index
    deprecated gone_on: "2019-05-04" do
      render json: { floof_monsters: [ FloofMonster.all ] }
    end
  end

  def show
    render json: { floof_monster: FloofMonster.find(params[:id]) }
  end
end

This will cause the floof monsters index to behave exactly the same, with two exceptions:

  • The Sunset: HTTP header will be set to the date given to deprecated, in the proper HTTP format
  • An extra log message is emitted about this endpoint being requested. This allows you to search for logs when getting consumers off of your deprecated endpoint

You can assert this is working in an acceptance test:

get "/api/floof_monsters" do
    example "GET all floof monsters" do
      expect(self).to show_deprecation(gone_on: "2019-05-04")
      expect(status).to eq(200)
      # and all other existing expectations about this endpoint's behavior…
    end
end

This expectation will check that the Sunset header is being added, but, more importantly, will fail after the retire date, telling you that you have a deprecated endpoint in production after the advertised retirement date. The endpoint will still work as before (this seems least surprising).

To then retire your endpoint, use gone! from the same Stitches::Deprecation module:

class Api::V1::FloofMonstersController << ApiController

  def index
    gone!
  end

  def show
    render json: { floof_monster: FloofMonster.find(params[:id]) }
  end
end

The have_deprecation matcher will succeed, but of course the rest of your test will fail since the endpoint is now not doing anything. To fix that, replace your test with be_gone:

get "/api/floof_monsters" do
    example "GET all floof monsters" do
      expect(self).to be_gone
      # all other expectations removed
    end
end

The reason to support the endpoint and have it return 410 vs. just removing it and responding with 404 is that if there are any stragglers still using the endpoint after the retirement date, it will be more obvious what is wrong than would be indicated by a 404.

Lastly, this also includes a migration to add the Stitches::Deprecation module to your ApiController if you are already using stitches:

> bin/rails g stitches:add_deprecation

Shipping This

  • If all good, I'll release an RC of this and try it in client-service
  • I'll use that to deprecate the endpoint
  • I'll then enhance our internal (private, sorry) api client to log this header and make sure that end is looking good
  • If so, I'll release this gem as 3.6.0
  • Add documenttion of this feature to this repo's wiki

@@ -0,0 +1,4 @@
{

Choose a reason for hiding this comment

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

Should this file be in your ~/.gitignore ?

# Example:
#
# def show
# deprecated on: "2019-04-09" do

Choose a reason for hiding this comment

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

The actual argument name is to_be_retired_on

@shawnacscott
Copy link

This is awesome, I'm excited for it!

One thing I'm curious is the naming of the different bits in here. I see that in different places we use deprecated, retired, and gone to describe different parts of this functionality. Is there any reason (maybe a naming conflict?) not to choose one set of language to use consistently? For me, something like:

  • deprecated to_be_deprecated_on:
  • deprecated!
    would be more clear and more obviously a side effect of the same batch of code.

@davetron5000
Copy link
Contributor Author

@shawnacscott yeah, "deprecated" I'm using to mean "this will still work, but it's going to be retired", meaning it's deprecated the second we use the deprecated method. "retired" I'm using to mean "the act of actually turning it off for good" and "gone" mean "it is now retired". As I type that out, I now think gone! should be retired! (or to_be_retired_on should be will_be_gone_on).

@brettfishman
Copy link
Contributor

@shawnacscott I totally understand your point. One thing to note about "gone!" is that it is an actual thing in the HTTP spec: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/410. For this reason, the gone! doesn't bother me.

@davetron5000
Copy link
Contributor Author

@brettfishman @shawnacscott I think both of your comments are right on. In 5e4f787 I changed it to be_gone: and have updated the PR description to show that. "retired" is no longer a concept so it's just:

  • deprecation - still works, but is gonna be gone on a date in the Sunset header
  • gone - it's not here anymore, HTTP 410/Gone

Copy link

@soulcutter soulcutter left a comment

Choose a reason for hiding this comment

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

This looks great. I like the naming you landed on.

@davetron5000 davetron5000 merged commit b56890c into master Jun 15, 2018
@alimac alimac deleted the deprecation branch December 30, 2019 19:42
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.

4 participants