Skip to content

Commit

Permalink
refactor: deprecate http handlers (#108)
Browse files Browse the repository at this point in the history
* refactor: deprecate httpapi handlers

* refactor: remove unnecessary commented code

* fix: middleware bug
  • Loading branch information
mabdh authored Apr 12, 2022
1 parent 8d94bee commit 1c6538c
Show file tree
Hide file tree
Showing 73 changed files with 3,639 additions and 11,515 deletions.
10 changes: 8 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
NAME="github.com/odpf/columbus"
VERSION=$(shell git describe --always --tags 2>/dev/null)
COVERFILE="/tmp/columbus.coverprofile"
PROTON_COMMIT := "4c76e086f7877efc8897ffac74dd45cf75646a78"
PROTON_COMMIT := "39dd0f995eec1369eaa4ab811f2142484a085af8"

.PHONY: all build test clean install proto

Expand All @@ -19,6 +19,12 @@ test:
test-coverage: test
go tool cover -html=coverage.txt -o cover.html

e2e-test:
go test ./test/... --tags=e2e

generate:
go generate ./...

dist:
@bash ./scripts/build.sh

Expand All @@ -28,7 +34,7 @@ lint:
proto: ## Generate the protobuf files
@echo " > generating protobuf from odpf/proton"
@echo " > [info] make sure correct version of dependencies are installed using 'make install'"
@buf generate https://github.com/odpf/proton/archive/${PROTON_COMMIT}.zip#strip_components=1 --template buf.gen.yaml --path odpf/compass
@buf generate https://github.com/odpf/proton/archive/${PROTON_COMMIT}.zip#strip_components=1 --template buf.gen.yaml --path odpf/compass -v
@echo " > protobuf compilation finished"

install: ## install required dependencies
Expand Down
92 changes: 29 additions & 63 deletions api/api.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package api

import (
"github.com/gorilla/mux"
"fmt"
"net/http"

"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
"github.com/newrelic/go-agent/v3/newrelic"
"github.com/odpf/columbus/api/httpapi"
"github.com/odpf/columbus/api/httpapi/handlers"
"github.com/odpf/columbus/api/httpapi/middleware"
Expand All @@ -10,6 +14,7 @@ import (
"github.com/odpf/columbus/discovery"
"github.com/odpf/columbus/discussion"
"github.com/odpf/columbus/lineage"
"github.com/odpf/columbus/metrics"
"github.com/odpf/columbus/star"
"github.com/odpf/columbus/tag"
"github.com/odpf/columbus/user"
Expand All @@ -18,6 +23,8 @@ import (

type Dependencies struct {
Logger log.Logger
NRApp *newrelic.Application
StatsdMonitor *metrics.StatsdMonitor
AssetRepository asset.Repository
DiscoveryRepository discovery.Repository
TagService *tag.Service
Expand Down Expand Up @@ -46,62 +53,16 @@ func NewHandlers(logger log.Logger, deps *Dependencies) *Handlers {
}

func NewHTTPHandlers(deps *Dependencies) *httpapi.Handler {
typeHandler := handlers.NewTypeHandler(
deps.Logger,
deps.TypeRepository,
)

assetHandler := handlers.NewAssetHandler(
deps.Logger,
deps.AssetRepository,
deps.DiscoveryRepository,
deps.StarRepository,
deps.LineageRepository,
)

recordHandler := handlers.NewRecordHandler(
deps.Logger,
deps.TypeRepository,
deps.DiscoveryService,
deps.RecordRepositoryFactory,
)
searchHandler := handlers.NewSearchHandler(
deps.Logger,
deps.DiscoveryService,
)
lineageHandler := handlers.NewLineageHandler(
deps.Logger,
deps.LineageRepository,
)
tagHandler := handlers.NewTagHandler(
deps.Logger,
deps.TagService,
)
tagTemplateHandler := handlers.NewTagTemplateHandler(
deps.Logger,
deps.TagTemplateService,
)
userHandler := handlers.NewUserHandler(
deps.Logger,
deps.StarRepository,
deps.DiscussionRepository,
)

discussionHandler := handlers.NewDiscussionHandler(
deps.Logger,
deps.DiscussionRepository,
)

return &httpapi.Handler{
Asset: assetHandler,
Type: typeHandler,
Record: recordHandler,
Search: searchHandler,
Lineage: lineageHandler,
Tag: tagHandler,
TagTemplate: tagTemplateHandler,
User: userHandler,
Discussion: discussionHandler,
Record: recordHandler,
}
}

Expand All @@ -118,26 +79,31 @@ func NewGRPCHandler(l log.Logger, deps *Dependencies) *v1beta1.Handler {
DiscoveryRepository: deps.DiscoveryRepository,

//deprecated
TypeRepository: deps.TypeRepository,
DiscoveryService: deps.DiscoveryService,
}
}

func RegisterHTTPRoutes(cfg Config, router *mux.Router, deps *Dependencies, handlerCollection *httpapi.Handler) {
// By default mux will decode url and then match the decoded url against the route
// we reverse the steps by telling mux to use encoded path to match the url
// then we manually decode via custom middleware (decodeURLMiddleware).
//
// This is to allow urn that has "/" to be matched correctly to the route
router.UseEncodedPath()
router.Use(middleware.DecodeURL())

router.PathPrefix("/ping").Handler(handlers.NewHeartbeatHandler())
func RegisterHTTPRoutes(cfg Config, mux *runtime.ServeMux, deps *Dependencies, handlerCollection *httpapi.Handler) error {
if err := mux.HandlePath(http.MethodGet, "/ping", runtime.HandlerFunc(func(w http.ResponseWriter, r *http.Request, pathParams map[string]string) {
fmt.Fprintf(w, "pong")
})); err != nil {
return err
}

v1Beta1SubRouter := router.PathPrefix("/v1beta1").Subrouter()
v1Beta1SubRouter.Use(middleware.ValidateUser(cfg.IdentityHeaderKey, deps.UserService))
if err := mux.HandlePath(http.MethodGet, "/v1beta1/types/{name}/records",
middleware.NewRelic(deps.NRApp, http.MethodGet, "/v1beta1/types/{name}/records",
middleware.StatsD(deps.StatsdMonitor,
middleware.ValidateUser(cfg.IdentityHeaderKey, deps.UserService, handlerCollection.Record.GetByType)))); err != nil {
return err
}

httpapi.RegisterRoutes(v1Beta1SubRouter, handlerCollection)
if err := mux.HandlePath(http.MethodGet, "/v1beta1/types/{name}/records/{id}",
middleware.NewRelic(deps.NRApp, http.MethodGet, "/v1beta1/types/{name}/records/{id}",
middleware.StatsD(deps.StatsdMonitor,
middleware.ValidateUser(cfg.IdentityHeaderKey, deps.UserService, handlerCollection.Record.GetOneByType)))); err != nil {
return err
}

// router.NotFoundHandler = http.HandlerFunc(handlers.NotFound)
// router.MethodNotAllowedHandler = http.HandlerFunc(handlers.MethodNotAllowed)
return nil
}
21 changes: 21 additions & 0 deletions api/grpc_interceptor/interceptor_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package grpc_interceptor

import (
"context"

pb_testproto "github.com/grpc-ecosystem/go-grpc-middleware/testing/testproto"
)

type dummyService struct {
pb_testproto.TestServiceServer
}

func (s *dummyService) Ping(ctx context.Context, ping *pb_testproto.PingRequest) (*pb_testproto.PingResponse, error) {
if ping.Value == "panic" {
panic("very bad thing happened")
}
if ping.Value == "nilpanic" {
panic(nil)
}
return s.TestServiceServer.Ping(ctx, ping)
}
24 changes: 24 additions & 0 deletions api/grpc_interceptor/statsd.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package grpc_interceptor

import (
"context"
"time"

"github.com/odpf/columbus/metrics"
"google.golang.org/grpc"
"google.golang.org/grpc/status"
)

func StatsD(mm *metrics.StatsdMonitor) grpc.UnaryServerInterceptor {
return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
if mm == nil {
return handler(ctx, req)
}
start := time.Now()
resp, err := handler(ctx, req)
code := status.Code(err)
mm.ResponseTimeGRPC(info.FullMethod, int64(time.Since(start)/time.Millisecond))
mm.ResponseStatusGRPC(info.FullMethod, code.String())
return resp, err
}
}
53 changes: 53 additions & 0 deletions api/grpc_interceptor/statsd_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package grpc_interceptor

import (
"context"
"testing"

grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware"
grpc_testing "github.com/grpc-ecosystem/go-grpc-middleware/testing"
pb_testproto "github.com/grpc-ecosystem/go-grpc-middleware/testing/testproto"
"github.com/odpf/columbus/lib/mocks"
"github.com/odpf/columbus/metrics"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)

var (
statsdPrefix = "columbusApi"
metricsSeparator = "."
)

type StatsDTestSuite struct {
*grpc_testing.InterceptorTestSuite
statsdClient *mocks.StatsdClient
}

func TestStatsDSuite(t *testing.T) {
statsdClient := new(mocks.StatsdClient)

monitor := metrics.NewStatsdMonitor(statsdClient, statsdPrefix, metricsSeparator)
s := &StatsDTestSuite{
InterceptorTestSuite: &grpc_testing.InterceptorTestSuite{
TestService: &dummyService{TestServiceServer: &grpc_testing.TestPingService{T: t}},
ServerOpts: []grpc.ServerOption{
grpc_middleware.WithUnaryServerChain(
StatsD(monitor)),
},
},
statsdClient: statsdClient,
}
suite.Run(t, s)
}

func (s *StatsDTestSuite) TestUnary_StatsDMetrics() {
s.statsdClient.EXPECT().Increment("columbusApi.responseStatusCode,statusCode=OK,method=/mwitkow.testproto.TestService/Ping").Once()
s.statsdClient.EXPECT().Timing("columbusApi.responseTime,method=/mwitkow.testproto.TestService/Ping", int64(0)).Once()
_, err := s.Client.Ping(context.Background(), &pb_testproto.PingRequest{Value: "something", SleepTimeMs: 9999})
code := status.Code(err)
require.Equal(s.T(), codes.OK, code)
s.statsdClient.AssertExpectations(s.T())
}
1 change: 0 additions & 1 deletion api/grpc_interceptor/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,5 @@ func ValidateUser(identityHeaderKey string, userSvc *user.Service) grpc.UnarySer
}
newCtx := user.NewContext(ctx, userID)
return handler(newCtx, req)

}
}
82 changes: 82 additions & 0 deletions api/grpc_interceptor/user_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package grpc_interceptor

import (
"context"
"errors"
"testing"

grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware"
grpc_testing "github.com/grpc-ecosystem/go-grpc-middleware/testing"
pb_testproto "github.com/grpc-ecosystem/go-grpc-middleware/testing/testproto"
"github.com/odpf/columbus/lib/mocks"
"github.com/odpf/columbus/user"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/status"
)

const (
identityHeaderKey = "Columbus-User-ID"
defaultProvider = "shield"
)

type UserTestSuite struct {
*grpc_testing.InterceptorTestSuite
userRepo *mocks.UserRepository
}

func TestUserSuite(t *testing.T) {
mockUserRepo := new(mocks.UserRepository)
userSvc := user.NewService(mockUserRepo, user.Config{
IdentityProviderDefaultName: defaultProvider,
})
s := &UserTestSuite{
InterceptorTestSuite: &grpc_testing.InterceptorTestSuite{
TestService: &dummyService{TestServiceServer: &grpc_testing.TestPingService{T: t}},
ServerOpts: []grpc.ServerOption{
grpc_middleware.WithUnaryServerChain(
ValidateUser(identityHeaderKey, userSvc)),
},
},
userRepo: mockUserRepo,
}
suite.Run(t, s)
}

func (s *UserTestSuite) TestUnary_IdentityHeaderNotPresent() {
_, err := s.Client.Ping(s.SimpleCtx(), &pb_testproto.PingRequest{Value: "something", SleepTimeMs: 9999})
code := status.Code(err)
require.Equal(s.T(), codes.InvalidArgument, code)
require.EqualError(s.T(), err, "rpc error: code = InvalidArgument desc = identity header is empty")
}

func (s *UserTestSuite) TestUnary_UserServiceError() {
userEmail := "user-email-error"
customError := errors.New("some error")
s.userRepo.EXPECT().GetID(mock.Anything, userEmail).Return("", customError)
s.userRepo.EXPECT().Create(mock.Anything, mock.Anything).Return("", customError)

ctx := metadata.AppendToOutgoingContext(context.Background(), identityHeaderKey, userEmail)
_, err := s.Client.Ping(ctx, &pb_testproto.PingRequest{Value: "something", SleepTimeMs: 9999})
code := status.Code(err)
require.Equal(s.T(), codes.Internal, code)

s.userRepo.AssertExpectations(s.T())
}

func (s *UserTestSuite) TestUnary_HeaderPassed() {
userEmail := "user-email"
userID := "user-id"
s.userRepo.EXPECT().GetID(mock.Anything, userEmail).Return(userID, nil)

ctx := metadata.AppendToOutgoingContext(s.SimpleCtx(), identityHeaderKey, userEmail)
_, err := s.Client.Ping(ctx, &pb_testproto.PingRequest{Value: "something", SleepTimeMs: 9999})
code := status.Code(err)
require.Equal(s.T(), codes.OK, code)

s.userRepo.AssertExpectations(s.T())
}
20 changes: 1 addition & 19 deletions api/httpapi/handler.go
Original file line number Diff line number Diff line change
@@ -1,27 +1,9 @@
package httpapi

import (
"net/http"

"github.com/gorilla/mux"
"github.com/odpf/columbus/api/httpapi/handlers"
)

type Handler struct {
Asset *handlers.AssetHandler
Type *handlers.TypeHandler
Record *handlers.RecordHandler
Search *handlers.SearchHandler
Lineage *handlers.LineageHandler
Tag *handlers.TagHandler
TagTemplate *handlers.TagTemplateHandler
User *handlers.UserHandler
Discussion *handlers.DiscussionHandler
}

func RegisterRoutes(router *mux.Router, handlerCollection *Handler) {
setupV1Beta1Router(router, handlerCollection)

router.NotFoundHandler = http.HandlerFunc(handlers.NotFound)
router.MethodNotAllowedHandler = http.HandlerFunc(handlers.MethodNotAllowed)
Record *handlers.RecordHandler
}
Loading

0 comments on commit 1c6538c

Please sign in to comment.