Skip to content

Comments

Fix/domains multiple api types lazy evaluation#13310

Open
cstea wants to merge 2 commits intoserverless:mainfrom
cstea:fix/domains-multiple-api-types-lazy-evaluation
Open

Fix/domains multiple api types lazy evaluation#13310
cstea wants to merge 2 commits intoserverless:mainfrom
cstea:fix/domains-multiple-api-types-lazy-evaluation

Conversation

@cstea
Copy link

@cstea cstea commented Feb 1, 2026

Summary

  • Fix: Uses lazy evaluation for getDefaultApiType() so it's only called when a domain actually needs a default API type
  • When all domains have explicit apiType, no API type detection is performed, avoiding the erroneous "Multiple API types
    detected" error
  • The default API type is computed once and cached when needed

Problem

Previously, getDefaultApiType() was called unconditionally during initializeVariables(). When multiple API types
(HTTP, REST, WebSocket) were detected in the CloudFormation template, it threw an error:

Multiple API types detected in CloudFormation template: HTTP, WEBSOCKET.
Please explicitly specify the apiType for each domain configuration.

This happened even when all domain configurations already had explicit apiType specified, making it impossible to use
multi-domain configurations with different API types as documented.

Solution

Implement lazy evaluation: getDefaultApiType() is now only called when a domain configuration actually needs a default
(when apiType is not explicitly specified). The result is cached to avoid redundant computations.

Test plan

  • Added 29 unit tests covering getDefaultApiType(), detectApiTypesFromTemplate(), and initializeVariables()
  • Tests verify lazy evaluation behavior (getDefaultApiType not called when not needed)
  • All existing tests pass
  • npm run lint passes
  • npm run prettier passes

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes & Improvements

    • Enhanced domain configuration with optimized API type detection and improved deferred computation handling.
    • Better fallback logic for domains without explicit API type specifications.
  • Tests

    • Added comprehensive test coverage for domain configuration, API type detection, and multi-API type scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

cstea and others added 2 commits January 31, 2026 23:17
Previously, getDefaultApiType() was called unconditionally during
initializeVariables(), throwing an error when multiple API types
(HTTP, REST, WebSocket) were detected in the CloudFormation template,
even when all domain configurations had explicit apiType specified.

This change implements lazy evaluation: getDefaultApiType() is now
only called when a domain configuration actually needs a default
(when apiType is not explicitly specified). The result is cached to
avoid redundant computations.

This allows users to configure multiple domains with different API
types (e.g., HTTP and WebSocket) as documented, as long as each
domain specifies its apiType explicitly.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add comprehensive unit tests for:
- getDefaultApiType(): Tests for single API type detection, multiple
  API types, and fallback behavior
- detectApiTypesFromTemplate(): Tests for all API types and edge cases
- initializeVariables(): Tests verifying lazy evaluation behavior,
  ensuring getDefaultApiType() is only called when needed

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@Mmarzex
Copy link
Contributor

Mmarzex commented Feb 1, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@github-actions
Copy link

github-actions bot commented Feb 1, 2026

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 1, 2026

📝 Walkthrough

Walkthrough

The ServerlessCustomDomain plugin's API type detection logic was refactored to use lazy evaluation instead of eager computation. The default API type is now computed only when needed and cached for reuse. Comprehensive unit tests were added to verify the behavior.

Changes

Cohort / File(s) Summary
Domain Plugin Refactoring
packages/serverless/lib/plugins/aws/domains/index.js
Simplified getDefaultApiType to return the first detected API type or default to HTTP. Modified initializeVariables to defer defaultApiType computation until required, with caching for subsequent domains.
Comprehensive Unit Tests
packages/serverless/test/unit/lib/plugins/aws/domains/index.test.js
Added full test coverage for getDefaultApiType, detectApiTypesFromTemplate, and initializeVariables, validating lazy evaluation, proper type detection, and correct propagation of explicit vs. default apiType values.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • eahefnawy

Poem

🐰 Hops of joy through the code we go,
Lazy loading makes it flow,
No more eager, compute with grace,
Only when needed, the right time, the right place!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main change: implementing lazy evaluation for handling multiple API types in domain configuration, which is the primary objective of this PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@cstea
Copy link
Author

cstea commented Feb 1, 2026

I have read the CLA Document and I hereby sign the CLA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants