Open
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:
WalkthroughRemoves legacy v1 dependency APIs, Groovy helpers, and v1 Spring wiring; adds/renames v2 endpoints to use Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as DependencyController
participant Service as DependencyService
participant DB as Mapper/DB
Client->>Controller: POST /api/2/dependency/{siteId}/publish_dependencies (paths)
Controller->>Service: getPublishingSoftDependencies(siteId, paths, target)
Service->>DB: query hard & soft dependencies
DB-->>Service: hardDependencies, softDependencies (paths/LightItem)
Service-->>Controller: hardDependencies, softDependencies
Controller->>Controller: remove hard from soft
Controller-->>Client: 200 OK { hardDependencies, softDependencies }
Client->>Controller: POST /api/2/dependency/{siteId}/dependencies (path)
Controller->>Service: getDependencies(siteId, path)
Service->>DB: query joined dependency items (LightItem)
DB-->>Service: LightItem list
Service-->>Controller: items
Controller-->>Client: 200 OK { items: [...] }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/api/studio-api.yaml (1)
5467-5491:⚠️ Potential issue | 🟠 MajorDeclare
siteIdas a path parameter here and stop requiring it in the body.Line 5467 introduces
{siteId}, but the operation never defines the correspondingin: pathparameter. That makes the contract invalid for OpenAPI validators/codegen, and keepingsiteIdrequired in the body leaves two sources of truth for the same value.🔧 Minimal contract fix
/api/2/dependency/{siteId}/publish_dependencies: post: + parameters: + - name: siteId + in: path + description: site ID + required: true + schema: + type: string requestBody: required: true content: application/json: schema: type: object properties: - siteId: - description: Site ID - type: string paths: description: Content paths to get dependencies for type: array items: type: string required: - - siteId - paths🤖 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 5467 - 5491, The OpenAPI operation /api/2/dependency/{siteId}/publish_dependencies (operationId: getPublishDependencies) is missing a path parameter declaration for siteId and also redundantly requires siteId in the requestBody; add a parameters entry for siteId with name: siteId, in: path, required: true, schema: { type: string }, and remove siteId from the requestBody schema properties and required list so the body only contains paths.
🧹 Nitpick comments (1)
src/main/java/org/craftercms/studio/controller/rest/v2/DependencyController.java (1)
57-60: Add a defensive copy before mutating the service-returned collection.Line 60 mutates the collection returned by
dependencyService.getSoftDependencies(...). While the current DAO implementation returns a mutable collection view, adding a defensive copy protects against future refactorings where the service might return immutable collections (e.g., via.toList()orCollections.unmodifiableCollection()).♻️ Suggested change
- Collection<LightItem> softDeps = dependencyService.getSoftDependencies(site, request.getPaths()); + Collection<LightItem> softDeps = new ArrayList<>(dependencyService.getSoftDependencies(site, request.getPaths()));import java.util.ArrayList;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/controller/rest/v2/DependencyController.java` around lines 57 - 60, The code currently calls dependencyService.getSoftDependencies(...) and then mutates the returned collection via softDeps.removeAll(hardDeps); make a defensive mutable copy of the softDeps before mutating it (e.g., construct a new ArrayList from the collection returned by dependencyService.getSoftDependencies(...)) so removeAll operates on your copy; update the variable/usage around softDeps in DependencyController to use that new mutable copy while leaving dependencyService.getSoftDependencies and getHardDependencies unchanged.
🤖 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 5574-5624: The OpenAPI entry for operationId getDependencies at
path /api/2/dependency/{siteId}/dependencies must document its interim behavior:
update the description to state that the handler is "Not yet implemented —
returns an empty placeholder/empty list" and/or add a '501 Not Implemented'
response entry (mirroring existing responses style) so clients know the endpoint
is forward-looking; touch the operationId getDependencies description and
responses block to include the descriptive note and optionally the new 501
response schema referencing existing error response components.
In
`@src/main/java/org/craftercms/studio/controller/rest/v2/DependencyController.java`:
- Around line 86-90: The current stub in DependencyController builds a
successful ResultOne with OK and an empty items list which falsely signals "no
dependencies"; instead return an explicit not-implemented error: replace the
stub that sets result.setResponse(OK) and an empty Collection<LightItem> with
the existing error/not-implemented path (for example set
result.setResponse(NOT_IMPLEMENTED) or throw the framework's
NotImplementedException) and include a short message explaining the endpoint is
not implemented; update any use of RESULT_KEY_ITEMS so it is not set for the
error response and ensure callers receive the non-200 status via
ResultOne/DependencyController.
---
Outside diff comments:
In `@src/main/api/studio-api.yaml`:
- Around line 5467-5491: The OpenAPI operation
/api/2/dependency/{siteId}/publish_dependencies (operationId:
getPublishDependencies) is missing a path parameter declaration for siteId and
also redundantly requires siteId in the requestBody; add a parameters entry for
siteId with name: siteId, in: path, required: true, schema: { type: string },
and remove siteId from the requestBody schema properties and required list so
the body only contains paths.
---
Nitpick comments:
In
`@src/main/java/org/craftercms/studio/controller/rest/v2/DependencyController.java`:
- Around line 57-60: The code currently calls
dependencyService.getSoftDependencies(...) and then mutates the returned
collection via softDeps.removeAll(hardDeps); make a defensive mutable copy of
the softDeps before mutating it (e.g., construct a new ArrayList from the
collection returned by dependencyService.getSoftDependencies(...)) so removeAll
operates on your copy; update the variable/usage around softDeps in
DependencyController to use that new mutable copy while leaving
dependencyService.getSoftDependencies and getHardDependencies unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c875540f-cb66-4e1f-aed6-b5c94af5bc4c
📒 Files selected for processing (10)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v2/service/dependency/DependencyService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/DependencyController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/model/rest/dependency/GetDependenciesRequestBody.javasrc/main/java/org/craftercms/studio/model/rest/dependency/GetPublishDependenciesRequestBody.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/webapp/default-site/scripts/api/DependencyServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/dependency/get-dependant.post.groovy
💤 Files with no reviewable changes (3)
- src/main/resources/crafter/studio/studio-services-context.xml
- src/main/webapp/default-site/scripts/rest/api/1/dependency/get-dependant.post.groovy
- src/main/webapp/default-site/scripts/api/DependencyServices.groovy
src/main/java/org/craftercms/studio/controller/rest/v2/DependencyController.java
Outdated
Show resolved
Hide resolved
jmendeza
force-pushed
the
feature/6061-deps
branch
from
4dc468a to
aa2a8b3
Compare
Member
Author
|
Removed v1 dependency endpoints:
I'm proposing to move |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/api/studio-api.yaml (1)
5467-5491:⚠️ Potential issue | 🟠 MajorDeclare
siteIdas a path parameter and stop requiring it in JSON.
{siteId}moved into the URL on Line 5467, but this operation still omits the path-parameter declaration and still requiressiteIdin the body. That leaves the OpenAPI contract inconsistent and forces clients to send the same value twice.🩹 Minimal spec fix
/api/2/dependency/{siteId}/publish_dependencies: post: tags: - dependency summary: Get list of publish dependencies for given content paths description: 'Required permission "content_read"' operationId: getPublishDependencies + parameters: + - name: siteId + in: path + description: site ID + required: true + schema: + type: string requestBody: required: true content: application/json: schema: type: object properties: - siteId: - description: Site ID - type: string paths: description: Content paths to get dependencies for type: array items: type: string required: - - siteId - paths#!/bin/bash set -euo pipefail echo "== OpenAPI block: /api/2/dependency/{siteId}/publish_dependencies ==" awk 'BEGIN{show=0} /\/api\/2\/dependency\/\{siteId\}\/publish_dependencies:/{show=1} show{print NR": "$0} /\/api\/2\/dependency\/\{siteId\}\/dependent_items:/{if (show) exit} ' src/main/api/studio-api.yaml echo echo "== Request model: GetPublishDependenciesRequestBody ==" rg -n -C3 'class\s+GetPublishDependenciesRequestBody|siteId|paths' \ src/main/java/org/craftercms/studio/model/rest/dependency/GetPublishDependenciesRequestBody.java🤖 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 5467 - 5491, The OpenAPI operation getPublishDependencies declares {siteId} in the URL but does not define it as a path parameter and still marks siteId as required in the JSON request body; update the YAML block for the operation to add a path parameter entry for "siteId" (in: path, required: true, schema: type: string, description: Site ID) and remove siteId from the requestBody schema (and its required list), and update the backing Java model GetPublishDependenciesRequestBody to remove the siteId property if present so the contract and model match the path-based siteId.
🤖 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 5467-5491: The OpenAPI operation getPublishDependencies declares
{siteId} in the URL but does not define it as a path parameter and still marks
siteId as required in the JSON request body; update the YAML block for the
operation to add a path parameter entry for "siteId" (in: path, required: true,
schema: type: string, description: Site ID) and remove siteId from the
requestBody schema (and its required list), and update the backing Java model
GetPublishDependenciesRequestBody to remove the siteId property if present so
the contract and model match the path-based siteId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3868677e-f6c1-4021-8a5b-bece9c3adf30
📒 Files selected for processing (14)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/service/dependency/DependencyService.javasrc/main/java/org/craftercms/studio/api/v2/service/dependency/DependencyService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/DependencyController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/impl/v1/service/dependency/DependencyServiceImpl.javasrc/main/java/org/craftercms/studio/model/rest/dependency/GetDependenciesRequestBody.javasrc/main/java/org/craftercms/studio/model/rest/dependency/GetPublishDependenciesRequestBody.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/webapp/default-site/scripts/api/DependencyServices.groovysrc/main/webapp/default-site/scripts/api/impl/dependency/SpringDependencyServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/dependency/get-dependant.post.groovysrc/main/webapp/default-site/scripts/rest/api/1/dependency/get-simple-dependencies.post.groovy
💤 Files with no reviewable changes (7)
- src/main/webapp/default-site/scripts/rest/api/1/dependency/get-dependant.post.groovy
- src/main/webapp/default-site/scripts/rest/api/1/dependency/get-simple-dependencies.post.groovy
- src/main/webapp/default-site/scripts/api/DependencyServices.groovy
- src/main/java/org/craftercms/studio/api/v1/service/dependency/DependencyService.java
- src/main/webapp/default-site/scripts/api/impl/dependency/SpringDependencyServices.groovy
- src/main/resources/crafter/studio/studio-services-context.xml
- src/main/java/org/craftercms/studio/impl/v1/service/dependency/DependencyServiceImpl.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/org/craftercms/studio/model/rest/dependency/GetDependenciesRequestBody.java
- src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/craftercms/studio/api/v2/service/dependency/DependencyService.java
- src/main/resources/crafter/studio/database-context.xml
- src/main/java/org/craftercms/studio/model/rest/dependency/GetPublishDependenciesRequestBody.java
4 tasks
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/java/org/craftercms/studio/controller/rest/v2/DependencyController.java`:
- Around line 53-57: In getPublishDependencies replace the transitive
soft-dependency call with the publish-time non-transitive lookup: call
dependencyService.getPublishingSoftDependencies(site, request.getPaths())
instead of dependencyService.getSoftDependencies(...), so getPublishDependencies
uses the non-recursive publish lookup for softDeps (leave hardDeps as-is), and
ensure any default target behavior is handled in DependencyService rather than
falling back to the recursive API.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/dependency/DependencyServiceImpl.java`:
- Around line 82-85: Add the same site and path guard annotations to the
2-argument overload of getHardDependencies in DependencyServiceImpl: annotate
the method getHardDependencies(`@SiteId` String site, Collection<String> paths)
with `@RequireSiteReady` and `@ProtectedResourceId`(PATH_LIST_RESOURCE_ID) (matching
the 3-arg overload) so the call chain from
DependencyController.getPublishDependencies() enforces the same security checks
before delegating to dependencyServiceInternal.getHardDependencies.
- Around line 153-157: The getDependencies and getDependencyPaths methods in
DependencyServiceImpl are missing the `@ProtectedResourceId`(PATH_RESOURCE_ID)
annotation on their path parameter, so update both method signatures to annotate
the String path parameter with `@ProtectedResourceId`(PATH_RESOURCE_ID) (matching
the pattern used in ContentServiceImpl/WorkflowServiceImpl) so `@HasPermission`
can evaluate path-based permissions; ensure any required import for
ProtectedResourceId is added and keep the parameter name path unchanged to match
permission evaluation.
In `@src/main/resources/org/craftercms/studio/api/v2/dal/DependencyDAO.xml`:
- Around line 246-252: The MyBatis <select id="getDependencyPaths"> lacks an
explicit result mapping for the Collection<String> return; update the <select>
definition (getDependencyPaths) to include an explicit
resultType="java.lang.String" (or resultType="string") or create and reference a
resultMap that maps the target_path column to the String result so MyBatis can
map the query result to Collection<String>.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 571b80be-b8cd-47b6-9d23-f63b0b1bacbb
📒 Files selected for processing (7)
src/main/java/org/craftercms/studio/api/v2/dal/DependencyDAO.javasrc/main/java/org/craftercms/studio/api/v2/service/dependency/DependencyService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/DependencyController.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/dependency/DependencyServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/dependency/internal/DependencyServiceInternalImpl.javasrc/main/resources/org/craftercms/studio/api/v2/dal/DependencyDAO.xml
src/main/java/org/craftercms/studio/controller/rest/v2/DependencyController.java
Show resolved
Hide resolved
src/main/java/org/craftercms/studio/impl/v2/service/dependency/DependencyServiceImpl.java
Show resolved
Hide resolved
src/main/java/org/craftercms/studio/impl/v2/service/dependency/DependencyServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/craftercms/studio/impl/v2/service/dependency/DependencyServiceImpl.java (1)
82-84:⚠️ Potential issue | 🟠 MajorAdd
@RequireSiteReadyto the 2-arggetHardDependenciesoverload.Line 83-84 still lacks the site-readiness guard used by the sibling overload (Line 75-79), so
/publish_dependenciessecurity/site preconditions are inconsistent on this path.Suggested fix
`@Override` + `@RequireSiteReady` `@HasPermission`(type = DefaultPermission.class, action = PERMISSION_CONTENT_READ) public Collection<LightItem> getHardDependencies(`@SiteId` String site, `@ProtectedResourceId`(PATH_LIST_RESOURCE_ID) Collection<String> paths) throws SiteNotFoundException { return dependencyServiceInternal.getHardDependencies(site, paths); }#!/bin/bash set -euo pipefail FILE="src/main/java/org/craftercms/studio/impl/v2/service/dependency/DependencyServiceImpl.java" CTRL="src/main/java/org/craftercms/studio/controller/rest/v2/DependencyController.java" echo "Inspect both getHardDependencies overloads and their annotations:" sed -n '70,92p' "$FILE" | cat -n echo echo "Find controller call sites to getHardDependencies (to confirm 2-arg path usage):" rg -nP --type=java -C3 '\bgetHardDependencies\s*\(' "$CTRL"Expected result: the 3-arg overload shows
@RequireSiteReady, while the 2-arg overload does not; controller includes call(s) into the 2-arg overload path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/impl/v2/service/dependency/DependencyServiceImpl.java` around lines 82 - 84, The two-arg overload of getHardDependencies in DependencyServiceImpl (method signature getHardDependencies(String site, Collection<String> paths)) is missing the `@RequireSiteReady` annotation used on the sibling overload; add `@RequireSiteReady` above this method to enforce the same site-readiness guard, and if necessary add the corresponding import for RequireSiteReady so the class compiles and both overloads have consistent preconditions.
🤖 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/java/org/craftercms/studio/impl/v2/service/dependency/DependencyServiceImpl.java`:
- Around line 82-84: The two-arg overload of getHardDependencies in
DependencyServiceImpl (method signature getHardDependencies(String site,
Collection<String> paths)) is missing the `@RequireSiteReady` annotation used on
the sibling overload; add `@RequireSiteReady` above this method to enforce the
same site-readiness guard, and if necessary add the corresponding import for
RequireSiteReady so the class compiles and both overloads have consistent
preconditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d97fba03-2fcc-4348-a1f2-126f1e93333a
📒 Files selected for processing (1)
src/main/java/org/craftercms/studio/impl/v2/service/dependency/DependencyServiceImpl.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace v1 APIs - Dependencies
craftercms/craftercms#6061
Summary by CodeRabbit
Breaking Changes
New Features
API Improvements