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

Introuce a new Server interface, rename Server to ServerMux #88

Open
1 task done
sagikazarmark opened this issue Nov 3, 2021 · 0 comments
Open
1 task done
Labels
Milestone

Comments

@sagikazarmark
Copy link
Member

sagikazarmark commented Nov 3, 2021

Preflight Checklist

  • I have searched the issue tracker for an issue that matches the one I want to file, without success.

Problem Description

Twirp introduced a generated TwirpServer interface with the following signature:

// TwirpServer is the interface generated server structs will support: they're
// HTTP handlers with additional methods for accessing metadata about the
// service. Those accessors are a low-level API for building reflection tools.
// Most people can think of TwirpServers as just http.Handlers.
type TwirpServer interface {
	http.Handler

	// ServiceDescriptor returns gzipped bytes describing the .proto file that
	// this service was generated from. Once unzipped, the bytes can be
	// unmarshalled as a
	// google.golang.org/protobuf/types/descriptorpb.FileDescriptorProto.
	//
	// The returned integer is the index of this particular service within that
	// FileDescriptorProto's 'Service' slice of ServiceDescriptorProtos. This is a
	// low-level field, expected to be used for reflection.
	ServiceDescriptor() ([]byte, int)

	// ProtocGenTwirpVersion is the semantic version string of the version of
	// twirp used to generate this file.
	ProtocGenTwirpVersion() string

	// PathPrefix returns the HTTP URL path prefix for all methods handled by this
	// service. This can be used with an HTTP mux to route Twirp requests.
	// The path prefix is in the form: "/<prefix>/<package>.<Service>/"
	// that is, everything in a Twirp route except for the <Method> at the end.
	PathPrefix() string
}

Go is lucky enough to have implicit interface implementation, that's why they can generate the interface.

For some cases, it might make sense to have a Server interface in TwirPHP as well.

Proposed Solution

Currently the name Server is occupied by a simple PSR-15 handler implementation for multiplexing different Twirp services.

Server was always a bad name for this class, so this is a great opportunity to change it. Unfortunately, this is a breaking change (at the same time: I have no idea if people use it in the first place as they can easily use their own routers).

Some candidates for a new name:

  • ServerMux
  • Router
  • RequestHandler

The proposed interface would implement a set of the TwirpServer interface:

<?php

use Psr\Http\Server\RequestHandlerInterface;

interface Server extends RequestHandlerInterface
{
    public function getProtocGenTwirpPhpVersion(): string;

    public function getPathPrefix(): string;
}

The interface would extend the RequestHandlerInterface interface (similarly to Twirp's interface).

getPathPrefix is already implemented by servers as of #86.

getProtocGenTwirpPhpVersion might not prove to be useful, but we should add the generator version somewhere.

The last method from TwirpServer is ServiceDescriptor. It might be useful for reflections, but Twirp doesn't have one at the moment and we can always add it later.

Although technically changing the interface is a breaking change, implementations need to update the runtime library and the code generator at the same time anyway.

Alternatives Considered

Twirp uses the name TwirpServer to avoid ambiguity for the generated interface. But for TwirPHP it would be part of the runtime library and I don't like the redundancy of the name.

Additional Information

No response

@sagikazarmark sagikazarmark added this to the 1.0.0 milestone Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant