-
Notifications
You must be signed in to change notification settings - Fork 192
Support: union types for web sdks #1273
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
Conversation
WalkthroughThis pull request introduces union response type support across the SDK. The core changes include parsing oneOf schemas in Swagger specifications into a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/SDK/Language/Web.php (1)
338-338: Consider updating the inline comment.The comment mentions "e.g., for console SDK" specifically, but this is a general feature that can be enabled for any SDK. Consider rephrasing to "Only generate union types when the feature is enabled" for clarity.
🔎 Suggested comment update
- // Only generate union types if the feature is enabled (e.g., for console SDK) + // Only generate union types if the feature is enabled
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/SDK/Language/Web.phpsrc/Spec/Swagger2.phptests/WebUnionTypesTest.phptests/resources/spec.json
🧰 Additional context used
🧬 Code graph analysis (2)
src/SDK/Language/Web.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
tests/WebUnionTypesTest.php (1)
src/SDK/Language/Web.php (3)
Web(7-473)getReturn(328-406)setEnableUnionTypes(27-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: dotnet (server)
- GitHub Check: go (server)
- GitHub Check: dart (server)
- GitHub Check: apple (client)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, DotNet80)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, Android14Java17)
🔇 Additional comments (10)
tests/resources/spec.json (2)
1997-2056: LGTM! Well-structured test endpoint for union types.The new
/mock/tests/unionendpoint correctly demonstrates union response types usingx-oneOf, referencing bothmockandaccountdefinitions. The metadata and security configuration align with existing test endpoints.
2229-2255: LGTM! Account definition is well-formed.The new
accountdefinition follows Swagger conventions with proper field types, descriptions, examples, and required properties.src/SDK/Language/Web.php (3)
9-12: LGTM! Feature flag defaults to backward-compatible behavior.The
$enableUnionTypesflag properly defaults tofalse, ensuring existing SDK generation remains unchanged until explicitly enabled.
22-31: LGTM! Fluent setter enables method chaining.The setter correctly returns
$thisfor fluent interface usage.
339-374: LGTM! Union type generation logic is sound.The implementation correctly:
- Verifies the feature is enabled and multiple models exist
- Filters out 'any' model types
- Handles generics appropriately
- Preserves backward compatibility by falling through to single-model handling when conditions aren't met
tests/WebUnionTypesTest.php (2)
10-30: LGTM! Clean test helpers.The
createSpec()andcreateMethod()helpers generate minimal but valid structures for testing, keeping tests focused and readable.
32-70: LGTM! Comprehensive test coverage for union type behavior.The three test cases properly verify:
- Default behavior (feature disabled) maintains backward compatibility
- Enabled feature generates union types for multiple models
- Single-model scenarios work correctly with the feature enabled
src/Spec/Swagger2.php (3)
152-152: LGTM! Proper initialization.The
$responseModelsarray is correctly initialized before processing responses.
160-179: LGTM! Union type parsing preserves backward compatibility.The implementation correctly:
- Maintains existing single-model
$refhandling- Extracts multiple models from
x-oneOfschemas- Sets
responseModelto the first union member for backward compatibilityThe only concern is the missing validation mentioned in the previous comment.
206-206: LGTM! Response models exposed in method metadata.The
responseModelsarray is properly included in the method output, enabling downstream generators to access union type information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/WebUnionTypesTest.phptests/resources/spec.json
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/WebUnionTypesTest.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: swift (server)
- GitHub Check: dotnet (server)
- GitHub Check: flutter (client)
- GitHub Check: android (client)
- GitHub Check: apple (client)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, Python312)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, DotNet80)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, Android5Java17)
🔇 Additional comments (4)
tests/resources/spec.json (4)
1997-2008: LGTM! Endpoint structure is correct.The new union test endpoint follows the established pattern and is properly structured.
2047-2055: LGTM! Security and parameters are correctly configured.The endpoint has appropriate security definitions and an empty parameters array, which is correct for this test endpoint.
2229-2255: LGTM! Account definition is well-structured.The new
accountdefinition is complete with proper typing, descriptions, examples, and required fields. TheadditionalProperties: falsesetting ensures strict validation.
2009-2023: The response schema withx-oneOfis correctly structured. The Swagger2 parser at lines 168-172 ofsrc/Spec/Swagger2.phpproperly extractsresponseModelsfrom thex-oneOfarray by accessing the guaranteed$refkey in each schema object, then removes the#/definitions/prefix. The test spec structure matches the parser's expectations exactly, andresponseModelsare subsequently used for union type generation in the Web SDK. No changes needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mock-server/app/http.php (1)
558-584: UseWhiteListvalidator to restrict thetypeparameter values.The
typeparameter currently accepts any text up to 4 characters, but only'stub'triggers specific behavior. Following the pattern established elsewhere in this file (Line 521), consider using aWhiteListvalidator to explicitly restrict the parameter to valid values.🔎 Proposed fix
+use Utopia\Validator\WhiteList;- ->param('type', 'mock', new Text(4), 'Type of model to return (mock or stub)', true) + ->param('type', 'mock', new WhiteList(['mock', 'stub']), 'Type of model to return (mock or stub)', true)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/tests.ymlmock-server/app/http.phpsrc/SDK/Language/Node.phpsrc/SDK/Language/ReactNative.phpsrc/SDK/Language/Web.phptests/Base.phptests/WebChromiumTest.phptests/WebNodeTest.phptests/languages/web/index.htmltests/languages/web/node.jstests/resources/spec.json
✅ Files skipped from review due to trivial changes (1)
- tests/resources/spec.json
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests.yml
🧰 Additional context used
🧬 Code graph analysis (7)
tests/WebNodeTest.php (1)
tests/Base.php (1)
Base(17-341)
src/SDK/Language/Node.php (1)
src/SDK/Language/Web.php (1)
getUnionReturnType(315-358)
tests/WebChromiumTest.php (1)
tests/Base.php (1)
Base(17-341)
src/SDK/Language/Web.php (1)
src/SDK/Language.php (1)
toPascalCase(116-119)
src/SDK/Language/ReactNative.php (1)
src/SDK/Language/Web.php (1)
getUnionReturnType(315-358)
mock-server/app/http.php (1)
mock-server/src/Utopia/Response.php (1)
Response(12-273)
tests/languages/web/node.js (1)
tests/languages/node/test.js (2)
response(18-18)general(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: build (8.3, Go118)
- GitHub Check: build (8.3, Ruby30)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, Swift56)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, PHP83)
- GitHub Check: build (8.3, Python313)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: build (8.3, DotNet90)
- GitHub Check: build (8.3, CLINode20)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: swift (server)
- GitHub Check: android (client)
- GitHub Check: apple (client)
🔇 Additional comments (9)
tests/WebChromiumTest.php (1)
34-34: LGTM!The addition of
UNION_RESPONSESto expected output is correctly positioned afterMODEL_RESPONSESand beforeEXCEPTION_RESPONSES, consistent with the test execution order in the test files.src/SDK/Language/Node.php (1)
117-121: LGTM!The union type check is correctly placed before the existing
responseModellogic, allowing early return when multiple response models are detected. This delegates to the parentWeb::getUnionReturnType()implementation, maintaining DRY principles across the inheritance hierarchy.tests/WebNodeTest.php (1)
34-34: LGTM!The
UNION_RESPONSESaddition aligns with WebChromiumTest.php, ensuring consistent test expectations across both web test variants.tests/languages/web/node.js (1)
143-156: LGTM!The union type tests properly validate both response variants:
- Mock type: validates
resultproperty exists- Stub type: validates both
dataandtypeproperties existThe console outputs align with the expected
UNION_RESPONSESvalues intests/Base.php.tests/languages/web/index.html (1)
207-220: LGTM!The browser-based union type tests mirror the Node.js implementation, ensuring consistent behavior validation across both runtime environments.
src/SDK/Language/ReactNative.php (1)
239-243: LGTM!The union type handling is consistent with the Node.php implementation, leveraging the inherited
getUnionReturnType()from the parent Web class.tests/Base.php (1)
65-69: LGTM!The
UNION_RESPONSESconstant follows the established naming convention and contains the expected output values that correspond to the console.log statements in the test files (mock'sresult, stub'sdata, and stub'stype).src/SDK/Language/Web.php (2)
370-374: LGTM!The union type check is correctly positioned before the single-model logic, providing a clean short-circuit path when multiple response models are present.
312-358: Well-structured union type generation.The
getUnionReturnType()method cleanly handles:
- Early exit for missing/empty/single response models
- Filtering out
'any'type- Proper
Models.prefix based onadditionalProperties- Generic type parameter population
The implementation is consistent with the existing single-model
getReturn()logic. Both callpopulateGenerics()on models from the spec without pre-validation, relying on the assumption that responseModels entries (derived from the OpenAPI spec'sx-oneOfdefinitions) are valid definition keys. This is a reasonable assumption for well-formed specs and matches the established pattern in the codebase.
What does this PR do?
Adds support for union return types in generated SDKs, allowing endpoints to return multiple model types.
Test Plan
Manual.
Related PRs and Issues
N/A.
Have you read the Contributing Guidelines on issues?
Yes.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.