Skip to content
Open
Changes from all commits
Commits
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
34 changes: 29 additions & 5 deletions packages/apps/src/microsoft/teams/apps/http_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,20 @@ def __init__(
app_id: Optional[str],
logger: Optional[Logger] = None,
skip_auth: bool = False,
server_factory: Optional[Callable[[FastAPI], uvicorn.Server]] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can these be more generic? Instead of FastAPI, can it just be ASGIApplication, and instead of uvicorn.Server, it should be whatever server is being used right? The whole point of this effort is that something other than uvicorn.Server should be returnable?

One option is to make this piece generic

):
"""
Args:
app_id: Optional Microsoft App ID.
logger: Optional logger.
skip_auth: Whether to skip JWT validation.
server_factory: Optional function that takes an ASGI app
and returns a configured `uvicorn.Server`.
"""
super().__init__()
self.logger = logger or ConsoleLogger().create_logger("@teams/http-plugin")
self._server: Optional[uvicorn.Server] = None
self._port: Optional[int] = None
self._server: Optional[uvicorn.Server] = None
self._on_ready_callback: Optional[Callable[[], Awaitable[None]]] = None
self._on_stopped_callback: Optional[Callable[[], Awaitable[None]]] = None

Expand Down Expand Up @@ -105,6 +114,14 @@ async def combined_lifespan(app: Starlette):

self.app = FastAPI(lifespan=combined_lifespan)

# Create uvicorn server if user provides custom factory method
if server_factory:
self._server = server_factory(self.app)
if self._server.config.app is not self.app:
raise ValueError(
"server_factory must return a uvicorn.Server configured with the provided FastAPI app instance."
)

# Add JWT validation middleware
if app_id and not skip_auth:
jwt_middleware = create_jwt_validation_middleware(
Expand Down Expand Up @@ -145,14 +162,21 @@ def on_stopped_callback(self, callback: Optional[Callable[[], Awaitable[None]]])

async def on_start(self, event: PluginStartEvent) -> None:
"""Start the HTTP server."""
port = event.port
self._port = event.port

try:
config = uvicorn.Config(app=self.app, host="0.0.0.0", port=port, log_level="info")
self._server = uvicorn.Server(config)
if self._server and self._server.config.port != self._port:
Copy link
Collaborator

Choose a reason for hiding this comment

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

we may want to move this port check into the constructor, so it fails earlier

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'm not throwing an error for this, just a warning to say the port being used is from this builder fn and not the one provided by app.
Also the port is not available then I think, because it can be provided via app.start(port=..) too right

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah thats true, it comes from the event itself- instead of saying 'plugin had requested' can we say the plugin start event?

self.logger.warning(
"Using port configured by server factory: %d, but plugin had requested port %d.",
self._server.config.port,
self._port,
)
self._port = self._server.config.port
else:
config = uvicorn.Config(app=self.app, host="0.0.0.0", port=self._port, log_level="info")
self._server = uvicorn.Server(config)

self.logger.info("Starting HTTP server on port %d", port)
self.logger.info("Starting HTTP server on port %d", self._port)

# The lifespan handler will call the callback when the server is ready
await self._server.serve()
Expand Down