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

feat(transport): expose server implementation on transport #3279

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

NathanBaulch
Copy link
Contributor

@NathanBaulch NathanBaulch commented Apr 15, 2024

Description (what this PR does / why we need it):

Expose the server implementation on Transporter, similar to what's available in GRPC interceptors. This gives middlewares the ability to make decisions or delegate functionality to the server/handlers that do the work.

Example middleware that delegates authentication checks to the server handler:

func Server() middleware.Middleware {
	return func(handler middleware.Handler) middleware.Handler {
		return func(ctx context.Context, req any) (any, error) {
			if tr, ok := transport.FromServerContext(ctx); !ok {
				return nil, errors.Unauthorized()
			} else if auth, ok := tr.Server().(Authenticator); !ok {
				return nil, errors.Unauthorized()
			} else if ctx, err := auth.Authenticate(ctx, tr); err != nil {
				return nil, err
			} else {
				return handler(ctx, req)
			}
		}
	}
}

This is an additive change that requires HTTP servers to be regenerated since they need to call http.SetServer alongside the existing http.SetOperation.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Apr 15, 2024
@shenqidebaozi
Copy link
Member

This change looks strange, I don't know why it's done this way

@NathanBaulch
Copy link
Contributor Author

I'm more than happy to answer any questions. My approach exactly mirrors the way Transport.Operation (aka grpc FullMethod) is intercepted and exposed to middlewares.

@NathanBaulch
Copy link
Contributor Author

For now I'm forced to work around this by post-processing the generated code with:

# hack: ensure http handlers capture server
sed -i 's/http.SetOperation/http.SetServer(ctx, srv)\n\t\t&/g' ../internal/api/server/v1/*_http.pb.go

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 14, 2024
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Oct 30, 2024
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.74%. Comparing base (4e9dac0) to head (3cca60d).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
transport/http/transport.go 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3279      +/-   ##
==========================================
- Coverage   84.12%   81.74%   -2.38%     
==========================================
  Files          89       92       +3     
  Lines        4182     4327     +145     
==========================================
+ Hits         3518     3537      +19     
- Misses        471      600     +129     
+ Partials      193      190       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants