fix(schema): add discriminator.mappings information on generated schema #2891
Conversation
WalkthroughThe pull request introduces significant updates across various files, primarily enhancing type safety and functionality within the Ts.ED framework. A new enumeration Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant Event
participant Mapper
User->>API: Send Event Data
API->>Event: Create Event Instance
Event->>Event: Set Type using EventType Enum
Event->>Mapper: Map Event to Schema
Mapper->>API: Return Mapped Schema
API->>User: Respond with Schema
Warning Rate limit exceeded@Romakita has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
packages/platform/platform-express/test/__snapshots__/discriminator.spec.ts.snapis excluded by!**/*.snappackages/specs/schema/test/integrations/__snapshots__/discriminator.integration.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
docs/docs/model.md(1 hunks)packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts(1 hunks)packages/specs/schema/src/components/default/requiredMapper.ts(1 hunks)packages/specs/schema/src/components/default/schemaMapper.ts(1 hunks)packages/specs/schema/src/components/index.ts(1 hunks)packages/specs/schema/src/domain/JsonSchema.ts(1 hunks)packages/specs/schema/src/utils/ref.ts(1 hunks)packages/specs/schema/test/integrations/discriminator.integration.spec.ts(14 hunks)
🧰 Additional context used
🪛 Biome
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
[error] 14-14: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (23)
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts (3)
1-5: LGTM! Well-structured imports
The imports are well-organized, properly typed, and follow best practices with explicit file extensions.
25-25: LGTM! Proper mapper registration
The mapper is correctly registered with a descriptive key name.
1-25: Verify integration with schema generation flow
Let's ensure this mapper is properly integrated into the schema generation pipeline and works with existing discriminator implementations.
✅ Verification successful
Let me gather more information about the integration by checking the schema mapper registration and usage patterns.
Based on the provided test files and implementation, I can now generate the final response.
Discriminator mapping implementation is properly integrated
The implementation is correctly integrated into the schema generation pipeline as evidenced by:
- The mapper is properly registered in the schema components index
- Comprehensive test coverage in
discriminator.integration.spec.tsshows the mapper correctly handles:- Basic discriminator mappings
- Multiple discriminator values
- Nested discriminator hierarchies
- Array and collection cases
- The JsonSchema class properly supports discriminator functionality through dedicated methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing discriminator usage patterns
echo "Checking discriminator usage patterns..."
rg -l "discriminator" --type ts
# Verify schema mapper integration
echo "Verifying schema mapper integration..."
ast-grep --pattern 'execMapper("discriminatorMappings"'
Length of output: 2207
Script:
#!/bin/bash
# Check how the mapper is integrated in the schema components
echo "Checking schema components integration..."
cat packages/specs/schema/src/components/index.ts
# Check discriminator integration tests
echo "Checking discriminator tests..."
cat packages/specs/schema/test/integrations/discriminator.integration.spec.ts
# Check JsonSchema implementation
echo "Checking JsonSchema implementation..."
cat packages/specs/schema/src/domain/JsonSchema.ts
Length of output: 65045
🧰 Tools
🪛 Biome
[error] 14-14: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
packages/specs/schema/src/components/index.ts (1)
22-22: LGTM! Verify the new module integration.
The export statement follows the established patterns and is appropriately placed within the "default" mappers group.
Let's verify the module's existence and integration:
✅ Verification successful
Module integration verified successfully
The discriminatorMappingsMapper module exists and is properly implemented:
- File exists at the correct location
- Implements the mapping logic for discriminator properties
- Correctly registered using
registerJsonSchemaMapper - Follows the established patterns for schema mappers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and integration of the discriminatorMappingsMapper module
# Check if the module file exists
fd discriminatorMappingsMapper.js
# Check if the module is properly integrated with other mappers
ast-grep --pattern 'class $_ implements JsonSchemaMapper {
$$$
}'
# Check for any direct usage of the mapper in the codebase
rg -l "discriminatorMappingsMapper" --type ts
Length of output: 273
Script:
#!/bin/bash
# Let's verify the implementation details of the discriminatorMappingsMapper
# Check the implementation of the mapper class
cat packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
# Check if it's properly registered in the schema mapper registry
rg -A 5 "registerSchemaMapper.*discriminator" --type ts
Length of output: 990
packages/specs/schema/src/utils/ref.ts (1)
56-56: Great improvement in type safety!
Changing the type from any to unknown | undefined is a positive enhancement that makes the parameter type more explicit and safer.
packages/specs/schema/src/components/default/requiredMapper.ts (3)
Line range hint 41-47: LGTM: Discriminator handling follows OpenAPI spec
The code correctly ensures that discriminator properties are always marked as required, which is essential for proper type discrimination in OpenAPI schemas. The implementation handles both partial and normal modes appropriately.
Also applies to: 53-55
Line range hint 8-71: LGTM: Well-structured and maintainable implementation
The code demonstrates good separation of concerns, effective use of functional programming concepts, and proper handling of edge cases. The implementation is type-safe and efficiently uses utility functions.
6-6:
Remove unused imports
The newly added imports createRef, createRefName, and toRef are not used in this file.
Apply this diff to remove the unused imports:
-import {createRef, createRefName, toRef} from "../../utils/ref.js";Let's verify if these imports are used in related discriminator mapping files:
packages/specs/schema/src/components/default/schemaMapper.ts (1)
136-136: LGTM! Clean integration of discriminator mappings processing.
The addition follows the established pattern and is correctly positioned in the transformation pipeline.
Let's verify the discriminatorMappings mapper registration:
✅ Verification successful
Verified: discriminatorMappings mapper is properly registered and implemented
The mapper is correctly registered and its implementation properly handles discriminator mappings by:
- Converting schema objects to refs using
toRefutility - Preserving string refs as-is
- Maintaining the discriminator mapping structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the discriminatorMappings mapper is properly registered
# Expected: Find the registration of the discriminatorMappings mapper
# Search for the mapper registration
rg -l "registerJsonSchemaMapper.*discriminatorMappings"
# Search for the mapper implementation
ast-grep --pattern 'function discriminatorMappingsMapper($_) {
$$$
}'
Length of output: 207
Script:
#!/bin/bash
# Let's examine the implementation and registration of the discriminatorMappings mapper
rg -A 10 "registerJsonSchemaMapper.*discriminatorMappings" packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
# Also check the mapper implementation
cat packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
Length of output: 1149
docs/docs/model.md (4)
1462-1466: Well-structured enum definition for event types.
The EventType enum follows best practices with clear, descriptive values that match the intended use cases.
1469-1471: Correct implementation of discriminator key.
The base Event class properly implements the discriminator pattern using @DiscriminatorKey() decorator on the type property.
1476-1480: Proper implementation of PageView discriminator.
The PageView class correctly:
- Extends the base
Eventclass - Uses
@DiscriminatorValuewith the correct enum value - Overrides the type property with the correct enum value
Line range hint 1485-1489: Proper implementation of Action discriminator with multiple values.
The Action class demonstrates advanced usage by accepting multiple discriminator values (ACTION and CLICK_ACTION) in a single class.
packages/specs/schema/test/integrations/discriminator.integration.spec.ts (10)
202-207: Same correction needed for 'mappings' field
Please apply the previous correction here as well, changing mappings to mapping in the discriminator object.
300-305: Same correction needed for 'mappings' field
Please apply the previous correction here as well, changing mappings to mapping in the discriminator object.
442-448: Same correction needed for 'mappings' field
Please apply the previous correction here as well, changing mappings to mapping in the discriminator object.
542-548: Same correction needed for 'mappings' field
Please apply the previous correction here as well, changing mappings to mapping in the discriminator object.
675-681: Same correction needed for 'mappings' field
Please apply the previous correction here as well, changing mappings to mapping in the discriminator object.
706-712: Same correction needed for 'mappings' field
Please apply the previous correction here as well, changing mappings to mapping in the discriminator object.
896-902: Same correction needed for 'mappings' field
Please apply the previous correction here as well, changing mappings to mapping in the discriminator object.
927-933: Same correction needed for 'mappings' field
Please apply the previous correction here as well, changing mappings to mapping in the discriminator object.
1060-1066: Same correction needed for 'mappings' field
Please apply the previous correction here as well, changing mappings to mapping in the discriminator object.
23-27: Good use of TypeScript enums for type safety
Defining EventType as an enum enhances code clarity and type safety throughout the implementation.
🧰 Tools
🪛 Biome
[error] 22-27: Do not export from a test file.
(lint/suspicious/noExportsInTest)
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
Outdated
Show resolved
Hide resolved
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
Outdated
Show resolved
Hide resolved
packages/specs/schema/test/integrations/discriminator.integration.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
[error] 17-17: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
Outdated
Show resolved
Hide resolved
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
Outdated
Show resolved
Hide resolved
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
Outdated
Show resolved
Hide resolved
Romakita
force-pushed
the
fix-discriminator-mappings
branch
from
274e505
to
60d0763
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
packages/specs/schema/src/components/default/requiredMapper.ts (1)
Line range hint
8-47: Add JSDoc documentation and enhance type safetyThe functions are well-structured but would benefit from documentation and more specific types.
Consider adding JSDoc documentation and improving type safety:
+ /** Maps required properties considering aliases */ function mapRequiredProps(obj: any, schema: JsonSchema, options: JsonSchemaOptions = {}) { - const {useAlias} = options; + const {useAlias = false} = options; const props = Object.keys(obj.properties || {}); return (keys: string[], key: string) => { const aliasedKey = useAlias ? (schema.alias.get(key) as string) || key : key; return props.includes(aliasedKey) ? keys.concat(aliasedKey) : keys; }; } + /** + * Extracts and processes required properties from the schema + * @param obj The object containing properties + * @param schema The JSON schema + * @param options Schema options + * @returns Array of required property names + */ function extractRequiredProps(obj: any, schema: JsonSchema, options: JsonSchemaOptions): string[] {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
packages/platform/platform-express/test/__snapshots__/discriminator.spec.ts.snapis excluded by!**/*.snappackages/specs/schema/test/integrations/__snapshots__/discriminator.integration.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
.gitignore(1 hunks)docs/docs/model.md(1 hunks)packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts(1 hunks)packages/specs/schema/src/components/default/requiredMapper.ts(1 hunks)packages/specs/schema/src/components/default/schemaMapper.ts(1 hunks)packages/specs/schema/src/components/index.ts(1 hunks)packages/specs/schema/src/domain/JsonSchema.ts(1 hunks)packages/specs/schema/src/utils/ref.ts(1 hunks)packages/specs/schema/test/integrations/discriminator.integration.spec.ts(14 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts (1)
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts:0-0
Timestamp: 2024-11-16T09:07:53.545Z
Learning: In TypeScript files within `packages/specs/schema/src/components/default/`, when the mapper name used in `registerJsonSchemaMapper` is not exported or used elsewhere, it's acceptable to use a string literal for the registration name without defining a constant.
packages/specs/schema/src/utils/ref.ts (1)
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/specs/schema/src/utils/ref.ts:59-61
Timestamp: 2024-11-16T08:52:26.301Z
Learning: In the `toRef` function of `packages/specs/schema/src/utils/ref.ts`, the `schema` parameter is always an object when it's not nullish.
🔇 Additional comments (30)
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts (1)
8-8: Verify property name: "mapping" vs "mappings"
The code uses mapping but the AI summary mentions mappings. This inconsistency needs to be verified to ensure correct property access.
✅ Verification successful
Based on the search results, I can generate the final response as we have clear evidence from both the code usage and OpenAPI specifications:
Property name 'mapping' is correct according to OpenAPI 3.0 specification
The code correctly uses mapping as the property name. This is verified by:
- OpenAPI 3.0 spec example in
os3.spec.jsonshows discriminator usingmappingproperty - No occurrences of
mappingswere found in the codebase - The implementation in
discriminatorMappingsMapper.tsconsistently usesmapping
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of both "mapping" and "mappings" in discriminator context
echo "Searching for discriminator.mapping usage:"
rg -A 2 "discriminator\s*[?]?\s*\.\s*mapping\s" --type ts
echo -e "\nSearching for discriminator.mappings usage:"
rg -A 2 "discriminator\s*[?]?\s*\.\s*mappings\s" --type ts
echo -e "\nSearching for OpenAPI/Swagger specs for correct property name:"
fd -e json -e yaml | xargs rg "discriminator" -A 5
Length of output: 3093
packages/specs/schema/src/components/index.ts (1)
22-22: LGTM! The new export follows consistent patterns.
The addition maintains the established file organization and naming conventions while supporting the PR's objective to enhance discriminator mappings.
Let's verify the existence and location of the new module:
✅ Verification successful
The export is correct but needs a path adjustment
The file exists as discriminatorMappingsMapper.ts but is being exported with a .js extension. This follows the package's convention as seen in the implementation, where imports use .js extensions even for TypeScript files.
The mapper is properly implemented and registered in the JsonSchemaMapper container to handle discriminator mappings in schemas. The export in index.ts correctly makes this functionality available to consumers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the discriminatorMappingsMapper.js file
# Check if the file exists in the expected location
fd discriminatorMappingsMapper.js packages/specs/schema/src/components/default
# Verify the module exports something
rg -l "export" $(fd discriminatorMappingsMapper.js packages/specs/schema/src/components/default)
Length of output: 104250
Script:
#!/bin/bash
# Search for the discriminatorMappingsMapper.js file in the repository
fd discriminatorMappingsMapper packages/specs/schema/src/components/default
# Check if there are any references to discriminatorMappingsMapper in the codebase
rg -l "discriminatorMappingsMapper" packages/specs/schema/src/components/default
Length of output: 234
Script:
#!/bin/bash
# Check the content of the discriminatorMappingsMapper.ts file
cat packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
# Check if there are any imports of this mapper in other files
rg -l "discriminatorMappingsMapper" packages/specs/schema/src
Length of output: 1030
.gitignore (1)
18-18: LGTM! The new coverage pattern is appropriate.
The addition of coverage-* complements the existing coverage entry and follows standard practices for handling multiple coverage reports.
Let's verify if this pattern is being used in test scripts:
✅ Verification successful
The coverage-* pattern is well justified by the monorepo's test setup
The verification confirms the pattern's necessity:
- Multiple packages use Vitest for test coverage (
--coverage.thresholds.autoUpdate=true) - Root package.json merges coverage reports from all packages using
merge-istanbulintocoverage/coverage-final.json - The pattern ensures all intermediate coverage files and directories are properly ignored
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for coverage-related patterns in test scripts
# Expected: Find coverage-related commands that might generate directories matching coverage-*
# Search for coverage-related commands in package.json scripts
rg -g "package.json" "coverage"
# Search for coverage configuration files
fd -g "jest.config.*" -x cat {} \; | rg "coverage"
fd -g ".nycrc*" -x cat {} \; | rg "directory"
Length of output: 6168
packages/specs/schema/src/utils/ref.ts (2)
56-56: LGTM! Good type safety improvement.
The change from any to unknown | undefined enhances type safety by making the parameter type more explicit and forcing proper type checking.
59-61: LGTM! Nullish check is sufficient.
The nullish check is appropriate here since we know from the author's clarification that schema is always an object when not nullish.
packages/specs/schema/src/components/default/requiredMapper.ts (2)
Line range hint 42-47: LGTM: Robust discriminator property handling
The code correctly ensures that discriminator properties are always marked as required, which is essential for proper type discrimination in OpenAPI schemas. This is handled consistently in both partial and full scenarios.
Also applies to: 56-58
6-6: Verify necessity of unused imports
The newly added imports createRef, createRefName, and toRef from ../../utils/ref.js are not utilized within this file. This could lead to unnecessary code bloat.
Let's verify if these imports are needed for future changes or if they should be removed:
packages/specs/schema/src/components/default/schemaMapper.ts (1)
136-136: Verify error handling for discriminator mapping.
The new mapper execution could potentially fail if the discriminator mapping is invalid or missing. Consider wrapping this transformation in a try-catch block or adding validation to handle potential errors gracefully.
docs/docs/model.md (4)
1462-1466: LGTM: Well-defined enum for event types.
The EventType enum is properly defined with clear, descriptive values that match the discriminator use case.
1467-1475: LGTM: Proper implementation of discriminator base class.
The Event class correctly implements the discriminator pattern:
- Uses
@DiscriminatorKey()to mark the type field - Properly declares the common properties shared by child classes
1476-1481: LGTM: Correct implementation of PageView discriminator.
The PageView class:
- Properly extends the base
Eventclass - Correctly uses
@DiscriminatorValuewith the appropriate enum value - Explicitly overrides the type field with the correct enum value
1482-1488: LGTM: Well-implemented Action class with multiple discriminator values.
The Action class demonstrates proper usage of multiple discriminator values:
- Correctly extends the base
Eventclass - Uses
@DiscriminatorValuewith multiple enum values (ACTION,CLICK_ACTION) - Adds its specific properties (
event)
packages/specs/schema/src/domain/JsonSchema.ts (1)
1045-1054: LGTM: The discriminator mapping is correctly implemented
The discriminator mappings are constructed appropriately, allowing for the resolution of correct references based on group options in the getSchema function.
packages/specs/schema/test/integrations/discriminator.integration.spec.ts (17)
23-27: Proper Use of EventType Enum
The introduction of the EventType enum enhances type safety and ensures consistent usage of event types across the codebase.
🧰 Tools
🪛 Biome
[error] 22-27: Do not export from a test file.
(lint/suspicious/noExportsInTest)
44-47: Consistent Usage of @DiscriminatorValue with Enum
The PageView class correctly utilizes @DiscriminatorValue(EventType.PAGE_VIEW) and overrides the type property, ensuring alignment with the EventType enum.
46-46: Confirm readonly Modifier on Overridden type Property
By marking the type property as override readonly, the subclass PageView immutably sets the type. Verify that this doesn't conflict with any code that expects type to be mutable, as the base class Event defines type without readonly.
52-54: Correct Implementation of @DiscriminatorValue in Action Class
Using EventType.ACTION and EventType.CLICK_ACTION in the @DiscriminatorValue decorator enhances maintainability and type safety.
125-130: Correct Use of Discriminator mapping Field
The discriminator object correctly uses the mapping field (singular), which aligns with the OpenAPI Specification. This addresses the previous issue regarding the incorrect use of mappings.
201-206: Proper Discriminator Mapping in Array Schema
The discriminator mappings are correctly defined within the array items in the JSON schema, ensuring proper schema validation for arrays of events.
299-305: Expanded Discriminator Mappings Including CustomAction
The addition of custom_action to the discriminator mapping ensures comprehensive coverage of all event types.
442-448: Accurate Discriminator Definition in Endpoint Schema
The generated JSON schema for the endpoint correctly includes the discriminator with its mappings, which is essential for client-side validation and code generation.
542-548: Correct Discriminator Mapping in Partial Schemas
The discriminator mappings in the partial schemas are accurately defined, facilitating proper handling of partial updates.
576-578: Ensure Snapshot Tests Reflect the Latest Schema
As the schema has been updated, confirm that the snapshot tests are passing and accurately reflect the latest changes.
You can run your test suite to ensure all snapshots are up to date.
675-681: Correct Discriminator Usage in OpenAPI Specification
The OpenAPI specification for the put operation correctly includes the discriminator with the appropriate mapping, which is crucial for API documentation and client code generation.
706-712: Consistent Response Schema with Discriminator
The response schema for the put operation consistently uses the discriminator, ensuring that consumers of the API can properly deserialize the responses.
896-902: Accurate Discriminator Mapping for Partial Input
The request body schema correctly defines the discriminator mappings for partial inputs, which is important for PATCH operations.
927-933: Consistent Discriminator in Response Schema for Partial Updates
The response schema for the patch operation maintains the correct discriminator mappings, ensuring consistency in API responses.
1060-1066: Proper Discriminator Definition in get Operation
The get operation's response schema correctly includes the discriminator, enabling accurate deserialization of returned event types.
Line range hint 1175-1175: Validate Snapshot for New Test Case
Ensure that the snapshot generated for the new test case involving the kind property matches the expected schema.
Please run the tests and check the snapshot to confirm correctness.
29-29: Verify Necessity of enums(EventType); Call
Please verify if the enums(EventType); function call is required for schema generation or enum registration. If it's unnecessary, it can be removed to clean up the code.
You can run the following script to check for any dependencies on the enums(EventType); call:
✅ Verification successful
Let me gather more context about the enums function and its usage.
Based on the code analysis, I can now provide a final response:
The enums(EventType); call is necessary - do not remove
The enums() function is a crucial utility that registers enum schemas for discriminator functionality in this test file. It's required because:
- It registers the
EventTypeenum for use in discriminator mapping - The test file validates complex discriminator scenarios where
EventTypevalues are used to differentiate between different event implementations - The schema generation and OpenAPI spec tests depend on proper enum registration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to 'enums(EventType)' in the codebase.
# Test: Find all occurrences of 'enums(EventType)'.
# Expect: Only this instance should be found.
rg 'enums\(EventType\)'
Length of output: 117
Script:
#!/bin/bash
# Search for the definition and usage of 'enums' function
ast-grep --pattern 'function enums'
# Search for other references to 'enums' function
rg 'enums\(' -A 2
# Look at the full content of the test file to understand the context
cat packages/specs/schema/test/integrations/discriminator.integration.spec.ts
Length of output: 35283
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
Outdated
Show resolved
Hide resolved
Romakita
force-pushed
the
fix-discriminator-mappings
branch
from
60d0763
to
8dfcd4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (2)
packages/core/src/utils/objects/deepMerge.ts (1)
Line range hint
67-83: Enhance type safety of the merge operation.While the implementation is functionally correct, the type safety could be improved to better align with the PR's goal of enhancing type safety across the codebase.
Consider these type improvements:
// Stronger type for the accumulator type DeepMergeable = Record<string, any>; export function deepMerge< T extends DeepMergeable, C extends DeepMergeable >( source: T, obj: C, options: DeepMergeOptions = {} ): T & C { // ... existing implementation } // Stronger typing for options export interface DeepMergeOptions { reducers?: { [K in string]: DeepMergeReducerCB; }; parentKey?: string; cleanUndefinedProps?: boolean; }This would:
- Ensure objects being merged are actually mergeable
- Provide better type inference for the merged result
- Make the options interface more precise
packages/specs/schema/src/utils/transformToOS2.ts (1)
Line range hint
424-474: Consider refactoring schema resolution in discriminator mappingThe current implementation handles schema resolution well but could be more maintainable. Consider extracting the schema resolution logic into a separate method.
Here's a suggested refactor:
convertDiscriminatorMapping(mapping: any) { + private resolveSchemaReference(schemaNameOrRef: string): any { + // Try resolving as schema name first + if (/^[a-zA-Z0-9._-]+$/.test(schemaNameOrRef)) { + try { + return this.resolveReference(this.spec, {$ref: `#/components/schemas/${schemaNameOrRef}`}, false); + } catch (err) { + console.debug(`Error resolving ${schemaNameOrRef} as schema name: ${err}`); + } + } + + // Try resolving as JSON Reference + try { + return this.resolveReference(this.spec, {$ref: schemaNameOrRef}, false); + } catch (err) { + console.debug(`Error resolving ${schemaNameOrRef} as JSON reference: ${err}`); + } + + return null; + } Object.keys(mapping).forEach((payload) => { const schemaNameOrRef = mapping[payload]; if (typeof schemaNameOrRef !== "string") { console.warn(`Ignoring ${schemaNameOrRef} for ${payload} in discriminator.mapping.`); return; } - // payload may be a schema name or JSON Reference string. - // OAS3 spec limits schema names to ^[a-zA-Z0-9._-]+$ - let schema; - if (/^[a-zA-Z0-9._-]+$/.test(schemaNameOrRef)) { - try { - schema = this.resolveReference(this.spec, {$ref: `#/components/schemas/${schemaNameOrRef}`}, false); - } catch (err) { - console.debug(`Error resolving ${schemaNameOrRef} for ${payload} as schema name in discriminator.mapping: ${err}`); - } - } - - if (!schema) { - try { - schema = this.resolveReference(this.spec, {$ref: schemaNameOrRef}, false); - } catch (err) { - console.debug(`Error resolving ${schemaNameOrRef} for ${payload} in discriminator.mapping: ${err}`); - } - } + const schema = this.resolveSchemaReference(schemaNameOrRef); if (schema) { schema["x-discriminator-value"] = payload; schema["x-ms-discriminator-value"] = payload; } else { console.warn(`Unable to resolve ${schemaNameOrRef} for ${payload} in discriminator.mapping.`); } }); }🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
packages/platform/platform-express/test/__snapshots__/discriminator.spec.ts.snapis excluded by!**/*.snappackages/specs/schema/test/integrations/__snapshots__/discriminator.integration.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
.gitignore(1 hunks)docs/docs/model.md(1 hunks)packages/core/src/utils/objects/cleanObject.ts(1 hunks)packages/core/src/utils/objects/deepMerge.ts(2 hunks)packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts(1 hunks)packages/specs/schema/src/components/default/requiredMapper.ts(2 hunks)packages/specs/schema/src/components/default/schemaMapper.ts(3 hunks)packages/specs/schema/src/components/index.ts(1 hunks)packages/specs/schema/src/domain/JsonSchema.ts(1 hunks)packages/specs/schema/src/utils/ref.ts(1 hunks)packages/specs/schema/src/utils/transformToOS2.ts(4 hunks)packages/specs/schema/test/integrations/discriminator.integration.spec.ts(14 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts (1)
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts:0-0
Timestamp: 2024-11-16T09:07:53.545Z
Learning: In TypeScript files within `packages/specs/schema/src/components/default/`, when the mapper name used in `registerJsonSchemaMapper` is not exported or used elsewhere, it's acceptable to use a string literal for the registration name without defining a constant.
packages/specs/schema/src/utils/ref.ts (1)
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/specs/schema/src/utils/ref.ts:59-61
Timestamp: 2024-11-16T08:52:26.301Z
Learning: In the `toRef` function of `packages/specs/schema/src/utils/ref.ts`, the `schema` parameter is always an object when it's not nullish.
🔇 Additional comments (35)
packages/core/src/utils/objects/cleanObject.ts (2)
13-15: 🧹 Nitpick (assertive)
Consider deep cloning for nested objects
The current implementation performs a shallow copy, which means nested objects will share references. This could lead to unintended side effects if the cleaned object is modified later.
Let's check if there are any usages with nested objects:
#!/bin/bash
# Search for calls to cleanObject with nested object parameters
ast-grep --pattern 'cleanObject({
$$$: {
$$$
}
$$$
})'Consider using a deep clone for the values if nested objects need to be handled:
- obj[key] = value;
+ obj[key] = typeof value === 'object' && value !== null
+ ? JSON.parse(JSON.stringify(value))
+ : value;6-16: Verify integration with discriminator mappings
Since this function is part of changes involving discriminator mappings, let's verify its usage in that context.
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts (1)
7-22: 🧹 Nitpick (assertive)
Align naming convention between file and implementation
There's an inconsistency in naming:
- File name uses plural form:
discriminatorMappingsMapper.ts - Function name and property access use singular form:
discriminatorMappingMapperandobj.discriminator.mapping
Consider aligning these names for better maintainability.
-function discriminatorMappingMapper(obj: {
- discriminator?: {mapping: Record<string, JsonSchema | string>}
+function discriminatorMappingsMapper(obj: {
+ discriminator?: {mappings: Record<string, JsonSchema | string>}
}, _: JsonSchema, options: JsonSchemaOptions) {
- if (obj.discriminator?.mapping) {
- const entries = Object.entries(obj.discriminator.mapping);
+ if (obj.discriminator?.mappings) {
+ const entries = Object.entries(obj.discriminator.mappings);
const newMapping: Record<string, string> = {};
for (const [key, value] of entries) {
newMapping[key] = isString(value) ? value : toRef(value, null, options).$ref;
}
- obj.discriminator.mapping = newMapping;
+ obj.discriminator.mappings = newMapping;
}
return obj;
}⛔ Skipped due to learnings
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/specs/schema/src/domain/JsonSchema.ts:1045-1054
Timestamp: 2024-11-16T08:52:24.617Z
Learning: When constructing discriminator mappings in `packages/specs/schema/src/domain/JsonSchema.ts`, it's necessary to assign the schema object to the mapping (i.e., `acc[key] = schema;`) instead of a schema reference, because the schema is needed to resolve the correct reference later based on given group options to the `getSchema` function.
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts:0-0
Timestamp: 2024-11-16T09:07:53.545Z
Learning: In TypeScript files within `packages/specs/schema/src/components/default/`, when the mapper name used in `registerJsonSchemaMapper` is not exported or used elsewhere, it's acceptable to use a string literal for the registration name without defining a constant.
packages/specs/schema/src/components/index.ts (1)
22-22: 💡 Codebase verification
The export statement has an incorrect file extension
The export statement uses .js extension but the actual file has .ts extension:
packages/specs/schema/src/components/default/discriminatorMappingsMapper.tsexists- The export statement incorrectly references
discriminatorMappingsMapper.js
Please update the export statement to use the correct .ts extension:
export * from "./default/discriminatorMappingsMapper.ts";🔗 Analysis chain
LGTM! The new export follows established patterns.
The addition of discriminatorMappingsMapper.js export is consistent with the file's structure and purpose. It's correctly grouped with other default mappers and maintains the established naming conventions.
Let's verify the existence of the exported module:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the discriminatorMappingsMapper.js file exists and exports expected functionality
# Check if the file exists in the expected location
fd discriminatorMappingsMapper.js packages/specs/schema/src/components/default
# Verify the file exports a mapper function
ast-grep --pattern 'export function $name($$$) { $$$ }' packages/specs/schema/src/components/default/discriminatorMappingsMapper.js
Length of output: 337
Script:
#!/bin/bash
# Let's search for the file in the entire repository
fd discriminatorMappingsMapper
# Also search for any file containing discriminator mapping related functionality
rg -l "discriminator.*mapping" --type ts
Length of output: 277
⛔ Skipped due to learnings
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts:0-0
Timestamp: 2024-11-16T09:07:53.545Z
Learning: In TypeScript files within `packages/specs/schema/src/components/default/`, when the mapper name used in `registerJsonSchemaMapper` is not exported or used elsewhere, it's acceptable to use a string literal for the registration name without defining a constant.
.gitignore (1)
18-18: LGTM! Good addition for coverage reports.
The new entry coverage-* is well-placed alongside existing coverage ignores and will help maintain a clean repository by excluding various coverage report formats that might be generated during testing.
packages/specs/schema/src/utils/ref.ts (2)
56-56: LGTM! Good type safety improvement.
The change from any to unknown | undefined enforces better type safety by requiring explicit type checking.
59-61: Skip review - implementation confirmed correct.
Based on the previous review discussion and learnings, the schema parameter is confirmed to always be an object when not nullish, making this implementation correct.
packages/specs/schema/src/components/default/requiredMapper.ts (3)
Line range hint 8-61: LGTM! Well-structured discriminator handling
The implementation correctly handles required properties while ensuring discriminator properties remain required, which aligns well with the PR's objective of enhancing discriminator mappings.
62-62: 🛠️ Refactor suggestion
Consider maintaining immutability
The change from returning a new object to directly mutating the input object (obj.required = required) could lead to unexpected side effects if the input object is used elsewhere in the codebase. Consider maintaining immutability by returning a new object instead:
- obj.required = required;
+ return {
+ ...obj,
+ required
+ };#!/bin/bash
# Description: Check for potential issues with object mutation
# Look for places where the same object might be used multiple times
# Search for other mappers that might process the same object
echo "Searching for other schema mappers..."
rg "registerJsonSchemaMapper" --type ts -B 5 -A 1
# Look for tests that might assume immutability
echo "Checking test files for immutability assumptions..."
fd -e spec.ts -e test.ts | xargs rg "requiredMapper"6-6: Verify necessity of unused imports
The newly added imports createRef, createRefName, and toRef from ref.js are not utilized within this file. Consider removing them if they're not needed for future implementations.
packages/specs/schema/src/components/default/schemaMapper.ts (3)
79-80: LGTM! Good refactoring.
The change simplifies the code by using direct assignment while maintaining the same functionality. This makes the code more consistent with the rest of the function's style.
100-102: LGTM! Code simplification.
The change streamlines the reducer logic by using direct assignment, making the code more readable while maintaining the same functionality.
133-133: Verify discriminator mapper registration.
The addition of the discriminator mapping step enhances the schema generation. Let's verify that the mapper is properly registered and available.
✅ Verification successful
Discriminator mapper is properly registered and implemented
The verification confirms that:
- The mapper is correctly registered as "discriminatorMapping" in the container
- The implementation handles schema discriminator mappings by converting them to proper references
- The mapper function is properly defined with the expected parameters (obj, schema, options)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the discriminator mapper registration and implementation
# Check for the mapper registration
rg -l "registerJsonSchemaMapper.*discriminatorMapping"
# Look for the mapper implementation
ast-grep --pattern 'function discriminatorMapping$_($$$) {
$$$
}'
Length of output: 202
Script:
#!/bin/bash
# Let's check the implementation details of the discriminator mapper
# Check the registration and implementation details
rg -A 10 "registerJsonSchemaMapper.*discriminatorMapping" packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
# Check if the mapper is imported in schemaMapper.ts
rg "import.*discriminatorMapping" packages/specs/schema/src/components/default/schemaMapper.ts
Length of output: 310
Script:
#!/bin/bash
# Let's try a different approach to verify the mapper
# Check the content of the discriminator mapper file
cat packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
# Check for any exports or function definitions related to discriminator mapping
rg -A 5 "export.*discriminator" packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
Length of output: 1076
packages/specs/schema/src/utils/transformToOS2.ts (2)
3-3: LGTM: Improved type safety with OS2Security import
The addition of the OS2Security type import enhances type safety for security scheme transformations.
Line range hint 65-115: LGTM: Enhanced security transformation with better type safety and error handling
The changes improve the robustness of the security scheme transformation:
- Added explicit return type
OS2Security | undefinedfor better type safety - Added null check before adding security definitions to prevent runtime errors
- Improved code organization for better maintainability
docs/docs/model.md (2)
1462-1466: LGTM! Well-structured enum declaration.
The EventType enum provides type-safe constants for event types, following consistent naming conventions.
1485-1485: LGTM! Well-implemented Action class with multiple discriminator values.
The class demonstrates good use of the @DiscriminatorValue decorator with multiple enum values.
packages/specs/schema/src/domain/JsonSchema.ts (1)
1045-1055: Discriminator mapping logic is correctly implemented
The code accurately constructs the discriminator mappings using the resolved schemas. Assigning the schema objects to the mapping is appropriate here to resolve the correct references later based on the given group options in the getSchema function.
packages/specs/schema/test/integrations/discriminator.integration.spec.ts (17)
7-7: Importing 'enums' function is appropriate
The addition of enums to the import statement ensures that the EventType enum is properly registered for schema generation and other utility purposes.
23-27: Definition of 'EventType' enum enhances type safety
Introducing the EventType enum with explicit values improves type safety and code clarity throughout the event handling logic.
🧰 Tools
🪛 Biome
[error] 22-27: Do not export from a test file.
(lint/suspicious/noExportsInTest)
29-29: Registration of 'EventType' with 'enums(EventType);'
Calling enums(EventType); correctly registers the enum, which may be necessary for the framework to process enum types appropriately.
33-33: Updating 'type' property to use 'EventType' enum
Changing the type property in the Event class to use the EventType enum reinforces type safety and aligns with the enum-based discriminator approach.
44-44: Using '@DiscriminatorValue' with enum values
Applying @DiscriminatorValue(EventType.PAGE_VIEW) in the PageView class ensures that the discriminator value matches the corresponding enum, enhancing consistency.
46-46: Overriding 'type' property with enum value
The override of the type property as override readonly type = EventType.PAGE_VIEW; correctly sets the discriminator value for instances of PageView.
52-52: Associating multiple discriminator values with a class
Using @DiscriminatorValue(EventType.ACTION, EventType.CLICK_ACTION) in the Action class appropriately maps multiple discriminator values to the same class definition.
125-130: Correct usage of 'mapping' in discriminator object
The discriminator object correctly uses the mapping field (singular) to map discriminator values to schema definitions, in accordance with the OpenAPI Specification.
201-206: Consistent discriminator mappings in array items
The discriminator mapping within array items is accurately defined, ensuring proper schema validation for arrays of events.
299-305: Including all relevant mappings in the discriminator
All discriminator values, including custom_action, are correctly mapped to their respective schema definitions, facilitating accurate deserialization.
442-448: Discriminator mappings in endpoint schemas
The discriminator mappings are properly included in the endpoint schemas, enhancing clarity and correctness in the API specifications.
542-548: Handling partial schemas with accurate discriminator mappings
In the partial schemas, the discriminator mappings are correctly adjusted to reference the partial versions of the schemas.
675-681: Specifying discriminator mappings in request bodies
The request body schema includes the correct discriminator mappings, ensuring that clients can send the appropriate event types.
706-712: Ensuring response schemas have correct discriminator mappings
The response schema accurately reflects the discriminator mappings, which is crucial for clients to interpret the responses properly.
896-902: Partial schemas in request bodies with correct discriminator
The use of partial schemas in the request body is correctly implemented with proper discriminator mappings.
927-933: Response schemas with full definitions and discriminator mappings
The response schemas reference the full definitions with accurate discriminator mappings, providing complete information to the clients.
1060-1066: Accurate discriminator mapping in GET endpoint response
The discriminator mappings in the GET endpoint's response schema are correctly specified, ensuring consistent API behavior.
packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts
Outdated
Show resolved
Hide resolved
Romakita
force-pushed
the
fix-discriminator-mappings
branch
from
8dfcd4a
to
eebb4ce
Compare
Romakita
force-pushed
the
fix-discriminator-mappings
branch
from
0ee823a
to
0f61bd9
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range comments (6)
packages/specs/schema/src/components/default/enumsMapper.ts (2)
Line range hint
5-15: Consider removing unused parameter and improving type safetyThe function has several areas that could be improved:
- The
schemaparameter is never used in the implementation- The function mutates the input parameter directly
- Type safety could be improved for the enum checks
Consider applying these improvements:
-export function enumsMapper(obj: any, schema: JsonSchema, options: JsonSchemaOptions) { +export function enumsMapper(obj: Record<string, any>, options: JsonSchemaOptions) { + const result = {...obj}; if (options.inlineEnums && obj.enum?.$isJsonDocument) { - obj.enum = obj.enum.toJSON().enum; + result.enum = obj.enum.toJSON().enum; } if (obj.enum) { - obj.type = obj.type === "object" || obj.type === undefined ? "string" : obj.type; + result.type = result.type === "object" || result.type === undefined ? "string" : result.type; } - return obj; + return result; }
Line range hint
5-17: Document the enum processing strategyThis component plays a crucial role in schema generation by handling enum transformations. Consider adding JSDoc comments to document:
- The purpose of enum mapping
- When
inlineEnumsshould be used- The transformation rules for enum types
Add documentation like this:
+/** + * Maps enum values in schema objects according to specified options. + * When inlineEnums is enabled, JSON document enums are converted to their primitive form. + * Object and undefined types are converted to strings when enum values are present. + * + * @param obj The schema object containing enum definitions + * @param options Schema generation options + * @returns The processed schema object + */ export function enumsMapper(obj: any, schema: JsonSchema, options: JsonSchemaOptions) {packages/specs/schema/src/components/default/enumsMapper.spec.ts (2)
Line range hint
4-22: Consider adding edge cases to the test suiteWhile the basic functionality is well-tested, consider adding test cases for:
- Empty enum values
- Invalid enum values
- When inlineEnums option is false
Would you like me to provide examples of these additional test cases?
Line range hint
25-42: Improve type safety in test casesInstead of using
as any, consider creating proper type definitions for the test parameters. This would make the tests more maintainable and type-safe.- {} as any, + {} as JsonSchema,packages/third-parties/schema-formio/src/utils/getFormioSchema.ts (1)
Line range hint
26-58: Consider improving error handling and separation of concerns.The function handles multiple responsibilities (model resolution, schema generation, component mapping, and form creation). Consider these improvements:
- Add error handling for schema generation failures
- Extract form creation logic into a separate function
- Add type validation for the options parameter
Here's a suggested refactor:
interface FormGenerationOptions extends JsonSchemaOptions { formioSpecific?: Record<string, unknown>; } async function createFormFromSchema( entity: JsonEntityStore, schema: any, options: FormGenerationOptions ): Promise<Omit<FormioForm, "_id" | "deleted" | "owner">> { const name = entity.schema.getName(); const machineName = paramCase(name); const resolvers: Promise<any>[] = []; try { const components = execMapper("properties", schema, { ...options, definitions: schema.definitions, resolvers }); const form = { title: name, type: "form", display: "form", tags: [], access: [], submissionAccess: [], ...entity.store.get<any>("formio:form", {}), name: machineName, machineName, components }; await Promise.all(resolvers.map((resolver) => resolver(form, options))); return form as unknown as FormioForm; } catch (error) { throw new Error(`Failed to create form from schema: ${error.message}`); } } export async function getFormioSchema( model: string | any | undefined, options: FormGenerationOptions = {} ): Promise<Omit<FormioForm, "_id" | "deleted" | "owner"> | undefined> { if (!model) { return undefined; } const resolvedModel = isString(model) ? FormsContainer.get(model) as Type<any> : model; if (!resolvedModel) { throw new Error(`Model not found: ${model}`); } const entity = JsonEntityStore.from(resolvedModel); const schema = getJsonSchema(entity, { ...options, inlineEnums: true, customKeys: true }); return createFormFromSchema(entity, schema, options); }packages/specs/schema/test/integrations/discriminator.integration.spec.ts (1)
Line range hint
61-65: Override 'type' property in 'CustomAction' classThe
CustomActionclass should explicitly assign thetypeproperty toEventType.CUSTOM_ACTIONand update the@DiscriminatorValuedecorator to maintain consistency with theEventclass and the discriminator logic.Apply this diff to assign the
typeproperty and update the decorator:-@DiscriminatorValue() +@DiscriminatorValue(EventType.CUSTOM_ACTION) class CustomAction extends Event { + override readonly type = EventType.CUSTOM_ACTION; @Required() event: string; @Property() meta: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
packages/platform/platform-express/test/__snapshots__/discriminator.spec.ts.snapis excluded by!**/*.snappackages/specs/schema/test/integrations/__snapshots__/discriminator.integration.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
.gitignore(1 hunks)docs/docs/model.md(1 hunks)packages/core/src/utils/objects/cleanObject.ts(1 hunks)packages/core/src/utils/objects/deepMerge.ts(2 hunks)packages/platform/platform-express/test/discriminator.spec.ts(1 hunks)packages/specs/schema/src/components/default/discriminatorMappingMapper.ts(1 hunks)packages/specs/schema/src/components/default/enumsMapper.spec.ts(3 hunks)packages/specs/schema/src/components/default/enumsMapper.ts(2 hunks)packages/specs/schema/src/components/default/requiredMapper.ts(2 hunks)packages/specs/schema/src/components/default/schemaMapper.ts(4 hunks)packages/specs/schema/src/components/index.ts(2 hunks)packages/specs/schema/src/components/open-spec/discriminatorMappingMapper.ts(1 hunks)packages/specs/schema/src/components/open-spec/enumsMapper.ts(1 hunks)packages/specs/schema/src/decorators/common/const.spec.ts(3 hunks)packages/specs/schema/src/domain/JsonSchema.ts(1 hunks)packages/specs/schema/src/index.ts(2 hunks)packages/specs/schema/src/interfaces/JsonSchemaOptions.ts(1 hunks)packages/specs/schema/src/utils/ref.ts(1 hunks)packages/specs/schema/src/utils/transformToOS2.ts(4 hunks)packages/specs/schema/test/integrations/discriminator.integration.spec.ts(17 hunks)packages/third-parties/schema-formio/src/utils/getFormioSchema.ts(1 hunks)
🧰 Additional context used
📓 Learnings (5)
packages/core/src/utils/objects/cleanObject.ts (1)
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/core/src/utils/objects/cleanObject.ts:9-9
Timestamp: 2024-11-16T15:43:27.145Z
Learning: In the `cleanObject` function in `packages/core/src/utils/objects/cleanObject.ts`, `null` is considered a valid value (like `0` or `false`) and should not be filtered out.
packages/core/src/utils/objects/deepMerge.ts (1)
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/core/src/utils/objects/deepMerge.ts:67-68
Timestamp: 2024-11-16T17:53:10.160Z
Learning: In `packages/core/src/utils/objects/deepMerge.ts`, the `createInstance(source)` function returns a fresh instance and does not mutate `source` or `obj` in the `deepMerge` function. Therefore, using `createInstance(source)` does not introduce mutability.
packages/specs/schema/src/components/default/discriminatorMappingMapper.ts (1)
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts:0-0
Timestamp: 2024-11-16T09:07:53.545Z
Learning: In TypeScript files within `packages/specs/schema/src/components/default/`, when the mapper name used in `registerJsonSchemaMapper` is not exported or used elsewhere, it's acceptable to use a string literal for the registration name without defining a constant.
packages/specs/schema/src/components/open-spec/discriminatorMappingMapper.ts (1)
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/specs/schema/src/components/default/discriminatorMappingsMapper.ts:0-0
Timestamp: 2024-11-16T09:07:53.545Z
Learning: In TypeScript files within `packages/specs/schema/src/components/default/`, when the mapper name used in `registerJsonSchemaMapper` is not exported or used elsewhere, it's acceptable to use a string literal for the registration name without defining a constant.
packages/specs/schema/src/utils/ref.ts (1)
Learnt from: Romakita
PR: tsedio/tsed#2891
File: packages/specs/schema/src/utils/ref.ts:59-61
Timestamp: 2024-11-16T08:52:26.301Z
Learning: In the `toRef` function of `packages/specs/schema/src/utils/ref.ts`, the `schema` parameter is always an object when it's not nullish.
🪛 Biome
packages/specs/schema/src/components/default/discriminatorMappingMapper.ts
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/specs/schema/src/components/open-spec/enumsMapper.ts
[error] 12-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 8-8: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
🔇 Additional comments (31)
packages/core/src/utils/objects/cleanObject.ts (2)
9-9: LGTM! Condition correctly handles undefined values
The condition properly filters out undefined values while respecting that null is a valid value, as per the established requirements.
13-16: LGTM! Clean object manipulation logic
The simplified assignment and accumulator typing is clear and type-safe.
packages/specs/schema/src/components/open-spec/discriminatorMappingMapper.ts (2)
5-7: Verify mapper necessity across all spec types
The mapper is registered for OPENAPI, SWAGGER, and ASYNCAPI. Let's verify if discriminator mapping functionality is needed for all these spec types.
#!/bin/bash
# Description: Check usage and necessity across spec types
# Search for discriminator-related code in the codebase
rg -A 5 "discriminator.*mapping"
# Check for other mappers registered across multiple spec types
ast-grep --pattern 'registerJsonSchemaMapper($_, $_, SpecTypes.$_)'1-3: Review file location relative to imports
The file is located in the open-spec directory, but it imports discriminatorMappingMapper from the default directory. This seems inconsistent with the directory structure.
Let's verify the directory structure and file organization:
packages/specs/schema/src/components/default/enumsMapper.ts (1)
17-17: Verify all references to the mapper have been updated
The change from "inlineEnums" to "enums" in the mapper registration is a breaking change. We should verify that all references to this mapper have been updated accordingly.
packages/specs/schema/src/components/open-spec/enumsMapper.ts (2)
1-6: LGTM! Well-structured imports
The imports are properly organized and follow ES module patterns, clearly showing dependencies.
18-20: Verify compatibility across different spec types
While the registration is technically correct, let's verify that the const-to-enum transformation is appropriate for all three spec types (OpenAPI, Swagger, and AsyncAPI).
✅ Verification successful
Let me gather more specific information about the enum transformation implementation.
Let me check the implementation of the enum mapper to understand how it handles different spec types.
Based on the implementation review, I can now provide the final response:
Enum transformation is compatible across all spec types
The enum mapper implementation shows that:
- The base
enumsMapperhandles standard enum transformations (inlining and type coercion) - The
wrapEnumsMapperadds const-to-enum conversion which is valid for all OpenAPI, Swagger, and AsyncAPI specs - The transformation maintains JSON Schema compatibility while ensuring proper type handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if const-to-enum transformation is documented/supported in different specs
# Search for const/enum handling documentation or similar implementations
rg -l "const.*enum|enum.*const" --type ts
# Look for potential spec-specific enum handling
ast-grep --pattern 'SpecTypes.$_ {
$$$
}'
Length of output: 1374
Script:
#!/bin/bash
# Check the implementation of enumsMapper and its tests
cat packages/specs/schema/src/components/open-spec/enumsMapper.ts
cat packages/specs/schema/src/components/default/enumsMapper.spec.ts
# Check if there are any spec-specific enum handling
rg -A 5 "SpecTypes\.(OPENAPI|SWAGGER|ASYNCAPI)" --type ts
Length of output: 92975
Script:
#!/bin/bash
# Check the implementation of enumsMapper and its wrapper
cat packages/specs/schema/src/components/default/enumsMapper.ts
cat packages/specs/schema/src/components/open-spec/enumsMapper.ts
# Check if there are any spec-specific enum handling tests
cat packages/specs/schema/src/components/default/enumsMapper.spec.ts
Length of output: 2787
packages/specs/schema/src/components/default/enumsMapper.spec.ts (2)
Line range hint 45-63: Expand type coverage in test cases
The test suite covers string type scenarios well, but consider adding tests for other primitive types (number, boolean, etc.) to ensure comprehensive type handling.
#!/bin/bash
# Check if there are other primitive types used in the codebase that should be tested
rg -t ts 'type:\s*["'\''](?:number|boolean|integer)' --glob '!*.spec.ts'1-1: Verify the .js extension in import path
The import statement uses a .js extension which is unusual in TypeScript files. While this might be intentional for module resolution, please verify if this follows the project's conventions.
✅ Verification successful
Based on the findings:
- The package is configured as CommonJS (
"type": "commonjs") - It has dual module support with both ESM and CJS outputs
- The build process includes TypeScript compilation and generates both ESM and CJS outputs
- The
.jsextension in imports appears to be part of the build output handling
The .js extension in imports is correct and follows the project's conventions
The use of .js extension in imports is intentional and aligns with the project's dual module support strategy. When TypeScript compiles the code, it needs to know whether to emit ESM or CJS imports, and the .js extension helps maintain consistency between the source and the compiled output.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other TypeScript files in the project use .js extensions in imports
rg -t ts '\.js"$' --glob '!*.spec.ts' -l
# Check if there's any documentation or configuration about this convention
fd -e md -e json -x grep -l "\.js.*import"
Length of output: 1818
Script:
#!/bin/bash
# Let's check for .js extensions in imports specifically in the same directory
rg -t ts '\.js"$' --glob '*.ts' packages/specs/schema/src/components/default/
# Check the tsconfig.json files for module resolution settings
fd tsconfig.json -x cat {}
# Check package.json files for type module or other relevant configurations
fd package.json -x cat {}
Length of output: 150873
Script:
#!/bin/bash
# Let's check the package.json in the schema directory for module type and other relevant configurations
cat packages/specs/schema/package.json
# Let's also check for .js extensions in imports in the same directory
rg -t ts '\.js"$' packages/specs/schema/src/components/default/
Length of output: 2048
packages/specs/schema/src/components/default/discriminatorMappingMapper.ts (2)
1-10: LGTM! Clean imports and well-defined interface
The imports are properly organized and the SchemaWithDiscriminator interface accurately represents the discriminator mapping structure as per OpenAPI specification.
36-36: LGTM! Proper mapper registration
The mapper registration follows the established convention of using string literals for registration names.
.gitignore (1)
18-18: LGTM! Appropriate addition to coverage exclusions.
The new pattern coverage-* is well-placed and complements the existing coverage entry, ensuring all coverage report variations are properly excluded from version control.
packages/specs/schema/src/utils/ref.ts (2)
56-56: LGTM! Good type safety improvement
The change from any to unknown | undefined is a positive improvement that enforces better type safety by requiring explicit type checking.
59-61: LGTM! Null check is sufficient
Based on the codebase context that schema is always an object when not nullish, the truthiness check provides adequate protection.
packages/specs/schema/src/components/index.ts (2)
11-11: LGTM! New discriminator mapping exports align with PR objectives.
The addition of discriminator mapping exports in both default and open-spec directories provides a consistent implementation approach for handling discriminator mappings in the generated schema.
Also applies to: 24-24
12-12: 🧹 Nitpick (assertive)
Verify the impact of enum mapper changes.
The introduction of enumsMapper.js and removal of inlineEnumsMapper.js suggests a refactoring of enum handling. While this change appears to be an improvement, we should verify that all existing enum functionality is preserved.
Also applies to: 25-25
✅ Verification successful
Let me gather more information about the enum handling changes by examining both the new and old implementations.
Let me gather more specific information about the enum mapper changes by examining the implementation and tests.
Based on the gathered information, I can now provide a final response about the enum mapper changes.
Enum mapper changes are safe and maintain functionality
The changes consolidate enum handling into a single enumsMapper.js file while preserving the inline enum functionality through the inlineEnums option. The implementation shows that:
- The
enumsMapperfunction properly handles both inline and referenced enums through theinlineEnumsoption - The functionality is well-tested and used across different spec types (JSON Schema, OpenAPI, Swagger)
- The change is a refactoring that maintains backward compatibility, as the
inlineEnumsoption is still supported and defaults totruefor JSON Schema specs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage and dependencies of the removed inlineEnumsMapper
# Check for any remaining references to inlineEnumsMapper
echo "Checking for remaining references to inlineEnumsMapper:"
rg "inlineEnumsMapper" -l
# Check for potential usage patterns that might be affected
echo "Checking for inline enum usage patterns:"
ast-grep --pattern 'enum $enumName { $$$ }'
Length of output: 283
Script:
#!/bin/bash
# Check the implementation of the new enumsMapper
echo "Checking new enumsMapper implementation:"
fd enumsMapper.js$ --exec cat {}
# Check git history for the removed inlineEnumsMapper
echo -e "\nChecking git history for inlineEnumsMapper:"
git log --patch --all -- "**/inlineEnumsMapper.js"
# Look for enum-related imports and usage
echo -e "\nChecking enum-related imports:"
rg "import.*enum" -A 2
# Check for enum usage patterns in TypeScript files
echo -e "\nChecking enum usage in TypeScript files:"
rg "enum\s+\w+" -A 3 --type ts
Length of output: 31710
Script:
#!/bin/bash
# Check the implementation of enumsMapper and its tests
echo "Checking enumsMapper implementation and tests:"
fd "enumsMapper\.(js|ts)$" --exec cat {}
# Check for any references to inlineEnums in the codebase
echo -e "\nChecking inlineEnums references:"
rg "inlineEnums" -A 2
# Check git history for the specific file changes
echo -e "\nChecking git history for enum mapper changes:"
git log -p -- "**/enumsMapper.js" "**/inlineEnumsMapper.js"
Length of output: 3697
packages/third-parties/schema-formio/src/utils/getFormioSchema.ts (1)
36-38: Verify the impact of inline enums on schema size and discriminator mappings.
The addition of inlineEnums: true looks good and aligns with the PR objectives. However, let's verify its integration with the component mapping system.
✅ Verification successful
Based on the gathered information, I can now provide a final response about the impact of inlineEnums: true:
The addition of inlineEnums: true is safe and improves schema generation for Form.io components.
The codebase analysis shows that:
- Enums are properly handled by dedicated mappers (
enumToComponent.ts) that convert them to Form.io components - The schema-formio package uses enums for both radio buttons and select boxes
- Inlining enums will improve the generated schema by including enum values directly where they are used, which is the preferred format for Form.io components
Key evidence:
stringToComponent.tsandarrayToComponent.tsboth handle enum conversion throughexecMapper("enum", schema, options)- Integration tests show proper enum handling for various Form.io component types
- No references to external enum definitions were found that would be broken by inlining
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how enum components are mapped and verify discriminator handling
# Check for enum component usage
ast-grep --pattern 'execMapper("enum", $_, $_)'
# Check for discriminator-related code
rg -A 5 'discriminator|mappings' --type ts
# Look for existing enum references in schemas
rg -A 5 '\$ref.*#\/definitions\/.*Enum' --type json
Length of output: 66078
Script:
#!/bin/bash
# Let's check how enums are used in the codebase and verify the impact of inlining them
# Check for enum-related code in schema-formio package
rg -A 5 'enum' --type ts packages/third-parties/schema-formio/
# Check for enum component mapping implementation
ast-grep --pattern 'function enumMapper($_, $_) {
$$$
}'
# Look for enum references in schema files
fd -e json -x cat {} \; | rg -A 5 '"enum":'
Length of output: 12961
packages/specs/schema/src/decorators/common/const.spec.ts (1)
62-65: LGTM! Consistent schema representation.
The OpenAPI schema representation is consistent with the previous test case, correctly using enum for constant value representation in the component schema.
packages/core/src/utils/objects/deepMerge.ts (3)
67-68: LGTM! The implementation preserves prototypes safely.
The use of createInstance(source) is a good approach as it creates a fresh instance while preserving the prototype chain.
80-83: LGTM! The reduce operation is now optimized.
The simplified assignment and return statement improve efficiency while maintaining the same functionality.
Line range hint 67-83: Verify integration with discriminator mapping functionality.
Since this is a core utility function being modified to support discriminator mappings, let's verify its integration and test coverage.
✅ Verification successful
Let me check the actual usage context in the discriminator mapping files and the test coverage to make a more informed decision.
Let me check the actual changes made to the deepMerge function to ensure the test coverage is adequate for the modifications.
Changes to deepMerge are safe and well-tested
The modifications to deepMerge are purely performance-related, changing the object property assignment approach from spread operator to direct assignment while maintaining the same functionality. The existing comprehensive test suite covers various merge scenarios including objects, arrays, and custom reducers. While there's no direct discriminator-specific test in deepMerge.spec.ts, the functionality is tested through the JsonDiscriminator.spec.ts integration tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of deepMerge in discriminator mapping related files and check test coverage
# Check usage in discriminator mapping related files
echo "Checking usage in discriminator mapping files..."
rg -l "deepMerge" packages/specs/schema/src/components/**/discriminator*
# Check for related test files
echo "Checking test coverage..."
fd -g "*deepMerge*.spec.ts" -g "*discriminator*.spec.ts" packages/
Length of output: 526
Script:
#!/bin/bash
# Check the actual usage context of deepMerge in discriminator mapping files
echo "Checking deepMerge usage context in discriminator mapping files..."
rg -A 5 -B 5 "deepMerge" packages/specs/schema/src/components/**/discriminatorMappingMapper.ts
echo "\nChecking test coverage for deepMerge..."
cat packages/core/src/utils/objects/deepMerge.spec.ts
Length of output: 5508
Script:
#!/bin/bash
# Get the actual changes made to deepMerge function
echo "Checking the actual changes in deepMerge.ts..."
git diff HEAD~1 packages/core/src/utils/objects/deepMerge.ts
# Check if there are any discriminator-specific tests
echo "\nChecking for discriminator-related tests..."
rg -l "discriminator" packages/core/src/utils/objects/deepMerge.spec.ts packages/specs/schema/src/**/*spec.ts
Length of output: 3119
packages/specs/schema/src/components/default/schemaMapper.ts (1)
16-16: Verify the impact of emptying IGNORES_OPENSPEC
The change from const IGNORES_OPENSPEC = ["const"] to an empty array means that "const" properties will now be included in OpenAPI specs. Please verify if this is intentional.
✅ Verification successful
Emptying IGNORES_OPENSPEC is intentional and safe
The git history shows this is a deliberate change. The const keyword was previously ignored for OpenAPI specs, but its removal from IGNORES_OPENSPEC aligns with modern OpenAPI specifications which support the const keyword for schema validation. The change improves schema consistency across JSON Schema and OpenAPI contexts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of 'const' keyword in schema definitions
# and verify if this change affects OpenAPI schema generation
# Look for const usage in schema definitions
rg -l "const\s*:" --type ts
# Look for potential OpenAPI schema generation tests
rg -l "openapi.*schema" --type test
Length of output: 280
Script:
#!/bin/bash
# Let's search for const-related tests and implementation
rg -A 5 "IGNORES_OPENSPEC" packages/specs/schema/src/components/default/schemaMapper.ts
# Look for const decorator or schema tests
rg -A 10 "const.*schema" packages/specs/schema/src/decorators/common/const.spec.ts
# Check OpenAPI schema generation tests with correct file type
rg -l "openapi.*schema" --type ts
# Look for const usage in schema generation
rg -A 5 "const.*schema" packages/specs/schema/src/domain/JsonSchema.spec.ts
Length of output: 17650
Script:
#!/bin/bash
# Let's check for const decorator implementation and its usage
rg -A 5 "@Const" packages/specs/schema/src/decorators/common/const.ts
# Look for const property handling in OpenAPI context
rg -A 5 "const.*openapi" --type ts
# Check the git history of the IGNORES_OPENSPEC change
git log -p packages/specs/schema/src/components/default/schemaMapper.ts | grep -A 10 "IGNORES_OPENSPEC"
Length of output: 1458
packages/specs/schema/src/index.ts (2)
25-26: LGTM! Well-structured implementation across specifications.
The parallel implementation of discriminator and enum mappers in both default and OpenAPI specifications demonstrates a consistent and well-thought-out architecture.
11-12: Verify the removal of inlineEnumsMapper.js
The renaming from inlineEnumsMapper.js to enumsMapper.js could be a breaking change if the old export was part of the public API.
✅ Verification successful
Based on the git history and grep results, I can now generate the final response:
Renaming of inlineEnumsMapper to enumsMapper is a safe change
The git history shows this is a straightforward rename with identical functionality:
- The mapper's implementation remains the same
- The registration name changed from "inlineEnums" to "enums"
- All references were updated in a single commit
- The change is part of an internal refactoring, not affecting public APIs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of inlineEnumsMapper in the codebase
# Expected: No references to inlineEnumsMapper should exist in the codebase
# Search for any remaining references to inlineEnumsMapper
rg "inlineEnumsMapper" --type ts
# Check git history to understand the usage pattern
git log -p --all -S "inlineEnumsMapper" -- "**/*.ts"
Length of output: 19403
packages/specs/schema/src/utils/transformToOS2.ts (2)
3-3: LGTM! Type safety improvement
The addition of type OS2Security import enhances type safety for security scheme transformations.
108-115: LGTM! Improved security definition handling
The new implementation properly checks for defined security definitions before adding them to the result object, preventing potential undefined values. The type annotation on the reduce accumulator also enhances type safety.
packages/specs/schema/src/domain/JsonSchema.ts (1)
1044-1054: LGTM! Discriminator mapping implementation looks correct.
The implementation properly maps discriminator types to their corresponding schemas, which will allow for correct reference resolution based on group options later.
Let's verify that all discriminator types are properly mapped:
docs/docs/model.md (3)
1462-1466: LGTM! Well-structured enum definition.
The EventType enum is well-defined with clear, descriptive values that follow consistent naming conventions.
1469-1471: LGTM! Clear discriminator key implementation.
The Event base class correctly implements the discriminator pattern with a helpful comment explaining the type property behavior.
1485-1486: LGTM! Proper implementation of multiple discriminator values.
The Action class correctly extends Event and demonstrates proper usage of @DiscriminatorValue with multiple event types.
packages/specs/schema/test/integrations/discriminator.integration.spec.ts (1)
23-27:
Add 'CUSTOM_ACTION' to the 'EventType' enum
To ensure consistency and type safety, add CUSTOM_ACTION to the EventType enum to represent the CustomAction type.
Apply this diff to include the new value:
export enum EventType {
PAGE_VIEW = "page_view",
ACTION = "action",
CLICK_ACTION = "click_action"
+ CUSTOM_ACTION = "custom_action"
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 22-27: Do not export from a test file.
(lint/suspicious/noExportsInTest)
| @@ -4,17 +4,14 @@ import {isProtectedKey} from "./isProtectedKey.js"; | |||
| * @param obj | |||
| * @param ignore | |||
| */ | |||
| export function cleanObject(obj: any, ignore: string[] = []): any { | |||
| export function cleanObject(obj: Record<string, unknown>, ignore: string[] = []): any { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider aligning return type with parameter type
The parameter type is Record<string, unknown> but the return type is any. This inconsistency could lead to type safety issues downstream.
-export function cleanObject(obj: Record<string, unknown>, ignore: string[] = []): any {
+export function cleanObject(obj: Record<string, unknown>, ignore: string[] = []): Record<string, unknown> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function cleanObject(obj: Record<string, unknown>, ignore: string[] = []): any { | |
| export function cleanObject(obj: Record<string, unknown>, ignore: string[] = []): Record<string, unknown> { |
| /** | ||
| * Inline enums when enum instead of using $ref. | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Implementation looks good with architectural considerations.
The addition of inlineEnums provides good flexibility for schema generation. This toggle allows users to choose between referenced vs. inline enum representations, which can be valuable for different use cases (e.g., schema size vs. readability).
Consider the following trade-offs when using this option:
- Inline enums may increase schema size but improve readability
- Referenced enums reduce duplication but require following references
🧹 Nitpick (assertive)
Consider enhancing the documentation with examples.
While the current documentation is clear, it would be helpful to show the difference between inlined and referenced enum representations in the JSDoc.
/**
- * Inline enums when enum instead of using $ref.
+ * Inline enums when enum instead of using $ref.
+ *
+ * @example
+ * // When false (default), enums use $ref:
+ * {
+ * "type": "string",
+ * "$ref": "#/components/schemas/UserType"
+ * }
+ *
+ * // When true, enums are inlined:
+ * {
+ * "type": "string",
+ * "enum": ["admin", "user", "guest"]
+ * }
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Inline enums when enum instead of using $ref. | |
| */ | |
| /** | |
| * Inline enums when enum instead of using $ref. | |
| * | |
| * @example | |
| * // When false (default), enums use $ref: | |
| * { | |
| * "type": "string", | |
| * "$ref": "#/components/schemas/UserType" | |
| * } | |
| * | |
| * // When true, enums are inlined: | |
| * { | |
| * "type": "string", | |
| * "enum": ["admin", "user", "guest"] | |
| * } | |
| */ |
| export function wrapEnumsMapper(obj: any, schema: JsonSchema, options: JsonSchemaOptions) { | ||
| obj = enumsMapper(obj, schema, options); | ||
|
|
||
| if (obj.const) { | ||
| obj.enum = [obj.const]; | ||
| delete obj.const; | ||
| } | ||
|
|
||
| return obj | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider adding JSDoc documentation
The function correctly wraps the base enumsMapper and handles the const-to-enum conversion pattern. The implementation is clean and follows schema transformation best practices.
Consider adding JSDoc documentation to explain:
- The purpose of the wrapper
- The transformation of
consttoenum - Parameter types and return value
+/**
+ * Wraps the base enumsMapper to provide additional enum handling capabilities.
+ * Converts 'const' properties to single-value 'enum' arrays for OpenAPI compatibility.
+ *
+ * @param obj - The object to transform
+ * @param schema - The JSON schema definition
+ * @param options - Schema processing options
+ * @returns The transformed object
+ */
export function wrapEnumsMapper(obj: any, schema: JsonSchema, options: JsonSchemaOptions) {Would you like me to help create a PR to add this documentation?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function wrapEnumsMapper(obj: any, schema: JsonSchema, options: JsonSchemaOptions) { | |
| obj = enumsMapper(obj, schema, options); | |
| if (obj.const) { | |
| obj.enum = [obj.const]; | |
| delete obj.const; | |
| } | |
| return obj | |
| } | |
| /** | |
| * Wraps the base enumsMapper to provide additional enum handling capabilities. | |
| * Converts 'const' properties to single-value 'enum' arrays for OpenAPI compatibility. | |
| * | |
| * @param obj - The object to transform | |
| * @param schema - The JSON schema definition | |
| * @param options - Schema processing options | |
| * @returns The transformed object | |
| */ | |
| export function wrapEnumsMapper(obj: any, schema: JsonSchema, options: JsonSchemaOptions) { | |
| obj = enumsMapper(obj, schema, options); | |
| if (obj.const) { | |
| obj.enum = [obj.const]; | |
| delete obj.const; | |
| } | |
| return obj | |
| } |
🧰 Tools
🪛 Biome
[error] 12-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 8-8: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
| function defaultDiscriminatorMappingMapper(obj: SchemaWithDiscriminator) { | ||
| if (obj.discriminator?.mapping) { | ||
| delete obj.discriminator.mapping; | ||
| } | ||
|
|
||
| return obj; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize performance by avoiding delete operator
The static analysis tool correctly flags the use of the delete operator which can impact performance.
Here's a more performant approach:
function defaultDiscriminatorMappingMapper(obj: SchemaWithDiscriminator) {
if (obj.discriminator?.mapping) {
- delete obj.discriminator.mapping;
+ obj.discriminator.mapping = undefined;
}
return obj;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function defaultDiscriminatorMappingMapper(obj: SchemaWithDiscriminator) { | |
| if (obj.discriminator?.mapping) { | |
| delete obj.discriminator.mapping; | |
| } | |
| return obj; | |
| } | |
| function defaultDiscriminatorMappingMapper(obj: SchemaWithDiscriminator) { | |
| if (obj.discriminator?.mapping) { | |
| obj.discriminator.mapping = undefined; | |
| } | |
| return obj; | |
| } |
🧰 Tools
🪛 Biome
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| export function discriminatorMappingMapper(obj: SchemaWithDiscriminator, _: JsonSchema, options: JsonSchemaOptions) { | ||
| if (obj.discriminator?.mapping) { | ||
| const entries = Object.entries(obj.discriminator.mapping); | ||
| const newMapping: Record<string, string> = {}; | ||
|
|
||
| for (const [key, value] of entries) { | ||
| newMapping[key] = isString(value) ? value : toRef(value, null, options).$ref; | ||
| } | ||
|
|
||
| obj.discriminator.mapping = newMapping; | ||
| } | ||
|
|
||
| return obj; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider implementing immutable transformation
While the function correctly transforms the discriminator mappings, it mutates the input object directly. Consider returning a new object to maintain immutability.
Here's a suggested immutable implementation:
export function discriminatorMappingMapper(obj: SchemaWithDiscriminator, _: JsonSchema, options: JsonSchemaOptions) {
if (obj.discriminator?.mapping) {
const entries = Object.entries(obj.discriminator.mapping);
const newMapping: Record<string, string> = {};
for (const [key, value] of entries) {
newMapping[key] = isString(value) ? value : toRef(value, null, options).$ref;
}
- obj.discriminator.mapping = newMapping;
+ return {
+ ...obj,
+ discriminator: {
+ ...obj.discriminator,
+ mapping: newMapping
+ }
+ };
}
return obj;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function discriminatorMappingMapper(obj: SchemaWithDiscriminator, _: JsonSchema, options: JsonSchemaOptions) { | |
| if (obj.discriminator?.mapping) { | |
| const entries = Object.entries(obj.discriminator.mapping); | |
| const newMapping: Record<string, string> = {}; | |
| for (const [key, value] of entries) { | |
| newMapping[key] = isString(value) ? value : toRef(value, null, options).$ref; | |
| } | |
| obj.discriminator.mapping = newMapping; | |
| } | |
| return obj; | |
| } | |
| export function discriminatorMappingMapper(obj: SchemaWithDiscriminator, _: JsonSchema, options: JsonSchemaOptions) { | |
| if (obj.discriminator?.mapping) { | |
| const entries = Object.entries(obj.discriminator.mapping); | |
| const newMapping: Record<string, string> = {}; | |
| for (const [key, value] of entries) { | |
| newMapping[key] = isString(value) ? value : toRef(value, null, options).$ref; | |
| } | |
| return { | |
| ...obj, | |
| discriminator: { | |
| ...obj.discriminator, | |
| mapping: newMapping | |
| } | |
| }; | |
| } | |
| return obj; | |
| } |
| get(@In("body") payload: Model) { | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding a TODO comment or implementation.
The empty method block could benefit from a comment explaining that it's intentionally empty for testing purposes.
@OperationPath("POST", "/")
get(@In("body") payload: Model) {
+ // Empty method for testing purposes
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get(@In("body") payload: Model) { | |
| } | |
| get(@In("body") payload: Model) { | |
| // Empty method for testing purposes | |
| } |
| item[key] = schema.getJsonType(); | ||
| return item; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Performance improvement noted, but consider immutability implications
The change from object spread to direct mutation of the accumulator object can improve performance by reducing object allocations. However, this introduces mutable state which might make the code harder to reason about and debug.
Consider documenting this design decision or using Object.freeze() if immutability is desired.
- item[key] = schema.getJsonType();
+ Object.freeze(item);
+ return {...item, [key]: schema.getJsonType()};Also applies to: 100-102
| obj = execMapper("enums", [obj, schema], options); | ||
| obj = execMapper("discriminatorMapping", [obj, schema], options); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document the schema transformation pipeline
The order of mapper execution is crucial for correct schema generation. The enum processing and discriminator mapping are placed at the end of the transformation pipeline, which is correct, but this design decision should be documented.
// Process basic schema properties
obj = execMapper("required", [obj, schema], options);
obj = execMapper("nullable", [obj, schema], options);
obj = alterOneOf(obj, schema, options);
+ // Process enums and discriminator mappings last to ensure all type information is available
obj = execMapper("enums", [obj, schema], options);
obj = execMapper("discriminatorMapping", [obj, schema], options);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| obj = execMapper("enums", [obj, schema], options); | |
| obj = execMapper("discriminatorMapping", [obj, schema], options); | |
| // Process basic schema properties | |
| obj = execMapper("required", [obj, schema], options); | |
| obj = execMapper("nullable", [obj, schema], options); | |
| obj = alterOneOf(obj, schema, options); | |
| // Process enums and discriminator mappings last to ensure all type information is available | |
| obj = execMapper("enums", [obj, schema], options); | |
| obj = execMapper("discriminatorMapping", [obj, schema], options); |
| @@ -108,7 +108,7 @@ describe("Discriminator", () => { | |||
| value: "value", | |||
| url: "https://url.com" | |||
| }) | |||
| .expect(200); | |||
| //.expect(200); | |||
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the search results, I can see that status code assertions with .expect(200) are consistently used throughout the codebase's test files, particularly in integration tests. This is a common pattern across different packages and test scenarios, indicating it's a standard practice in the project for verifying successful API responses.
Keep the status code assertion to maintain consistent test coverage.
The status code assertion .expect(200) should not be commented out as it:
- Follows the established testing pattern across the codebase
- Ensures the API maintains its expected behavior
- Helps catch unintended changes in response status codes
- //.expect(200);
+ .expect(200);🔗 Analysis chain
Consider keeping the status code assertion.
The status code check .expect(200) has been commented out. While the test still verifies the response body structure, removing the status code assertion weakens the test coverage. Unless there's a specific reason to remove this check (e.g., the status code is expected to vary), I recommend keeping it to ensure the API maintains its expected behavior.
Let's check if other similar tests in the codebase consistently verify status codes:
- //.expect(200);
+ .expect(200);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar test patterns to see if status code verification is a common practice
rg -B 2 -A 2 '\.expect\(200\)' --type ts
Length of output: 101991
| @@ -62,7 +62,7 @@ function getSupportedMimeTypes(content: any) { | |||
| } | |||
|
|
|||
| export function transformSecurity(securitySchemes: any) { | |||
| function map(security: any) { | |||
| function map(security: any): OS2Security | undefined { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety for flow type
The use of as never type assertion in the flow type assignment might hide potential type issues. Consider using a proper type union or enum instead.
- flow: flowType as never,
+ type FlowType = 'implicit' | 'password' | 'application' | 'accessCode';
+ flow: flowType as FlowType,Also applies to: 99-99
|
🎉 This PR is included in version 7.84.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 8.0.0-rc.5 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Information
Improve discriminator mappings
Todos
Summary by CodeRabbit
New Features
EventTypeenumeration for better type safety in event handling.inlineEnumsoption toJsonSchemaOptionsfor inline enum support.Bug Fixes
toReffunction to prevent issues with undefined schemas.Documentation
Refactor
Chores
.gitignoreto exclude coverage files.