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.

Sign up for GitHub

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

Add Deprecation headers for legacy rest endpoints #7686

Merged
merged 10 commits into from
Oct 29, 2020
Merged

Add Deprecation headers for legacy rest endpoints #7686

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
923ad0a
add deprecation headers for legacy rest endpoints
clevinson Oct 27, 2020
f2e4a8a
add deprecation headers for missing tx routes
clevinson Oct 27, 2020
17c4cf9
rm handler-level deprecation headers
clevinson Oct 27, 2020
abd6170
switch to middleware Route.Use method for setting deprecation Headers
clevinson Oct 27, 2020
b7e828d
set deprecation headers using subrouter
clevinson Oct 27, 2020
abccbb2
cleanup gofmt
clevinson Oct 28, 2020
7d509fd
goimports
clevinson Oct 28, 2020
3e39d45
Update client/rest/rest.go
clevinson Oct 28, 2020
abe10d8
update deprecation headers to be set on each module individually
clevinson Oct 29, 2020
2791a54
Merge branch 'master' into clevinson/legacy-deprecation-headers-7636
mergify[bot] Oct 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/docs/swagger-ui/swagger-ui-bundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41773,4 +41773,4 @@
}, o.resolve = i, e.exports = o, o.id = 1058
}])
});
//# sourceMappingURL=swagger-ui-bundle.js.map
//# sourceMappingURL=swagger-ui-bundle.js.map
2 changes: 1 addition & 1 deletion client/docs/swagger_legacy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2595,4 +2595,4 @@ definitions:
total:
type: array
items:
$ref: "#/definitions/Coin"
$ref: "#/definitions/Coin"
26 changes: 26 additions & 0 deletions client/rest/rest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package rest

import (
"net/http"

"github.com/gorilla/mux"
)

// addHTTPDeprecationHeaders is a mux middleware function for adding HTTP
// 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")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

w.Header().Set("Link", "<https://docs.cosmos.network/v0.40/interfaces/rest.html>; rel=\"deprecation\"")
clevinson marked this conversation as resolved.
Show resolved Hide resolved
h.ServeHTTP(w, r)
})
}

// WithHTTPDeprecationHeaders returns a new *mux.Router, identical to its input
// but with the addition of HTTP Deprecation headers. This is used to mark legacy
// amino REST endpoints as deprecated in the REST API.
func WithHTTPDeprecationHeaders(r *mux.Router) *mux.Router {
subRouter := r.NewRoute().Subrouter()
subRouter.Use(addHTTPDeprecationHeaders)
return subRouter
}
7 changes: 4 additions & 3 deletions types/module/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@ package module
import (
"encoding/json"

"github.com/grpc-ecosystem/grpc-gateway/runtime"

"github.com/gorilla/mux"
"github.com/grpc-ecosystem/grpc-gateway/runtime"
"github.com/spf13/cobra"
abci "github.com/tendermint/tendermint/abci/types"

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/rest"
"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@aaronc aaronc Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@aaronc aaronc Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

for _, b := range bm {
b.RegisterRESTRoutes(clientCtx, rtr)
b.RegisterRESTRoutes(clientCtx, r)
}
}

Expand Down
4 changes: 3 additions & 1 deletion x/auth/client/rest/rest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package rest

import (
"github.com/cosmos/cosmos-sdk/client/rest"
clevinson marked this conversation as resolved.
Show resolved Hide resolved
"github.com/gorilla/mux"

"github.com/cosmos/cosmos-sdk/client"
Expand All @@ -24,7 +25,8 @@ func RegisterRoutes(clientCtx client.Context, r *mux.Router, storeName string) {
}

// RegisterTxRoutes registers all transaction routes on the provided router.
func RegisterTxRoutes(clientCtx client.Context, r *mux.Router) {
func RegisterTxRoutes(clientCtx client.Context, rtr *mux.Router) {
r := rest.WithHTTPDeprecationHeaders(rtr)
r.HandleFunc("/txs/{hash}", QueryTxRequestHandlerFn(clientCtx)).Methods("GET")
r.HandleFunc("/txs", QueryTxsRequestHandlerFn(clientCtx)).Methods("GET")
r.HandleFunc("/txs", BroadcastTxRequest(clientCtx)).Methods("POST")
Expand Down