feat: ✨ Federated gateway #18
Conversation
WalkthroughThe pull request introduces several modifications across multiple files to implement GraphQL federation using Apollo Gateway. Key changes include the addition of GraphQL-related imports and configurations in the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
libs/application-config/src/lib/subgraphs-config.ts (1)
5-10: Consider implementing a more robust subgraph configuration system.The current implementation could benefit from several improvements:
- Type safety for configuration values
- Environment-specific configurations
- Support for multiple subgraphs
Consider implementing a configuration factory:
interface SubgraphConfig { name: string; host: string; port: number; ssl?: boolean; path?: string; } function createSubgraphEndpoint(config: SubgraphConfig): ServiceEndpointDefinition { const protocol = config.ssl ? 'https' : 'http'; const path = config.path || '/graphql'; return { name: config.name, url: `${protocol}://${config.host}:${config.port}${path}`, }; }libs/application-config/src/lib/applications-config.ts (2)
16-20: Consider documenting the port configuration.The gateway configuration looks good with appropriate environment variable fallbacks.
Consider adding a comment explaining the significance of port 3333 and documenting the expected environment variable format in a README or configuration guide.
23-27: Document the federated architecture design.The user subgraph configuration is well-structured with appropriate naming and port allocation.
Consider creating an architecture document explaining:
- The role of each subgraph
- Port allocation strategy (15xxx range for subgraphs)
- Environment variable configuration for different deployment scenarios
apps/gateway/src/app/app.module.ts (2)
1-5: Consider adding error handling imports for gateway setup.While the basic imports are in place, consider importing error handling utilities from
@nestjs/commonsuch asOnModuleInitandLoggerto properly handle gateway initialization errors and logging.import { IntrospectAndCompose } from '@apollo/gateway'; import { ApolloGatewayDriver, ApolloGatewayDriverConfig } from '@nestjs/apollo'; -import { Module } from '@nestjs/common'; +import { Module, OnModuleInit, Logger } from '@nestjs/common'; import { GraphQLModule } from '@nestjs/graphql'; import { subgraphsConfig } from '@graphql-federation-workspace/applications-config';
12-19: Add essential gateway configurations for production readiness.The current gateway configuration lacks several important production-ready features:
- Security configurations (CORS, authentication)
- Operational settings (health checks, timeouts)
- Development tools (playground configuration)
Consider enhancing the configuration with these essential features:
GraphQLModule.forRoot<ApolloGatewayDriverConfig>({ driver: ApolloGatewayDriver, gateway: { supergraphSdl: new IntrospectAndCompose({ subgraphs: subgraphsConfig, + pollIntervalInMs: 10000, // Regular schema polling + subgraphHealthCheck: true, // Enable health checks }), }, + cors: { + origin: process.env.ALLOWED_ORIGINS?.split(',') || ['http://localhost:3000'], + credentials: true, + }, + playground: process.env.NODE_ENV !== 'production', + context: ({ req }) => ({ req }), // For authentication/authorization }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.vscode/settings.json(1 hunks)apps/gateway/src/app/app.module.ts(1 hunks)apps/gateway/src/main.ts(1 hunks)apps/users-application/src/main.ts(1 hunks)libs/application-config/src/index.ts(1 hunks)libs/application-config/src/lib/applications-config.ts(2 hunks)libs/application-config/src/lib/subgraphs-config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- .vscode/settings.json
- apps/gateway/src/main.ts
- apps/users-application/src/main.ts
🔇 Additional comments (4)
libs/application-config/src/index.ts (1)
2-2: LGTM! Verify impact on existing consumers.
The addition of the subgraphs configuration export aligns well with the federation implementation objective.
Let's verify if this new export affects existing consumers:
✅ Verification successful
No impact on existing consumers - safe to proceed
The verification shows that there are currently no external consumers importing from the @*/application-config package, with only the package's own test configuration file referencing it. This means the new export can be safely added without risk of affecting existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential impact on existing consumers
# Look for wildcard imports that might be affected by the new export
# Search for wildcard imports of application-config
rg -g '!node_modules' -g '!dist' "from ['\"]@[^'\"]+/application-config['\"]" -A 1
Length of output: 90
Script:
#!/bin/bash
# Let's try a different approach to find imports from application-config
# First, let's check all imports from this package using a simpler pattern
rg -g '!node_modules' -g '!dist' "@.*application-config"
# Also check for any files that might be importing from this package
fd -e ts -e js --exec grep -l "application-config"
Length of output: 150
libs/application-config/src/lib/subgraphs-config.ts (1)
1-3: LGTM! Imports are properly structured.
The imports are correctly specified, importing the necessary type from Apollo Gateway and the local configuration.
libs/application-config/src/lib/applications-config.ts (2)
29-33: Verify the inclusion of task subgraph in current scope.
While the configuration is well-structured, the task subgraph wasn't mentioned in the PR objectives which focused on the gateway and user service.
#!/bin/bash
# Description: Verify if task subgraph is referenced in other federation setup files
# Expect: References to task subgraph in federation configuration
# Search for task subgraph references in federation setup
rg -l "task.*subgraph" --type ts
# Check if task subgraph is included in federation setup
ast-grep --pattern 'subgraphs.*task'1-4: LGTM! Verify all ApplicationConfig implementations.
The addition of the required name property is appropriate for service identification in a federated setup.
✅ Verification successful
All ApplicationConfig implementations correctly include the name property
All three implementations (gatewayConfig, userSubGraph, and taskSubGraph) properly include the required name property with appropriate service identifiers for federation setup. The changes are consistent and complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all implementations of ApplicationConfig include the name property
# Expect: All implementations should have a 'name' property defined
# Search for ApplicationConfig implementations
ast-grep --pattern 'const $_: ApplicationConfig = {
$$$
}'
Length of output: 73
Script:
#!/bin/bash
# Let's try different approaches to find ApplicationConfig implementations
# Search for object literals with host and port properties
rg "{\s*host.*port" --type ts -A 5
# Search for files importing or using ApplicationConfig
rg "ApplicationConfig" --type ts
# Search for const assignments with type annotations
ast-grep --pattern 'const $_ = {
host: $_,
port: $_,
$$$
}'
Length of output: 625
Script:
#!/bin/bash
# Let's examine the actual implementations in the config file
cat libs/application-config/src/lib/applications-config.ts
Length of output: 812
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/application-config/src/lib/applications-config.spec.ts (1)
Line range hint
11-72: Enhance test coverage for required name property.The current tests verify the presence of the name property but don't validate that it's required. Consider adding a test case to verify that the ApplicationConfig interface properly enforces the name property as required.
Example test case to add:
it('should require name property in configuration', () => { // @ts-expect-error - name property is required const configWithoutName: ApplicationConfig = { host: 'localhost', port: '3333' }; expect(configWithoutName).toBeDefined(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
libs/application-config/src/lib/applications-config.spec.ts(2 hunks)
🔇 Additional comments (2)
libs/application-config/src/lib/applications-config.spec.ts (2)
22-22: LGTM! Default configuration names are well-structured.
The added name properties follow a clear naming convention that aligns well with the federation architecture, distinguishing between the gateway and its subgraphs.
Also applies to: 28-28, 34-34
57-57: Consider making service names configurable via environment variables.
While other properties (host, port) are configurable through environment variables, the service names are hardcoded. Consider adding environment variables (e.g., GATEWAY_NAME, USER_NAME, TASK_NAME) to allow name configuration in different deployment scenarios.
Let's verify if any other configuration files follow this pattern:
Also applies to: 63-63, 69-69
Closes: #8
Summary by CodeRabbit
Release Notes
New Features
Enhancements
nameproperty for better specificity.Bug Fixes
These updates enhance the application's capabilities and improve configuration clarity for users.