feat: Policy v1 endpoints #479
Conversation
WalkthroughThis pull request introduces new API functionality for handling policies. A new OpenAPI specification file defines the API’s schemas and POST endpoint for creating policies. An associated route file implements a POST endpoint with middleware for authentication, body parsing, and authorization. In the backend, the policy creation function has been updated to return an extended object containing additional properties such as targets and various approval details. These changes enhance the structure and clarity of the API endpoint and its related controller and service interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route
participant Middleware
participant API
Client->>Route: POST /v1/policies
Route->>Middleware: Authenticate, Parse Body, Authorize
Middleware-->>Route: Validated Request
Route->>API: Call createPolicy with Request Body
API-->>Route: Return extended policy object
Route->>Client: Respond 200 OK with policy object
Poem
✨ Finishing Touches
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/webservice/src/app/api/v1/policies/route.ts (1)
26-36: Consider more specific error responsesWhile the current error handling is adequate, consider returning more specific error types to help API consumers troubleshoot issues more effectively.
.catch((error) => { log.error("Failed to create policy", { error }); return NextResponse.json( - { error: "Failed to create policy" }, + { + error: "Failed to create policy", + code: error.code || "POLICY_CREATE_ERROR", + message: process.env.NODE_ENV === "production" + ? "An error occurred while creating the policy" + : error.message + }, { status: INTERNAL_SERVER_ERROR }, ); }),apps/webservice/src/app/api/v1/policies/openapi.ts (3)
90-102: Consider makingdescriptionfield optionalIn the
Policyschema,descriptionis marked as required, but it's a common practice to make description fields optional. This gives users more flexibility when creating policies.required: [ "name", - "description", "priority", "enabled", "workspaceId", "targets", "denyWindows", "deploymentVersionSelector", "versionAnyApprovals", "versionUserApprovals", "versionRoleApprovals", ],
173-194: Consider adding more specific error response typesThe error response schema is very generic. Consider adding more specific error types for different failure scenarios (e.g., validation error, unauthorized, forbidden, etc.) to help API consumers better handle errors.
description: "Internal Server Error", content: { "application/json": { schema: { type: "object", properties: { error: { type: "string" }, + code: { type: "string" }, + message: { type: "string" }, }, }, }, }, }, + description: "Bad Request", + content: { + "application/json": { + schema: { + type: "object", + properties: { + error: { type: "string" }, + issues: { + type: "array", + items: { + type: "object", + properties: { + path: { type: "array", items: { type: "string" } }, + message: { type: "string" } + } + } + } + }, + }, + }, + }, +},
10-103: Add field descriptions to improve API documentationThe schema definitions would benefit from descriptions for each field to improve the API documentation. This helps API consumers understand the purpose and constraints of each field.
Example for the PolicyTarget schema:
PolicyTarget: { type: "object", properties: { - deploymentSelector: { type: "object", additionalProperties: true }, + deploymentSelector: { + type: "object", + additionalProperties: true, + description: "Selector for targeting specific deployments based on attributes" + }, // Add descriptions for other fields }, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webservice/src/app/api/v1/policies/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/policies/route.ts(1 hunks)packages/api/src/router/policy/create.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/api/v1/policies/route.tspackages/api/src/router/policy/create.tsapps/webservice/src/app/api/v1/policies/openapi.ts
🧬 Code Graph Analysis (2)
apps/webservice/src/app/api/v1/policies/route.ts (2)
apps/webservice/src/app/api/v1/auth.ts (2)
authn(15-21)authz(23-56)apps/webservice/src/app/api/v1/body-parser.tsx (1)
parseBody(6-15)
packages/api/src/router/policy/create.ts (1)
packages/db/src/schema/policy.ts (1)
policy(30-46)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
apps/webservice/src/app/api/v1/policies/route.ts (3)
14-14: Clear route logging setupGood practice creating a child logger specifically for this route. This will make log messages more discoverable and easier to filter when troubleshooting issues in production.
16-37: Well-structured API endpoint implementation with proper middleware chainThe implementation correctly uses a middleware chain approach for authentication, body validation, and authorization. This promotes separation of concerns and makes the code more maintainable.
The error handling follows good practices by:
- Logging the error with context
- Returning a user-friendly error message without exposing internal details
- Using standard HTTP status codes
19-25: Authorization check is properly scopedThe authorization middleware correctly verifies the user has permission to create a policy for the specific workspace identified in the request body. This follows the principle of least privilege.
packages/api/src/router/policy/create.ts (1)
92-100: Improved return value with comprehensive policy informationThe return value now includes all related policy data, which provides a more complete view of the created policy. This is a good enhancement that eliminates the need for additional API calls to fetch related information.
apps/webservice/src/app/api/v1/policies/openapi.ts (1)
3-9: Good API documentation setupClear API metadata with version information. This helps with API discoverability and versioning.
| required: ["timeZone", "rrule"], | ||
| }, |
There was a problem hiding this comment.
Inconsistency in required fields across schema definitions
There's an inconsistency in the DenyWindow schema requirements:
- In the components schema (line 26), both
timeZoneandrruleare required - In the request body schema (line 136), only
timeZoneis required
This could lead to validation inconsistencies.
Similar inconsistencies exist for VersionRoleApproval. Ensure that the requirements are consistent across all schema definitions.
// In the request body schema:
required: ["timeZone"],
+required: ["timeZone", "rrule"],Also applies to: 136-137
| deploymentVersionSelector: { | ||
| $ref: "#/components/schemas/DeploymentVersionSelector", | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider stricter validation for deploymentVersionSelector
The deploymentVersionSelector field allows any additional properties (additionalProperties: true), which might be too permissive and could lead to unexpected data being accepted.
Consider using a more specific schema that only allows known properties, or at least document the expected structure more clearly.
🌐 Web query:
What are best practices for handling additional properties in OpenAPI schemas?
💡 Result:
Handling additional properties in OpenAPI schemas requires balancing flexibility with strictness to ensure API reliability and clarity. Here are key best practices based on industry standards and expert recommendations:
1. Default to Strict Validation with additionalProperties: false
- Enforce explicit property definitions by setting
additionalProperties: falsein schemas where no dynamic fields are expected[7]. This prevents API clients from sending unexpected data and ensures documentation accuracy[2][7]. - Example:
components: schemas: StrictObject: type: object properties: name: type: string additionalProperties: false
2. Use additionalProperties: true for Controlled Flexibility
- Allow dynamic but unstructured data in scenarios like user-generated content or future-proof extensibility[2][5]. This enables APIs to accept unknown properties without schema updates[2][7].
- Example:
components: schemas: FlexibleObject: type: object properties: title: type: string additionalProperties: true
3. Restrict to Typed Additional Properties
- Constrain additional properties to specific types (e.g.,
type: stringortype: integer) to maintain data consistency while allowing extensibility[2][5][7]. This is critical for scenarios like metadata storage or configuration objects. - Example:
components: schemas: StringMap: type: object properties: category: type: string additionalProperties: type: string
4. Avoid Overusing Additional Properties
- Do not use
additionalPropertiesas a shortcut to avoid documenting well-known fields[7]. Explicitly define all expected properties to improve API discoverability and reduce client-side validation errors[7][10].
5. Consider Code Generation Implications
- Test generated client/server code to ensure compatibility with
additionalPropertiesconfigurations. For example:additionalProperties: trueoften translates toMap<String, Object>in Java[7].- Typed additional properties (e.g.,
type: integer) may generateMap<String, Integer>[5][7].
6. Document Usage Clearly
- Specify in descriptions when and why additional properties are allowed[2][10]. For example:
components: schemas: UserMetadata: description: >- Key-value pairs for user-defined metadata. Additional properties must be strings. type: object additionalProperties: type: string
7. Align with JSON Schema Behavior
- In OpenAPI 3.1+,
additionalPropertiesfollows JSON Schema semantics, where:- Unspecified
additionalPropertiesdefaults totrue[9]. - Use
falseto block extras or a schema to validate them[7][9].
- Unspecified
Comparison of Approaches
| Approach | Use Case | Code Example |
|---|---|---|
additionalProperties: false |
Strict data contracts | Prevents unexpected fields[7] |
additionalProperties: true |
Dynamic/unstructured data | Accepts any type[2][5] |
additionalProperties: {type: X} |
Type-safe extensibility | Ensures data integrity[2][7] |
By following these practices, developers can create OpenAPI schemas that are both robust for API consumers and adaptable to evolving requirements[2][7].
Citations:
- 1: https://swagger.io/specification/
- 2: https://www.speakeasy.com/guides/openapi/additionalproperties
- 3: https://swagger.io/docs/specification/v3_0/data-models/inheritance-and-polymorphism/
- 4: https://docs.aws.amazon.com/amazonq/latest/qbusiness-ug/plugins-api-schema-best-practices.html
- 5: https://www.ibm.com/docs/en/zos-connect/3.0.0?topic=api-using-additional-properties
- 6: https://www.apimatic.io/blog/2022/11/14-best-practices-to-write-openapi-for-better-api-consumption
- 7: https://www.apimatic.io/openapi/additionalproperties
- 8: https://learn.openapis.org/best-practices.html
- 9: Make it clear that
additionalPropertiescan only have a schema value OAI/OpenAPI-Specification#668 - 10: https://www.getambassador.io/blog/openapi-specification-structure-best-practices
Action: Refine DeploymentVersionSelector Schema Validation
The current definition of deploymentVersionSelector (lines 139–141 in apps/webservice/src/app/api/v1/policies/openapi.ts) references a schema that, based on its usage, defaults to allowing additional properties. This lax validation could inadvertently allow unexpected or malformed data.
Based on best practices:
- If only known properties are expected: Refine the schema by explicitly defining allowed properties and set
additionalProperties: false. - If some level of flexibility is needed: Consider constraining additional properties to a specific type (e.g.,
type: string) or document the expected structure clearly.
Please update the DeploymentVersionSelector schema accordingly to adhere to a stricter validation strategy, ensuring better API reliability and improved client-side validation.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
apps/webservice/src/app/api/v1/policies/openapi.ts (2)
25-27:⚠️ Potential issueInconsistency in DenyWindow required fields
There's a mismatch between the schema definitions:
- In the components schema (line 26), both
timeZoneandrruleare required- In the request body schema (line 136-137), only
timeZoneis requiredThis inconsistency could lead to validation issues where a request passes validation but fails when processed by backend logic.
required: ["timeZone"], + required: ["timeZone", "rrule"],Also applies to: 136-137
32-35: 🛠️ Refactor suggestionImprove schema validation for DeploymentVersionSelector
Using
additionalProperties: truecreates a lax validation that could allow unexpected data through the API. Consider defining a more specific schema that explicitly lists allowed properties.deploymentVersionSelector: { type: "object", - additionalProperties: true, + additionalProperties: false, + properties: { + // Define specific allowed properties here + } },openapi.v1.json (3)
4389-4392:⚠️ Potential issueInconsistency in DenyWindow required fields
Similar to the TypeScript file, there's a mismatch between the schema definitions:
- In the components schema (line 4389-4392), both
timeZoneandrruleare required- In the request body schema (line 1481-1484), only
timeZoneis requiredAlso applies to: 1481-1484
4445-4448:⚠️ Potential issueInconsistency in VersionRoleApproval required fields
Similar to the TypeScript file, there's a mismatch between the schema definitions:
- In the components schema (line 4445-4448), both
roleIdandrequiredApprovalsCountare required- In the request body schema (line 1519-1521), only
roleIdis requiredAlso applies to: 1519-1521
4400-4403: 🛠️ Refactor suggestionImprove schema validation for DeploymentVersionSelector
Using
additionalProperties: truecreates a lax validation that could allow unexpected data through the API.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/api/v1/policies/openapi.ts(1 hunks)openapi.v1.json(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/api/v1/policies/openapi.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
apps/webservice/src/app/api/v1/policies/openapi.ts (3)
3-8: LGTM: API metadata is properly definedThe OpenAPI specification metadata is correctly defined with appropriate version information.
108-112: LGTM: POST endpoint is well-definedThe
/v1/policiesendpoint is properly defined with a clear summary and operation ID.
182-204: LGTM: Response definitions are comprehensiveThe response definitions include both success (200) and error (500) cases with appropriate schemas.
openapi.v1.json (2)
1432-1563: LGTM: Policy endpoint implementation matches TypeScript specThe POST endpoint for
/v1/policiesis correctly implemented with the same schema structure as defined in the TypeScript specification file.
2225-2382: LGTM: Resource schemas endpoints are well-definedThe resource schemas endpoints (
/v1/resource-schemas) are properly defined with appropriate request/response structures for both creation and deletion operations.
| required: ["roleId", "requiredApprovalsCount"], | ||
| }, |
There was a problem hiding this comment.
Inconsistency in VersionRoleApproval required fields
The required fields are inconsistent between schema definitions:
- In the components schema (line 56), both
roleIdandrequiredApprovalsCountare required - In the request body schema (line 167), only
roleIdis required
This could lead to unexpected validation errors if the backend expects requiredApprovalsCount but the API doesn't enforce it.
required: ["roleId"],
+ required: ["roleId", "requiredApprovalsCount"],Also applies to: 167-168
| deploymentSelector: { type: "object", additionalProperties: true }, | ||
| environmentSelector: { type: "object", additionalProperties: true }, | ||
| releaseSelector: { type: "object", additionalProperties: true }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen validation for selector objects
All selector objects currently use additionalProperties: true, which is too permissive and could lead to unexpected data being accepted. This approach makes it difficult to validate client-provided input and might cause issues during processing.
deploymentSelector: { type: "object", additionalProperties: true },
environmentSelector: { type: "object", additionalProperties: true },
releaseSelector: { type: "object", additionalProperties: true },Consider using more specific schemas with explicit properties and set additionalProperties: false to ensure only expected data is allowed.
Committable suggestion skipped: line range outside the PR's diff.
| "Policy1": { | ||
| "type": "object", | ||
| "properties": { | ||
| "id": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| }, | ||
| "name": { | ||
| "type": "string" | ||
| }, | ||
| "description": { | ||
| "type": "string" | ||
| }, | ||
| "priority": { | ||
| "type": "number" | ||
| }, | ||
| "createdAt": { | ||
| "type": "string", | ||
| "format": "date-time" | ||
| }, | ||
| "enabled": { | ||
| "type": "boolean" | ||
| }, | ||
| "workspaceId": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| }, | ||
| "targets": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/PolicyTarget" | ||
| } | ||
| }, | ||
| "denyWindows": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/DenyWindow" | ||
| } | ||
| }, | ||
| "deploymentVersionSelector": { | ||
| "$ref": "#/components/schemas/DeploymentVersionSelector" | ||
| }, | ||
| "versionAnyApprovals": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/VersionAnyApproval" | ||
| } | ||
| }, | ||
| "versionUserApprovals": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/VersionUserApproval" | ||
| } | ||
| }, | ||
| "versionRoleApprovals": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/VersionRoleApproval" | ||
| } | ||
| } | ||
| }, | ||
| "required": [ | ||
| "id", | ||
| "name", | ||
| "priority", | ||
| "createdAt", | ||
| "enabled", | ||
| "workspaceId", | ||
| "targets", | ||
| "denyWindows", | ||
| "versionUserApprovals", | ||
| "versionRoleApprovals" | ||
| ] | ||
| } |
There was a problem hiding this comment.
Schema naming inconsistency with TypeScript definition
The schema in the JSON file is named Policy1, while in the TypeScript file it's named Policy. This inconsistency might cause reference issues if not handled correctly.
Consider aligning the naming convention between both files to avoid confusion and potential bugs.
Summary by CodeRabbit
New Features
Documentation