Mark openapi attributes as required for reads #1772
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on enhancing the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/plugins/openapi/src/rest-generator.ts (2)
Line range hint
958-970: Clarify ID requirement in create modeIn the
generateModelEntitymethod (lines 958-970), when in 'create' mode, you conditionally require theidfield if there are ID fields without default values. Consider adding a comment or refining the logic to clarify under what circumstances theidfield is required during creation, as this can impact client implementations.If the API allows clients to provide their own IDs when creating resources, ensure this is clearly documented in the OpenAPI specification.
Line range hint
606-621: Potential overlap in filter parameters for relationship fieldsIn the
generateFilterParametersmethod (lines 606-621), relationship fields are skipped for filtering due to the comment:// TODO: how to express nested filters?However, clients may need to filter on related resources. Consider implementing support for nested filters or updating the OpenAPI specification to document the current limitations.
Implementing nested filtering would enhance the API's usability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/plugins/openapi/tests/baseline/rest-3.0.0.baseline.yamlis excluded by!**/*.yamlpackages/plugins/openapi/tests/baseline/rest-3.1.0.baseline.yamlis excluded by!**/*.yaml
📒 Files selected for processing (1)
- packages/plugins/openapi/src/rest-generator.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
packages/plugins/openapi/src/rest-generator.ts (6)
877-879: Correctly marking all fields as required in read modeBy adding all fields to the
requiredarray whenmode === 'read'(lines 877-879), the OpenAPI schema now accurately reflects that all attributes are included in the response, aligning with the JSON:API specification. This ensures that the generated documentation is consistent with the actual API behavior.
875-879: Ensure nullable fields are appropriately handled in the schemaWhile all fields are marked as required in read mode, please verify that nullable fields are correctly represented in the OpenAPI schema. In OpenAPI, a field can be required but still allow
nullvalues ifnullable: trueis set. This distinction informs clients that a field will always be present but may contain anullvalue.To confirm that nullable fields are properly handled, you can review the schema generation logic for nullable attributes.
Line range hint
992-1003: Accurate mapping of field types to OpenAPI schemaThe
fieldTypeToOpenAPISchemamethod provides a comprehensive and accurate mapping of data model field types to OpenAPI schema types. Utilizingts-patternfor pattern matching enhances readability and maintainability.
Line range hint
923-934: Include relationships in response schemasIn the
generateDataModelComponentsmethod (lines 923-934), you've added relationships to the response schemas for both single and list responses. This inclusion ensures that the generated OpenAPI specification properly documents the relationships between resources, which is crucial for clients to navigate the API effectively.
875-879: Consistent handling of required properties across modesThere appears to be differing logic for determining required fields between 'create' and 'read' modes (lines 875-879 and 958-970). In 'create' mode, fields without defaults are required unless they are optional or array relations. In 'read' mode, all fields are marked as required. Verify that this inconsistency is intentional and that it reflects the API's expected behavior.
Consider whether the required properties in 'create' mode should also include non-nullable fields to ensure consistency.
Also applies to: 958-970
Line range hint
549-570: Ensure security schemes are correctly appliedThe methods generating operations (e.g.,
makeResourceList,makeResourceCreate, etc.) include asecurityproperty that depends on resource metadata and policy analysis (lines 549-570). Verify that the security requirements are accurately reflected based on the access control policies, ensuring that the generated OpenAPI specification enforces the intended authentication and authorization constraints.Consider adding tests to validate that the security schemes are correctly assigned to each operation.
|
As far as I can tell the test fails aren't related to my changes here(?) |
|
I've only been running the tests within the openapi plugin folder and they succeed locally. I don't have enough memory to comfortably run the entire test suite locally. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/plugins/openapi/src/rest-generator.ts (3)
Line range hint
812-916: Consider refactoringgenerateModelEntityfor improved readability and maintainability.The
generateModelEntitymethod is quite long and handles multiple responsibilities. Consider breaking it down into smaller, more focused methods to improve readability and maintainability. Here's a suggested refactor:private generateModelEntity(model: DataModel, mode: 'read' | 'create' | 'update'): OAPI.SchemaObject { - const idFields = model.fields.filter((f) => isIdField(f)); - // For compound ids, each component is also exposed as a separate field - const fields = idFields.length > 1 ? model.fields : model.fields.filter((f) => !isIdField(f)); - - const attributes: Record<string, OAPI.SchemaObject> = {}; - const relationships: Record<string, OAPI.ReferenceObject | OAPI.SchemaObject> = {}; - - const required: string[] = []; - - for (const field of fields) { - if (isRelationshipField(field)) { - let relType: string; - if (mode === 'create' || mode === 'update') { - relType = field.type.array ? '_toManyRelationship' : '_toOneRelationship'; - } else { - relType = field.type.array ? '_toManyRelationshipWithLinks' : '_toOneRelationshipWithLinks'; - } - relationships[field.name] = this.wrapNullable(this.ref(relType), field.type.optional); - } else { - attributes[field.name] = this.generateField(field); - if ( - mode === 'create' && - !field.type.optional && - !hasAttribute(field, '@default') && - // collection relation fields are implicitly optional - !(isDataModel(field.$resolvedType?.decl) && field.type.array) - ) { - required.push(field.name); - } else if (mode === 'read') { - // Until we support sparse fieldsets, all fields are required for read operations - required.push(field.name); - } - } - } + const { attributes, relationships, required } = this.generateAttributesAndRelationships(model, mode); + const properties = this.generateProperties(model, mode, attributes, relationships); + const toplevelRequired = this.generateToplevelRequired(model, mode); - const toplevelRequired = ['type', 'attributes']; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let properties: any = { - type: { type: 'string' }, - attributes: { - type: 'object', - required: required.length > 0 ? required : undefined, - properties: attributes, - }, - }; - - if (mode === 'create') { - // 'id' is required if there's no default value - const idFields = model.fields.filter((f) => isIdField(f)); - if (idFields.length && idFields.every((f) => !hasAttribute(f, '@default'))) { - properties = { id: { type: 'string' }, ...properties }; - toplevelRequired.unshift('id'); - } - } else { - // 'id' always required for read and update - properties = { id: { type: 'string' }, ...properties }; - toplevelRequired.unshift('id'); - } // eslint-disable-next-line @typescript-eslint/no-explicit-any const result: any = { type: 'object', description: `The "${model.name}" model`, required: toplevelRequired, properties, } satisfies OAPI.SchemaObject; - if (Object.keys(relationships).length > 0) { - result.properties.relationships = { - type: 'object', - properties: relationships, - }; - } + this.addRelationshipsToResult(result, relationships); return result; } +private generateAttributesAndRelationships(model: DataModel, mode: 'read' | 'create' | 'update') { + // Implementation details... +} + +private generateProperties(model: DataModel, mode: 'read' | 'create' | 'update', attributes: Record<string, OAPI.SchemaObject>, relationships: Record<string, OAPI.ReferenceObject | OAPI.SchemaObject>) { + // Implementation details... +} + +private generateToplevelRequired(model: DataModel, mode: 'read' | 'create' | 'update') { + // Implementation details... +} + +private addRelationshipsToResult(result: any, relationships: Record<string, OAPI.ReferenceObject | OAPI.SchemaObject>) { + // Implementation details... +}This refactoring breaks down the large method into smaller, more focused methods. Each new method handles a specific aspect of the entity generation, making the code more modular and easier to maintain.
Line range hint
926-945: Enhance error handling and documentation infieldTypeToOpenAPISchema.While the method effectively maps Prisma field types to OpenAPI schema types, consider the following improvements:
- Add error handling for unknown types:
private fieldTypeToOpenAPISchema(type: DataModelFieldType): OAPI.ReferenceObject | OAPI.SchemaObject { return match(type.type) .with('String', () => ({ type: 'string' })) .with(P.union('Int', 'BigInt'), () => ({ type: 'integer' })) .with('Float', () => ({ type: 'number' })) .with('Decimal', () => this.oneOf({ type: 'number' }, { type: 'string' })) .with('Boolean', () => ({ type: 'boolean' })) .with('DateTime', () => ({ type: 'string', format: 'date-time' })) .with('Bytes', () => ({ type: 'string', format: 'byte', description: 'Base64 encoded byte array' })) .with('Json', () => ({})) .otherwise((t) => { const fieldDecl = type.reference?.ref; - invariant(fieldDecl, `Type ${t} is not a model reference`); - return this.ref(fieldDecl?.name); + if (!fieldDecl) { + throw new Error(`Unsupported field type: ${t}`); + } + return this.ref(fieldDecl.name); }); }
- Add JSDoc comments to improve documentation:
+/** + * Converts a Prisma field type to an OpenAPI schema object. + * @param type - The Prisma field type to convert. + * @returns An OpenAPI schema object or reference. + * @throws {Error} If an unsupported field type is encountered. + */ private fieldTypeToOpenAPISchema(type: DataModelFieldType): OAPI.ReferenceObject | OAPI.SchemaObject { // ... (existing implementation) }These changes will improve error handling for unsupported types and provide better documentation for developers working with this method.
Line range hint
1-1010: Overall assessment: Solid implementation with room for improvementThe
RESTfulOpenAPIGeneratorclass provides a comprehensive implementation for generating OpenAPI specifications for RESTful APIs. The code is generally well-structured and covers various aspects of the OpenAPI specification.Main areas for improvement:
- Code organization: Consider breaking down larger methods (e.g.,
generateModelEntity) into smaller, more focused methods to improve readability and maintainability.- Error handling: Enhance error handling in methods like
fieldTypeToOpenAPISchemato provide more informative error messages for unsupported types.- Documentation: Add JSDoc comments to key methods to improve code documentation and make it easier for other developers to understand and maintain the code.
- Configuration options: Consider adding more configuration options, such as the ability to specify required fields in read operations, to provide greater flexibility in API design.
Addressing these points will further enhance the quality and maintainability of the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/plugins/openapi/src/rest-generator.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/plugins/openapi/src/rest-generator.ts (1)
Line range hint
918-924: LGTM: ThegenerateFieldmethod is well-implemented.The
generateFieldmethod effectively handles the generation of OpenAPI schema for individual fields, including support for array types and nullable fields. The implementation is concise and clear.
Hey it's caused by a typing issue with new release of tanstack-query. Test cases previously didn't pin to a specific dependency version. I've made a fix and merged it into your branch. |
Fixes #1744 by marking all fields as required for read requests. This allows for more accurate documentation and better type safety for generated clients, until sparse fieldsets are implemented.