Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new OpenAPI POST /api/2/sites endpoint (operationId: createSiteFromBlueprint) with a discriminated request body for blueprint vs remote repository site creation, four new request/auth schemas, and a required-property name mismatch in the remote request schema; also re-indents one existing GET operation and a schema. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Auth
participant Repo
Note over Client,API: Create site (POST /api/2/sites)
Client->>API: POST /api/2/sites (CreateSiteFromBlueprintRequest or CreateSiteFromRemoteRequest)
API->>Auth: validate credentials (CreateSiteRemoteAuth oneOf)
Auth-->>API: auth result
API->>Repo: fetch repository at remoteUrl (if remote flow)
Repo-->>API: repo/archive or error
API->>API: create site from blueprint/archive
API-->>Client: 201 Created (ApiResponse) or error (400/401/409/500)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/main/api/studio-api.yaml (1)
1931-1933: Rename the operationId to match the endpoint.
createSiteFromBlueprintis misleading oncesource: remoteis supported, and generated clients will expose that name as part of their public API.✏️ Suggested rename
- operationId: createSiteFromBlueprint + operationId: createSite🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 1931 - 1933, Rename the OpenAPI operationId createSiteFromBlueprint to a neutral name that matches the endpoint intent (e.g., createSite) so it no longer implies blueprint-only behavior; update the operationId value in src/main/api/studio-api.yaml (operationId: createSite) and regenerate any client code or references that rely on createSiteFromBlueprint to avoid breaking public API surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/api/studio-api.yaml`:
- Around line 8896-8910: The CreateSiteRequest schema overloads the JSON key
"name" for both the site name and the remote repository name; update the OpenAPI
schema so the site-level "name" remains the site name and the repository block
uses a distinct key (e.g., "remoteName") and/or "remoteUrl" to match the backend
model (see RemoteRepository.java). Edit the CreateSiteRequest properties and any
nested remote-related properties to replace the duplicated "name" with
"remoteName" (and ensure remoteUrl is present where needed), update descriptions
to reflect the change, and confirm all references in the spec that used the old
"name" for remote repository are updated accordingly so the request can carry
both site name and custom remote name.
- Around line 1948-1966: The examples under examples.createFromBlueprint and
examples.createFromRemote are invalid because they omit the required siteId and
use field names that don’t match the schema: change createFromBlueprint to
include siteId and rename blueprintId to blueprint (matching the schema), and
change createFromRemote to include siteId and rename repositoryUrl to url (keep
remoteBranch and authentication as-is); update those example objects so their
keys exactly match the request schema (siteId, blueprint, url, remoteBranch,
authentication).
- Around line 1967-1982: The v2 create-site responses block currently lists 201,
400, 401 and 500 but omits the duplicate-site conflict (409) documented
elsewhere as CODE: 5001; add a '409' response entry to this operation's
responses that references the shared conflict response schema (or defines a
response pointing to the existing CODE: 5001 payload) so the contract exposes
the "Site already exists" failure path; update the same responses object that
contains the '201'/'400'/'401'/'500' entries and reference the common response
or schema used by other create endpoints for consistency.
- Around line 8922-8931: The CreateSiteFromBlueprintRequest schema is missing a
siteParams bag to carry blueprint descriptor parameters; update the
CreateSiteFromBlueprintRequest (which already allOf CreateSiteRequest) to
include a new optional property siteParams that matches the existing usage in
CreateSiteFromMarketplace (preferably $ref to the shared SiteParams schema if
one exists, otherwise use type: object with additionalProperties: true) so v2
can represent parameterized blueprint creation and align with
available_blueprints and CreateSiteFromMarketplace.
---
Nitpick comments:
In `@src/main/api/studio-api.yaml`:
- Around line 1931-1933: Rename the OpenAPI operationId createSiteFromBlueprint
to a neutral name that matches the endpoint intent (e.g., createSite) so it no
longer implies blueprint-only behavior; update the operationId value in
src/main/api/studio-api.yaml (operationId: createSite) and regenerate any client
code or references that rely on createSiteFromBlueprint to avoid breaking public
API surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58d92ead-feab-4127-a910-003b863ff4d1
📒 Files selected for processing (1)
src/main/api/studio-api.yaml
jmendeza
force-pushed
the
feature/6061
branch
2 times, most recently
from
e57753a to
ac5bc50
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/api/studio-api.yaml (2)
8954-8955:⚠️ Potential issue | 🟠 MajorRequire
remoteUrl, not undefinedurl.Line 8943 declares
remoteUrl, but Line 8955 makesurlrequired. That leaves the actual repository URL optional for validators/generated clients and makes the schema self-contradictory.🔧 Minimal fix
required: - - url + - remoteUrlRun the following script to confirm the mismatch; expected output includes
undefined required entries: ['url']:#!/bin/bash python - <<'PY' try: import yaml except ModuleNotFoundError: import subprocess, sys subprocess.check_call([sys.executable, "-m", "pip", "install", "--quiet", "pyyaml"]) import yaml with open("src/main/api/studio-api.yaml") as fh: doc = yaml.safe_load(fh) schema = doc["components"]["schemas"]["CreateSiteFromRemoteRequest"]["allOf"][1] props = set(schema.get("properties", {})) required = set(schema.get("required", [])) print("properties:", sorted(props)) print("required:", sorted(required)) print("undefined required entries:", sorted(required - props)) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 8954 - 8955, The schema CreateSiteFromRemoteRequest has properties listing remoteUrl but its required array incorrectly lists url; update the required entry to "remoteUrl" (or alternatively add a top-level "url" property if that was intended) so the required array matches the properties; locate the allOf entry for CreateSiteFromRemoteRequest and replace "url" with "remoteUrl" in the required list to remove the undefined required entry.
8927-8936:⚠️ Potential issue | 🟠 MajorBlueprint creation is missing fields the bound DTO requires.
src/main/java/org/craftercms/studio/model/rest/marketplace/CreateSiteRequest.java:36-60expectsblueprintIdandblueprintVersion, andsrc/main/java/org/craftercms/studio/impl/v2/service/marketplace/internal/MarketplaceServiceInternalImpl.java:446-474validatessiteParams. With onlyblueprint, this schema can't describe the versioned, parameterized request the endpoint actually accepts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 8927 - 8936, The OpenAPI schema CreateSiteFromBlueprintRequest is missing fields required by the server-side DTO/validation; update this component so it extends CreateSiteRequest and defines properties blueprintId (string) and blueprintVersion (string) as required (matching org.craftercms.studio.model.rest.marketplace.CreateSiteRequest), and add a siteParams property (object with additionalProperties: true or an appropriate schema) to match the parameters validated in MarketplaceServiceInternalImpl (the code that inspects siteParams). Ensure the property names exactly match blueprintId, blueprintVersion and siteParams so the generated DTOs and request validation align.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/api/studio-api.yaml`:
- Around line 1940-1947: The OpenAPI spec exposes a 'remote' branch that the
backend doesn't support; remove the 'remote' mapping and the oneOf reference to
'#/components/schemas/CreateSiteFromRemoteRequest' (leave only the 'blueprint'
mapping/ref) by editing the discriminator block (propertyName: source, mapping:
... ) and the oneOf array in src/main/api/studio-api.yaml; this prevents
advertising remoteUrl/remoteBranch/authentication until the controller
(org.craftercms.studio.controller.rest.v2.SitesController) and the
CreateSiteRequest binding and MarketplaceServiceInternalImpl logic support it.
---
Duplicate comments:
In `@src/main/api/studio-api.yaml`:
- Around line 8954-8955: The schema CreateSiteFromRemoteRequest has properties
listing remoteUrl but its required array incorrectly lists url; update the
required entry to "remoteUrl" (or alternatively add a top-level "url" property
if that was intended) so the required array matches the properties; locate the
allOf entry for CreateSiteFromRemoteRequest and replace "url" with "remoteUrl"
in the required list to remove the undefined required entry.
- Around line 8927-8936: The OpenAPI schema CreateSiteFromBlueprintRequest is
missing fields required by the server-side DTO/validation; update this component
so it extends CreateSiteRequest and defines properties blueprintId (string) and
blueprintVersion (string) as required (matching
org.craftercms.studio.model.rest.marketplace.CreateSiteRequest), and add a
siteParams property (object with additionalProperties: true or an appropriate
schema) to match the parameters validated in MarketplaceServiceInternalImpl (the
code that inspects siteParams). Ensure the property names exactly match
blueprintId, blueprintVersion and siteParams so the generated DTOs and request
validation align.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b70cd188-04f9-45b7-baee-be1c56a571aa
📒 Files selected for processing (1)
src/main/api/studio-api.yaml
jmendeza
force-pushed
the
feature/6061
branch
2 times, most recently
from
0eafde0 to
f11034b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/api/studio-api.yaml (1)
1931-1989:⚠️ Potential issue | 🟠 MajorDocument the interim behavior of the
remotebranch.This spec keeps
sourceType=remotein the v2 design, but the provided controller context still only shows/create_site_from_marketplace, and this operation does not tell clients what to expect in the meantime. Please mark the remote path as planned and add a501response so the contract does not read as fully available yet.Based on learnings, For API v2 OpenAPI specs (YAML files under src/main/api/), it is acceptable to keep forward-looking request/response variants and discriminator branches in the spec even when the backend implementation is not complete yet. Instead of removing the planned design, document the planned/expected status and interim behavior for the not-yet-implemented paths (e.g., specify 501 Not Implemented or the appropriate placeholder response), so consumers know what will be supported and what response to expect in the interim.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 1931 - 1989, The createSiteFromBlueprint operation (operationId: createSiteFromBlueprint) currently includes a discriminator branch for sourceType="remote" but the backend only supports the marketplace route; update the OpenAPI operation to mark the remote variant as planned by adding a 501 Not Implemented response for the operation and a short description indicating interim behavior for sourceType=remote. Specifically, in the responses block for createSiteFromBlueprint add a '501' response (description: Not Implemented / Planned) and provide a content example (e.g., application/json referencing the existing ApiResponse schema or a brief message) so clients know that requests with sourceType: "remote" will receive 501 until the remote implementation is available. Ensure the operationId createSiteFromBlueprint and the discriminator mapping for sourceType: remote remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/api/studio-api.yaml`:
- Around line 1940-1969: The request shape for site creation is inconsistent:
the operation uses a discriminator on sourceType with
CreateSiteFromBlueprintRequest and CreateSiteFromRemoteRequest expecting a
nested source object, but elsewhere (schema named source and the
CreateSiteFromRemoteRequest fields) the contract is flat and/or expects a string
enum; fix by making the shape consistent—either (A) keep the discriminator and
update CreateSiteFromBlueprintRequest and CreateSiteFromRemoteRequest to define
a nested "source" object (with blueprint or
remoteUrl/remoteName/remoteBranch/authentication) and update the "source"
enum/type to an object or remove that flat "source" schema, or (B) flatten the
operation and examples to use top-level blueprint/remoteUrl fields and remove
the nested "source" object and discriminator; update the examples
(createFromBlueprint/createFromRemote) and the schema definitions
CreateSiteFromBlueprintRequest and CreateSiteFromRemoteRequest to match the
chosen model so validators/generators agree.
---
Duplicate comments:
In `@src/main/api/studio-api.yaml`:
- Around line 1931-1989: The createSiteFromBlueprint operation (operationId:
createSiteFromBlueprint) currently includes a discriminator branch for
sourceType="remote" but the backend only supports the marketplace route; update
the OpenAPI operation to mark the remote variant as planned by adding a 501 Not
Implemented response for the operation and a short description indicating
interim behavior for sourceType=remote. Specifically, in the responses block for
createSiteFromBlueprint add a '501' response (description: Not Implemented /
Planned) and provide a content example (e.g., application/json referencing the
existing ApiResponse schema or a brief message) so clients know that requests
with sourceType: "remote" will receive 501 until the remote implementation is
available. Ensure the operationId createSiteFromBlueprint and the discriminator
mapping for sourceType: remote remain intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e92921e2-8a6b-4808-8c8b-ff7b3e4fa83f
📒 Files selected for processing (1)
src/main/api/studio-api.yaml
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/main/api/studio-api.yaml (3)
8938-8956:⚠️ Potential issue | 🟠 MajorRequire
remoteUrl, not a nonexistenturlfield.Line 8955 still requires
url, but this schema only definesremoteUrl. That makes every valid remote request — including the example on Line 1964 — fail validation.🔧 Suggested fix
required: - - url + - remoteUrl🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 8938 - 8956, The CreateSiteFromRemoteRequest schema erroneously lists required: - url while the schema defines remoteUrl; update the required array in CreateSiteFromRemoteRequest to require remoteUrl instead of url so validation matches the defined properties (refer to CreateSiteFromRemoteRequest and its properties remoteUrl, remoteName, remoteBranch, authentication).
1931-1987:⚠️ Potential issue | 🟠 MajorDocument that
sourceType=remoteis still planned.The
remotebranch is intentionally forward-looking, but this operation still reads as fully available. Please call out the planned status and advertise the interim response so clients do not treat it as live today.📝 Suggested doc-only update
- description: 'Required Permission: "create_site"' + description: >- + Required Permission: "create_site" + Note: `sourceType=remote` is planned but not yet implemented; requests may return `501 Not Implemented`. ... '409': $ref: '#/components/responses/Conflict' '401': $ref: '#/components/responses/Unauthorized' + '501': + description: Not Implemented + content: + application/json: + schema: + type: object + properties: + response: + $ref: '#/components/schemas/ApiResponse' '500': $ref: '#/components/responses/InternalServerError'Based on learnings: forward-looking request variants may stay in
src/main/api/studio-api.yaml, but the planned/interim behavior should be documented (for example,501 Not Implemented).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 1931 - 1987, The OpenAPI operation createSiteFromBlueprint exposes a forward-looking "remote" variant via the discriminator propertyName sourceType and the CreateSiteFromRemoteRequest schema but doesn't document its planned/unimplemented status; update the operationId createSiteFromBlueprint documentation to mark sourceType=remote as "planned" (not yet implemented), add the expected interim response (e.g., 501 Not Implemented) to responses, and include a clear note in the requestBody description explaining clients should treat remote as future work and that current behavior will return the interim response when sourceType=remote.
8927-8936:⚠️ Potential issue | 🟠 MajorKeep
siteParamson blueprint creation.
available_blueprintsalready exposes descriptor parameters, andCreateSiteFromMarketplacealready accepts them. Without the same bag here, the new v2 endpoint cannot represent parameterized blueprint creation.➕ Suggested schema addition
CreateSiteFromBlueprintRequest: allOf: - $ref: '#/components/schemas/CreateSiteRequest' - type: object properties: blueprint: type: string description: The ID of the blueprint to use + siteParams: + type: object + description: Object containing all parameters for the blueprint. It should include all required parameters from the descriptor + additionalProperties: + type: string required: - blueprint🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 8927 - 8936, The CreateSiteFromBlueprintRequest schema is missing the siteParams bag needed to represent parameterized blueprint creation; update the CreateSiteFromBlueprintRequest (which currently allOf CreateSiteRequest) to include the same siteParams property shape that available_blueprints exposes and that CreateSiteFromMarketplace accepts (i.e., add a properties.siteParams entry with the same type/structure and include it in required if CreateSiteRequest requires it) so the v2 endpoint can accept descriptor parameters for blueprint creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/api/studio-api.yaml`:
- Around line 8938-8956: The CreateSiteFromRemoteRequest schema erroneously
lists required: - url while the schema defines remoteUrl; update the required
array in CreateSiteFromRemoteRequest to require remoteUrl instead of url so
validation matches the defined properties (refer to CreateSiteFromRemoteRequest
and its properties remoteUrl, remoteName, remoteBranch, authentication).
- Around line 1931-1987: The OpenAPI operation createSiteFromBlueprint exposes a
forward-looking "remote" variant via the discriminator propertyName sourceType
and the CreateSiteFromRemoteRequest schema but doesn't document its
planned/unimplemented status; update the operationId createSiteFromBlueprint
documentation to mark sourceType=remote as "planned" (not yet implemented), add
the expected interim response (e.g., 501 Not Implemented) to responses, and
include a clear note in the requestBody description explaining clients should
treat remote as future work and that current behavior will return the interim
response when sourceType=remote.
- Around line 8927-8936: The CreateSiteFromBlueprintRequest schema is missing
the siteParams bag needed to represent parameterized blueprint creation; update
the CreateSiteFromBlueprintRequest (which currently allOf CreateSiteRequest) to
include the same siteParams property shape that available_blueprints exposes and
that CreateSiteFromMarketplace accepts (i.e., add a properties.siteParams entry
with the same type/structure and include it in required if CreateSiteRequest
requires it) so the v2 endpoint can accept descriptor parameters for blueprint
creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6eff2215-ef4f-489d-978a-8a2aa9fe015c
📒 Files selected for processing (1)
src/main/api/studio-api.yaml
| discriminator: | ||
| propertyName: sourceType | ||
| mapping: | ||
| blueprint: '#/components/schemas/CreateSiteFromBlueprintRequest' | ||
| remote: '#/components/schemas/CreateSiteFromRemoteRequest' |
There was a problem hiding this comment.
I believe for this to be correct, the discriminator property should be declared on each of the oneOf schemas (is not at the moment).
There was a problem hiding this comment.
sourceType is declared on CreateSiteRequest, which is the base for both CreateSiteFromBlueprintRequest and CreateSiteFromRemoteRequest.
Would that do?
|
Notice this PR depends on craftercms/commons#424 |
|
Please notice this PR will break the UI if merged before craftercms/craftercms#8595 is done |
3 participants
Create API v2
craftercms/craftercms#6061
Summary by CodeRabbit