Add Deprecation headers for legacy rest endpoints #7686

clevinson · 2020-10-27T01:03:38Z

Description

closes: #7636

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

x/auth/client/rest/query.go

codecov · 2020-10-27T01:09:58Z

Codecov Report

Merging #7686 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #7686   +/-   ##
=======================================
  Coverage   54.12%   54.13%           
=======================================
  Files         611      611           
  Lines       38631    38639    +8     
=======================================
+ Hits        20909    20917    +8     
  Misses      15590    15590           
  Partials     2132     2132

aaronc

Why you need to add a header for every endpoint? Can't you just set them globally in server/ or at least in the RegisterRESTRouters of each handler? There should be a way to just attach middleware.

aaronc · 2020-10-27T01:26:11Z

https://pkg.go.dev/github.com/gorilla/mux@v1.8.0#Router.Use

x/bank/client/rest/query.go

x/evidence/client/rest/query.go

x/slashing/client/rest/query.go

x/upgrade/client/rest/query.go

aaronc · 2020-10-27T01:57:49Z

Swagger is out of date already and explicitly unmaintained afaik

clevinson · 2020-10-27T02:06:09Z

@aaronc then why did we go through the work of updating it with all the GRPC Gateway routes?

Currently most of the legacy routes are marked as "Deprecated" in the swagger as well. I think @anilcse was largely the one updating all of this.

What do you mean by explicitly unmaintianed?

aaronc · 2020-10-27T02:33:17Z

Wait are we merging the grpc gateway swagger with the old swagger? We shouldn't be doing that. Early on I was told that old swagger is unmaintained and we shouldn't bother with it.

anilcse · 2020-10-27T02:59:11Z

Wait are we merging the grpc gateway swagger with the old swagger? We shouldn't be doing that. Early on I was told that old swagger is unmaintained and we shouldn't bother with it.

Yes, we merged legacy rest swagger and gRPC gateway swagger docs in #7246 . The only reason was to have a single endpoint for swagger docs. It's just a config change, we can just remove the rest swagger entry from client/docs/config.json and regenerate swagger docs.

clevinson · 2020-10-27T03:05:52Z

Just switched to using Route.Use with a middleware func, but that seems to be affecting the entire server (including tendermint rpc & /node_info) as the rtr is passed in by reference in RegisterRESTRoutes...

I'll take a look more at this tomorrow and resolve.

amaury1093

(including tendermint rpc & /node_info)

Those should be deprecated too imo. If we want to keep them as REST, we should do so via grpc-gateway.

x/auth/client/rest/rest.go

alessio

Left just a tiny comment. Otherwise looks good

aaronc

LGTM

aaronc · 2020-10-28T14:18:26Z

client/rest/rest.go

+// Deprecation headers to a http handler
+func addHTTPDeprecationHeaders(h http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Deprecation", "true")


Do we also want to set Sunset and or Warning headers?

Sunset headers requires a specific date of sunset, I can add Warning headers with essentially the same information, but a bit more verbose.

aaronc · 2020-10-28T17:40:31Z

types/module/module.go

@@ -110,8 +110,9 @@ func (bm BasicManager) ValidateGenesis(cdc codec.JSONMarshaler, txEncCfg client.

 // RegisterRESTRoutes registers all module rest routes
 func (bm BasicManager) RegisterRESTRoutes(clientCtx client.Context, rtr *mux.Router) {
+	r := rest.WithHTTPDeprecationHeaders(rtr)


Actually shouldn't be be doing this module by module? Putting this in BasicManager means that other projects with other modules will have their REST routes auto-deprecated. That's not really what we want right?

I thought this was what we wanted, as this entire process for generating REST routes will be deprecated at the SDK level in future releases, no?

You're probably right... That would make SDK maintenance easier. Still this deprecation goes to clients. RegisterRESTRoutes should be deprecated for module developers. Module developers may decide not to deprecate their own REST routes for clients and instead do that wiring on their own.

Sorry- i'm not following exactly what you mean. Is this still something you are wanting changed? Or do you now think the current implementation is correct in the context of this PR?

Yes It still should be their decision. So I would prefer we do module by module.

REST routes for clients and instead do that wiring on their own.

This should be on by default to let the developers and users to know and prepare for that.
At some point we will remove the REST API.

If we need to coustomze it at the SDK level, then we can add a parameter to the RegisterRESTRoutes function:

RegisterRESTRoutes(clientCtx client.Context, rtr *mux.Router, deprecateREST bool)

client/rest/rest.go

clevinson · 2020-10-28T20:41:07Z

Those should be deprecated too imo. If we want to keep them as REST, we should do so via grpc-gateway.

Thoughts on if we should also be deprecating these routes? IMO these aren't as crucial to deprecate as they aren't made redundant by any new grpc-gateway routes. Would appreciate some other opinions on if these should be included or not (cc @aaronc, @anilcse, @alexanderbez)

anilcse · 2020-10-28T20:48:04Z

Thoughts on if we should also be deprecating these routes? IMO these aren't as crucial to deprecate as they aren't made redundant by any new grpc-gateway routes. Would appreciate some other opinions on if these should be included or not (cc @aaronc, @anilcse, @alexanderbez)

May be we should introduce respective gRPC gateway routes and mark these as deprecated.

aaronc · 2020-10-28T20:49:30Z

May be we should introduce respective gRPC gateway routes and mark these as deprecated.

Agreed, we ideally want clients to have a single RPC layer they have to deal with that covers pretty much everything

clevinson · 2020-10-29T02:31:14Z

(including tendermint rpc & /node_info)

Those should be deprecated too imo. If we want to keep them as REST, we should do so via grpc-gateway.

@amaurymartiny I wrote this up in an issue (#7719) so this discusison can happen separately, and tackled later depending on the path forward.

@aaronc you still have Changes requested status. Can you mark as approved or elaborate what still needs changing from your side?

aaronc · 2020-10-29T02:33:52Z

@clevinson I responded stating that I still think other modules should get to decide if they deprecate. You didn't make the change or respond to my comment.

* add deprecation headers for legacy rest endpoints * add deprecation headers for missing tx routes * rm handler-level deprecation headers * switch to middleware Route.Use method for setting deprecation Headers * set deprecation headers using subrouter * cleanup gofmt * goimports * Update client/rest/rest.go * update deprecation headers to be set on each module individually Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

add deprecation headers for legacy rest endpoints

923ad0a

clevinson commented Oct 27, 2020

View reviewed changes

x/auth/client/rest/query.go Outdated Show resolved Hide resolved

add deprecation headers for missing tx routes

f2e4a8a

aaronc reviewed Oct 27, 2020

View reviewed changes

clevinson commented Oct 27, 2020

View reviewed changes

rm handler-level deprecation headers

17c4cf9

switch to middleware Route.Use method for setting deprecation Headers

abd6170

clevinson added 2 commits October 27, 2020 15:43

set deprecation headers using subrouter

b7e828d

cleanup gofmt

abccbb2

clevinson marked this pull request as ready for review October 28, 2020 01:14

clevinson requested review from alessio and alexanderbez as code owners October 28, 2020 01:14

clevinson added the backport/0.40.x (Stargate) label Oct 28, 2020

clevinson requested review from amaury1093, blushi and robert-zaremba October 28, 2020 01:37

robert-zaremba approved these changes Oct 28, 2020

View reviewed changes

amaury1093 reviewed Oct 28, 2020

View reviewed changes

fedekunze approved these changes Oct 28, 2020

View reviewed changes

alessio reviewed Oct 28, 2020

View reviewed changes

x/auth/client/rest/rest.go Outdated Show resolved Hide resolved

alessio approved these changes Oct 28, 2020

View reviewed changes

aaronc approved these changes Oct 28, 2020

View reviewed changes

aaronc requested changes Oct 28, 2020

View reviewed changes

clevinson commented Oct 28, 2020

View reviewed changes

client/rest/rest.go Show resolved Hide resolved

clevinson added 2 commits October 28, 2020 13:01

goimports

7d509fd

Update client/rest/rest.go

3e39d45

clevinson mentioned this pull request Oct 29, 2020

Add gRPC + grpc-gateway routes as alternative to legacy tendermint REST endpoints #7719

Closed

update deprecation headers to be set on each module individually

abe10d8

amaury1093 approved these changes Oct 29, 2020

View reviewed changes

aaronc approved these changes Oct 29, 2020

View reviewed changes

aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 29, 2020

Merge branch 'master' into clevinson/legacy-deprecation-headers-7636

2791a54

mergify bot merged commit 536eb68 into master Oct 29, 2020

mergify bot deleted the clevinson/legacy-deprecation-headers-7636 branch October 29, 2020 11:37

clevinson mentioned this pull request Oct 29, 2020

[backports]: v0.40.0-rc2 Cherry-picks #7734

Closed

45 tasks

amaury1093 mentioned this pull request Jul 20, 2021

Remove the legacy REST API #7517

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Deprecation headers for legacy rest endpoints #7686

Add Deprecation headers for legacy rest endpoints #7686

clevinson commented Oct 27, 2020 •

edited by aaronc

Loading

codecov bot commented Oct 27, 2020 •

edited

Loading

aaronc left a comment

aaronc commented Oct 27, 2020

aaronc commented Oct 27, 2020

clevinson commented Oct 27, 2020 •

edited

Loading

aaronc commented Oct 27, 2020

anilcse commented Oct 27, 2020

clevinson commented Oct 27, 2020 •

edited

Loading

amaury1093 left a comment

alessio left a comment

aaronc left a comment

aaronc Oct 28, 2020

clevinson Oct 28, 2020

aaronc Oct 28, 2020

clevinson Oct 28, 2020

aaronc Oct 28, 2020 •

edited

Loading

clevinson Oct 29, 2020

aaronc Oct 29, 2020 •

edited

Loading

robert-zaremba Oct 29, 2020

robert-zaremba Oct 29, 2020

clevinson commented Oct 28, 2020

anilcse commented Oct 28, 2020

aaronc commented Oct 28, 2020

clevinson commented Oct 29, 2020

aaronc commented Oct 29, 2020

Add Deprecation headers for legacy rest endpoints #7686

Add Deprecation headers for legacy rest endpoints #7686

Conversation

clevinson commented Oct 27, 2020 • edited by aaronc Loading

Description

codecov bot commented Oct 27, 2020 • edited Loading

Codecov Report

aaronc left a comment

Choose a reason for hiding this comment

aaronc commented Oct 27, 2020

aaronc commented Oct 27, 2020

clevinson commented Oct 27, 2020 • edited Loading

aaronc commented Oct 27, 2020

anilcse commented Oct 27, 2020

clevinson commented Oct 27, 2020 • edited Loading

amaury1093 left a comment

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

aaronc Oct 28, 2020

Choose a reason for hiding this comment

clevinson Oct 28, 2020

Choose a reason for hiding this comment

aaronc Oct 28, 2020

Choose a reason for hiding this comment

clevinson Oct 28, 2020

Choose a reason for hiding this comment

aaronc Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

clevinson Oct 29, 2020

Choose a reason for hiding this comment

aaronc Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

robert-zaremba Oct 29, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 29, 2020

Choose a reason for hiding this comment

clevinson commented Oct 28, 2020

anilcse commented Oct 28, 2020

aaronc commented Oct 28, 2020

clevinson commented Oct 29, 2020

aaronc commented Oct 29, 2020

clevinson commented Oct 27, 2020 •

edited by aaronc

Loading

codecov bot commented Oct 27, 2020 •

edited

Loading

clevinson commented Oct 27, 2020 •

edited

Loading

clevinson commented Oct 27, 2020 •

edited

Loading

aaronc Oct 28, 2020 •

edited

Loading

aaronc Oct 29, 2020 •

edited

Loading