Item nav order APIs update#3916
Draft
jmendeza wants to merge 2 commits into
Draft
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 navigation-order persistence, services, Spring beans, Groovy wrappers and v1 REST scripts; adds v2 OpenAPI endpoints and a polymorphic ReorderItemRequest model; introduces controller stubs and updates internal wiring and tests accordingly. ChangesContent Ordering API Migration (v1 → v2)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 7
🧹 Nitpick comments (1)
src/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.java (1)
31-36: Consider allowing optional boundary anchors to support first/last position reorders.The current requirement for both
pathBeforeandpathAfterto be non-empty prevents reordering items to first or last position, where one anchor is naturally absent. When implementing the reorder service, consider making one anchor optional and validating that at least one is provided.Optional DTO adjustment for future implementation
import jakarta.validation.constraints.NotEmpty; +import jakarta.validation.constraints.AssertTrue; import org.craftercms.commons.validation.annotations.param.ValidExistingContentPath; +import org.springframework.util.StringUtils; @@ `@NotEmpty` `@ValidExistingContentPath` private String parentPath; - `@NotEmpty` + `@ValidExistingContentPath` private String pathBefore; - `@NotEmpty` + `@ValidExistingContentPath` private String pathAfter; + + `@AssertTrue`(message = "Either pathBefore or pathAfter must be provided") + public boolean hasAnchor() { + return StringUtils.hasText(pathBefore) || StringUtils.hasText(pathAfter); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.java` around lines 31 - 36, Rework the ReorderItemRequest DTO so reordering can target first/last: remove the `@NotEmpty` requirement from at least one of the two fields (pathBefore and pathAfter) and add a class-level validation that enforces "at least one anchor present" (e.g., implement a custom validator annotation like `@AtLeastOneAnchor` or add an isValid() method used by a class-level `@AssertTrue`) that checks (pathBefore != null && !pathBefore.isEmpty()) || (pathAfter != null && !pathAfter.isEmpty()); keep or adapt `@ValidExistingContentPath` checks to only run when the corresponding field is non-empty. Ensure validators reference the ReorderItemRequest class and the pathBefore/pathAfter fields so the service handling reorders can accept null/empty for one anchor but not both.
🤖 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 5482-5483: The schema property nextOrder is declared as type
"integer" which will reject valid fractional ordering values; update the OpenAPI
schema entries declaring nextOrder (and any identical order-like fields at the
other occurrence) to use type "number" instead of "integer" so calculated or
between-item order values are accepted; locate the nextOrder definitions in the
YAML (the shown block and the similar block at the other occurrence) and change
their type to "number" while preserving any existing descriptions or
constraints.
- Around line 5475-5491: The response objects for these new TODO endpoints are
missing an interim 501 Not Implemented entry; update the response blocks that
currently list '200', '400', '401', '404', and '500' (including the shown block
and the two other new endpoint response blocks introduced alongside it) by
adding a '501' response entry that references the shared NotImplemented response
(e.g., "501": $ref: '#/components/responses/NotImplemented') so clients see the
interim behavior.
- Line 5460: The OpenAPI spec has a duplicate operationId "getNextItemOrder"
(declared twice), which breaks codegen; rename one of the operations to a unique
identifier (e.g., "getNextItemOrderV2" or a more specific name like
"getNextItemOrderForCollection") and update any references that rely on that
operationId (generated client method names, $ref usages, or tooling configs) so
they point to the new name; ensure the operationId change is applied in the
operation object where "operationId: getNextItemOrder" is declared and verify
downstream codegen and tests compile against the new identifier.
In
`@src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java`:
- Around line 376-377: The endpoint mappings in the controller are out of sync
with the OpenAPI spec; update the mapping constants and `@GetMapping/`@PostMapping
annotations so the routes use the spec's paths (e.g., "/item/next-order",
"/item/orders", "/item/reorder") instead of the current ".../order/..."
variants: rename or adjust GET_ITEM_NEXT_ORDER and its handler getNextItemOrder,
the corresponding constant and handler for the item orders endpoint (referenced
around the getItemOrders method), and the reorder endpoint constant/handler
(around the reorderItems/reorder method) so each mapping and constant exactly
match the OpenAPI contract.
- Around line 376-380: The three REST handlers currently returning null (e.g.,
getNextItemOrder) must call the appropriate content service methods to compute
the values and return them wrapped in a Result payload instead of null; locate
getNextItemOrder (and the two adjacent unimplemented methods referenced) and
replace the TODO/null with calls to your domain service (e.g.,
contentService.getNextItemOrder(siteId, parentPath) or the equivalent method
names used in this class), handle/translate any checked exceptions into proper
error Results, and return Result.success(theComputedValue) (or the project’s
standard Result factory) so the endpoints produce valid responses.
In
`@src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java`:
- Around line 70-72: The path constants GET_ITEM_NEXT_ORDER, GET_ITEMS_ORDER,
and REORDER_ITEM currently use "/order" segments but must match the OpenAPI v2
spec; update the constant values in RequestMappingConstants to SITE_ID +
"/item/next-order", SITE_ID + "/item/orders", and SITE_ID + "/item/reorder"
respectively so the controller-registered routes (GET_ITEM_NEXT_ORDER,
GET_ITEMS_ORDER, REORDER_ITEM) align with the published API paths.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java`:
- Around line 529-531: The method updateNavOrder(String siteId, String path,
Document document) currently returns false unconditionally which silently allows
stale nav-order data; change it to fail fast by throwing an
UnsupportedOperationException (or IllegalStateException) with a clear message
like "updateNavOrder not implemented — legacy nav-order logic removed" inside
ContentServiceInternalImpl.updateNavOrder so callers (page writes/copies and
updateNavOrderForMove) cannot proceed silently; ensure the exception text names
updateNavOrder and suggests the replacement work is required.
---
Nitpick comments:
In
`@src/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.java`:
- Around line 31-36: Rework the ReorderItemRequest DTO so reordering can target
first/last: remove the `@NotEmpty` requirement from at least one of the two fields
(pathBefore and pathAfter) and add a class-level validation that enforces "at
least one anchor present" (e.g., implement a custom validator annotation like
`@AtLeastOneAnchor` or add an isValid() method used by a class-level `@AssertTrue`)
that checks (pathBefore != null && !pathBefore.isEmpty()) || (pathAfter != null
&& !pathAfter.isEmpty()); keep or adapt `@ValidExistingContentPath` checks to only
run when the corresponding field is non-empty. Ensure validators reference the
ReorderItemRequest class and the pathBefore/pathAfter fields so the service
handling reorders can accept null/empty for one anchor but not both.
🪄 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: ac1fffea-e171-4114-974f-f75a65826175
📒 Files selected for processing (23)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.javasrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.javasrc/main/java/org/craftercms/studio/api/v1/service/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xmlsrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovysrc/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
💤 Files with no reviewable changes (17)
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.java
- src/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovy
- src/main/resources/crafter/studio/database-context.xml
- src/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovy
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.java
- src/main/resources/crafter/studio/studio-services-context.xml
- src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
- src/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovy
- src/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xml
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.java
Comment on lines
+376
to
+380
| @GetMapping(GET_ITEM_NEXT_ORDER) | ||
| public Result getNextItemOrder(@ValidSiteId @PathVariable String siteId, @NotEmpty @ValidExistingContentPath @RequestParam String parentPath) { | ||
| // TODO | ||
| return null; | ||
| } |
There was a problem hiding this comment.
These new API handlers are unimplemented and currently return null.
All three endpoints are exposed but non-functional, which will produce invalid responses for consumers and block the feature objective. Please implement service calls and return proper Result payloads before merge.
Also applies to: 387-391, 394-398
🤖 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/ContentController.java`
around lines 376 - 380, The three REST handlers currently returning null (e.g.,
getNextItemOrder) must call the appropriate content service methods to compute
the values and return them wrapped in a Result payload instead of null; locate
getNextItemOrder (and the two adjacent unimplemented methods referenced) and
replace the TODO/null with calls to your domain service (e.g.,
contentService.getNextItemOrder(siteId, parentPath) or the equivalent method
names used in this class), handle/translate any checked exceptions into proper
error Results, and return Result.success(theComputedValue) (or the project’s
standard Result factory) so the endpoints produce valid responses.
Comment on lines
+529
to
+531
| private boolean updateNavOrder(String siteId, String path, Document document) { | ||
| // TODO: Check the nav order exists in the document, if not, add it with the next item order value | ||
| return false; |
There was a problem hiding this comment.
Don't ship updateNavOrder(...) as a no-op.
This method is now the only nav-order hook used by page writes/copies and by updateNavOrderForMove(...). Returning false unconditionally means page descriptors stop getting nav-order updates once the legacy service is removed, so copy/move/write flows can silently persist stale ordering data. At minimum, this should fail fast until the replacement logic is implemented instead of succeeding with broken ordering.
🤖 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/content/internal/ContentServiceInternalImpl.java`
around lines 529 - 531, The method updateNavOrder(String siteId, String path,
Document document) currently returns false unconditionally which silently allows
stale nav-order data; change it to fail fast by throwing an
UnsupportedOperationException (or IllegalStateException) with a clear message
like "updateNavOrder not implemented — legacy nav-order logic removed" inside
ContentServiceInternalImpl.updateNavOrder so callers (page writes/copies and
updateNavOrderForMove) cannot proceed silently; ensure the exception text names
updateNavOrder and suggests the replacement work is required.
jmendeza
force-pushed
the
feature/6061-item-orders
branch
from
fa7060e to
b9e01ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/api/studio-api.yaml`:
- Around line 5345-5353: The 200 responses return bare payload fields (e.g.,
nextOrder, order and array item schemas) instead of the v2 ApiResponse envelope;
update each affected response schema (the ones returning nextOrder and the
array/item responses referenced around the diff) to wrap the existing schema
under a top-level object property named response (i.e., replace the current
schema with an object that has properties.response equal to the original schema
and preserves type/description), so clients receive { response:
<originalPayload> } consistent with ApiResponse; ensure the referenced fields
nextOrder and any array item schemas are moved unchanged into response.
In
`@src/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.java`:
- Around line 31-36: ReorderItemRequest currently requires both pathBefore and
pathAfter which prevents boundary moves; decide if boundary reorders are
allowed, then either (A) if boundaries supported: remove the `@NotEmpty`
constraint from pathBefore/pathAfter (make them nullable) and add a class-level
validator (e.g., an `@Constraint` on ReorderItemRequest or an `@AssertTrue` boolean
method) that enforces the business rule (at least one present or exactly one
present depending on the requirement) and update the reorderItem endpoint
contract to reflect the rule, or (B) if boundaries are not supported: keep
`@NotEmpty` but add documentation to the reorderItem API contract stating both
neighbors are required. Ensure validators reference
ReorderItemRequest.pathBefore and .pathAfter and the endpoint reorderItem in
tests/contract docs.
🪄 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: 4b14727f-70cd-4317-9296-34bb814ef90f
📒 Files selected for processing (23)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.javasrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.javasrc/main/java/org/craftercms/studio/api/v1/service/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xmlsrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovysrc/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
💤 Files with no reviewable changes (17)
- src/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovy
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.java
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovy
- src/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.java
- src/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xml
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java
- src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
- src/main/resources/crafter/studio/database-context.xml
- src/main/resources/crafter/studio/studio-services-context.xml
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java
- src/main/webapp/default-site/scripts/api/ServiceFactory.groovy
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
jmendeza
force-pushed
the
feature/6061-item-orders
branch
4 times, most recently
from
a55aa73 to
a751208
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java (1)
379-387:⚠️ Potential issue | 🔴 CriticalEndpoints are still exposed as stubs returning
null.Both
getItemsOrder(...)andreorderItem(...)are mapped but unimplemented, so these APIs currently return invalid responses instead of aResultpayload backed by service logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java` around lines 379 - 387, Implement the two stub endpoints to delegate to the content service and return a proper Result: in getItemsOrder(String siteId, String parentPath) call the service method that retrieves item order (e.g., contentService.getItemsOrder(siteId, parentPath) or equivalent), wrap the returned data into a successful Result payload and return it; in reorderItem(String siteId, ReorderItemRequest request) call the service method that applies the reorder (e.g., contentService.reorderItem(siteId, request)), handle validation/exceptions as before in this controller, and return a Result indicating success (or the updated order) instead of null; keep existing annotations (`@ValidSiteId`, `@ValidExistingContentPath`, `@Valid`) and reuse the controller’s standard Result creation pattern/methods for consistent response shape.src/main/api/studio-api.yaml (2)
5414-5415:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn
orderasnumber(double), notinteger.At Line 5415,
orderis declared asinteger, which can reject valid fractional ordering values produced by reorder calculations.Suggested fix
order: - type: integer + type: number + format: double🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/api/studio-api.yaml` around lines 5414 - 5415, The OpenAPI schema property named "order" is currently declared as type: integer which will reject fractional reorder values; change the "order" property in the studio-api YAML schema from type: integer to type: number and (optionally) add format: double so fractional ordering values are accepted (update the property definition for "order" accordingly).
5349-5356:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a top-level v2 response envelope instead of per-item
response.At Line 5350 the 200 schema is an array, and at Line 5354 each item contains
response. This makes the API contract inconsistent with the v2 envelope pattern used elsewhere ({ response, ... }) and complicates generated clients.Suggested minimal shape fix
'200': description: OK content: application/json: schema: - type: array - items: - type: object - properties: - response: - $ref: '#/components/schemas/ApiResponse' - path: - type: string - order: - type: number - format: double - label: - type: string + type: object + properties: + response: + $ref: '#/components/schemas/ApiResponse' + items: + type: array + items: + type: object + properties: + path: + type: string + order: + type: number + format: double + label: + type: string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/api/studio-api.yaml` around lines 5349 - 5356, The 200 response currently defines an array whose items each contain a per-item "response" field (referenced as ApiResponse), which violates the v2 top-level envelope; change the schema so the operation returns a single object with a top-level "response" property whose value is the array of item objects. Concretely, replace the existing schema: type: array / items: { type: object / properties: { response: $ref: '#/components/schemas/ApiResponse', ... } } with schema: type: object / properties: { response: { type: array, items: { type: object, properties: { /* move item properties here except per-item response */ } } } } so ApiResponse is referenced at the top-level "response" instead of per-item.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/main/api/studio-api.yaml`:
- Around line 5414-5415: The OpenAPI schema property named "order" is currently
declared as type: integer which will reject fractional reorder values; change
the "order" property in the studio-api YAML schema from type: integer to type:
number and (optionally) add format: double so fractional ordering values are
accepted (update the property definition for "order" accordingly).
- Around line 5349-5356: The 200 response currently defines an array whose items
each contain a per-item "response" field (referenced as ApiResponse), which
violates the v2 top-level envelope; change the schema so the operation returns a
single object with a top-level "response" property whose value is the array of
item objects. Concretely, replace the existing schema: type: array / items: {
type: object / properties: { response: $ref: '#/components/schemas/ApiResponse',
... } } with schema: type: object / properties: { response: { type: array,
items: { type: object, properties: { /* move item properties here except
per-item response */ } } } } so ApiResponse is referenced at the top-level
"response" instead of per-item.
In
`@src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java`:
- Around line 379-387: Implement the two stub endpoints to delegate to the
content service and return a proper Result: in getItemsOrder(String siteId,
String parentPath) call the service method that retrieves item order (e.g.,
contentService.getItemsOrder(siteId, parentPath) or equivalent), wrap the
returned data into a successful Result payload and return it; in
reorderItem(String siteId, ReorderItemRequest request) call the service method
that applies the reorder (e.g., contentService.reorderItem(siteId, request)),
handle validation/exceptions as before in this controller, and return a Result
indicating success (or the updated order) instead of null; keep existing
annotations (`@ValidSiteId`, `@ValidExistingContentPath`, `@Valid`) and reuse the
controller’s standard Result creation pattern/methods for consistent response
shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c55bed0-f18b-4dca-8965-7c0e4157c51f
📒 Files selected for processing (23)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.javasrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.javasrc/main/java/org/craftercms/studio/api/v1/service/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xmlsrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovysrc/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
💤 Files with no reviewable changes (17)
- src/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.java
- src/main/resources/crafter/studio/database-context.xml
- src/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovy
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java
- src/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovy
- src/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.java
- src/main/resources/crafter/studio/studio-services-context.xml
- src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
- src/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.java
- src/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovy
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xml
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.java
- src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/default-site/scripts/api/ServiceFactory.groovy
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
jmendeza
force-pushed
the
feature/6061-item-orders
branch
from
a751208 to
e577469
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/api/studio-api.yaml`:
- Around line 10750-10792: The base schema ReorderItemRequestBase currently
permits all three type values which allows invalid combinations; restrict the
subtype schemas by adding a concrete type enum in each allOf: update
ReorderItemReferencePathRequest to include a properties.type enum limited to the
reference-path variants (e.g., ["addBefore","addAfter"]) and update
ReorderItemInsertBetweenRequest to include properties.type enum
["insertBetween"] (or equivalent values you use for insertBetween), so each
derived schema narrows the allowed type and enforces correct discriminator
behavior.
In
`@src/main/java/org/craftercms/studio/model/rest/content/order/ReorderItemRequest.java`:
- Around line 34-35: Replace the `@NotEmpty` annotation with `@NotBlank` on all path
fields in the ReorderItemRequest class so whitespace-only strings are rejected
up-front; specifically update the annotations on parentPath, referencePath,
previousPath, nextPath (and the fifth path field annotated similarly) to use
javax.validation.constraints.NotBlank instead of NotEmpty so constraint
validation returns a 400 for blank paths before reaching
`@ValidExistingContentPath`.
🪄 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: 50430f18-2688-4255-8792-2fde9650fdcf
📒 Files selected for processing (23)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.javasrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.javasrc/main/java/org/craftercms/studio/api/v1/service/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/model/rest/content/order/ReorderItemRequest.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xmlsrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovysrc/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
💤 Files with no reviewable changes (17)
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.java
- src/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.java
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovy
- src/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovy
- src/main/resources/crafter/studio/database-context.xml
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.java
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java
- src/main/resources/crafter/studio/studio-services-context.xml
- src/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xml
- src/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovy
- src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java
- src/main/webapp/default-site/scripts/api/ServiceFactory.groovy
jmendeza
force-pushed
the
feature/6061-item-orders
branch
2 times, most recently
from
cc20aad to
816c727
Compare
jmendeza
force-pushed
the
feature/6061-item-orders
branch
from
816c727 to
f2a4381
Compare
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.
craftercms/craftercms#6061
Item nav order APIs update
Summary by CodeRabbit
New Features
Deprecations / Breaking Changes