Drop states for folder items #3810
Conversation
jmendeza
force-pushed
the
feature/7724
branch
from
db8f5b9 to
18bdc2f
Compare
WalkthroughChanges adjust folder semantics and permissions, remove an old API overload, modify available-actions resolution logic, update SQL/DAO to treat folders with state 0, refine file creation to check existence first, alter copy/move folder discovery, and add an upgrade pipeline and SQL script to reset folder states. One file updates header years. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Resolver as SemanticsAvailableActionsResolverImpl
participant Auth as PermissionsService
participant Workflow as WorkflowService
User->>Resolver: calculateContentItemAvailableActions(username, siteId, detailedItem)
Resolver->>Auth: getUserPermissionsBitmap(username, siteId, detailedItem)
Auth-->>Resolver: userPermissionsBitmap
Note over Resolver: Mask with systemTypeBitmap
alt detailedItem.systemType != "folder"
Resolver->>Workflow: getWorkflowStateBitmap(detailedItem)
Workflow-->>Resolver: workflowStateBitmap
Note over Resolver: AND with workflowStateBitmap
else folder
Note over Resolver: Skip workflow-state masking
end
Note over Resolver: applySpecialUseCaseFilters(...)
Resolver-->>User: availableActionsBitmap
sequenceDiagram
autonumber
participant Repo as GitContentRepositoryImpl
participant FS as FileSystem
Repo->>FS: exists(path)
alt file exists
Note over Repo: Skip create, no error
else not exists
Repo->>FS: createNewFile(path)
alt creation fails
Repo-->>Repo: Handle error/throw
else created
Note over Repo: Proceed
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-08-20T19:08:06.459ZApplied to files:
📚 Learning: 2025-09-25T20:27:54.135ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
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: 0
🧹 Nitpick comments (8)
src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java (1)
350-351: Avoid magic number; prefer explicit long literal or named constantUse 0L (or introduce a NO_STATE constant) for clarity in
.withState(0).- .withState(0) + .withState(0L)src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.5-to-5.0.0.6.sql (1)
17-19: LGTM; consider reducing unnecessary writesThe script is correct. Optionally add
AND state <> 0to avoid rewriting already‑clean rows on large tables.-UPDATE item SET state = 0 WHERE system_type = 'folder' ; +UPDATE item SET state = 0 WHERE system_type = 'folder' AND state <> 0 ;src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml (1)
348-371: Exclude folders from generic state mutations for DB hygieneTo avoid reintroducing non‑zero states for folders via bulk updates/unlock, exclude
system_type = 'folder'in these statements. Behaviorally no change (Item.getState returns 0), but it keeps data clean.--- a/src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml +++ b/src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml @@ <update id="setStatesBySiteAndPathBulk"> - UPDATE item SET state = state | #{statesBitMap} + UPDATE item SET state = state | #{statesBitMap} WHERE site_id = #{siteId} AND path IN @@ -</update> +</update> @@ <update id="resetStatesBySiteAndPathBulk"> - UPDATE item SET state = state & ~#{statesBitMap} + UPDATE item SET state = state & ~#{statesBitMap} WHERE site_id = #{siteId} AND path IN @@ -</update> +</update> @@ <update id="updateStatesBySiteAndPathBulk"> - UPDATE item set state = (state | #{onStatesBitMap}) & ~#{offStatesBitMap} - WHERE site_id = #{siteId} AND path IN + UPDATE item set state = (state | #{onStatesBitMap}) & ~#{offStatesBitMap} + WHERE site_id = #{siteId} + AND system_type <> '${systemTypeFolder}' + AND path IN @@ </update> @@ <update id="updateStatesByQuery"> - UPDATE item i INNER JOIN site s ON i.site_id = s.id + UPDATE item i INNER JOIN site s ON i.site_id = s.id SET i.state = (i.state | #{onStatesBitMap}) & ~#{offStatesBitMap} WHERE s.site_id = #{siteId} AND s.deleted = 0 AND i.ignored = 0 + AND i.system_type <> '${systemTypeFolder}' @@ </update> @@ <update id="unlockItemByPath"> - UPDATE item i INNER JOIN site s + UPDATE item i INNER JOIN site s ON i.site_id = s.id SET i.locked_by = #{lockOwnerId}, i.state = i.state & #{lockedBitOff} WHERE s.site_id = #{siteId} AND s.deleted = 0 AND i.path = #{path} + AND i.system_type <> '${systemTypeFolder}' </update>Also applies to: 515-527, 443-461
src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml (1)
170-178: Also exclude folders in other publish state updatersOptional: mirror the folder exclusion in
updateItemStateBitsandrecalculateItemStateBitsto avoid writing state bits for folders.--- a/src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml +++ b/src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml @@ <update id="updateItemStateBits"> @@ - WHERE item.id = updates.id + WHERE item.id = updates.id + AND item.system_type <> '${systemTypeFolder}' </update> @@ <update id="recalculateItemStateBits"> - UPDATE item i + UPDATE item i INNER JOIN item_publish_item ipi ON i.id = ipi.item_id INNER JOIN publish_item pi ON ipi.publish_item_id = pi.id SET i.state = (i.state | #{onStatesBitMap}) WHERE pi.package_id = #{packageId} + AND i.system_type <> '${systemTypeFolder}' </update>Also applies to: 253-276
src/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.java (1)
274-286: Don’t compute workflow state for folders; set system type and state explicitlySetting saved/closed bits is irrelevant for folders. Set
systemTypefirst and persiststate=0Lto make intent obvious and avoid transient non‑zero states.- Item item = instantiateItem(siteId, folderPath) - .withLastModifiedBy(userObj.getId()) - .withLastModifiedOn(DateUtils.getCurrentTime()) - .withLabel(folderName) - .withParentId(parentId) - .build(); - item.setState(ItemState.savedAndClosed(item.getState())); - item.setSystemType(CONTENT_TYPE_FOLDER); + Item item = instantiateItem(siteId, folderPath) + .withLastModifiedBy(userObj.getId()) + .withLastModifiedOn(DateUtils.getCurrentTime()) + .withLabel(folderName) + .withParentId(parentId) + .withSystemType(CONTENT_TYPE_FOLDER) + .withState(0L) + .build(); upsertEntry(item);src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java (2)
116-122: Avoid re‑adding ITEM_UNLOCK for folders via admin special caseIf a folder ever carries a stray USER_LOCKED bit, admins could see “Unlock” despite FOLDER excluding it. Guard this block by system type.
- if ((itemState & USER_LOCKED.value) > 0 && (result & ITEM_UNLOCK) == 0) { + if (!CONTENT_TYPE_FOLDER.equals(itemSystemType) + && (itemState & USER_LOCKED.value) > 0 && (result & ITEM_UNLOCK) == 0) {
193-195: Use CONTENT_TYPE_FOLDER constant instead of string literalKeeps folder checks consistent with the rest of the class and avoids drift.
- if ("folder".equals(itemSystemType)) { + if (CONTENT_TYPE_FOLDER.equals(itemSystemType)) {src/main/java/org/craftercms/studio/api/v2/security/SemanticsAvailableActionsResolver.java (1)
35-35: Fix Javadoc param typeUpdate parameter doc to match the new signature.
- * @param detailedItem Item + * @param detailedItem ContentItem
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/org/craftercms/studio/api/v2/dal/Item.java(3 hunks)src/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.java(1 hunks)src/main/java/org/craftercms/studio/api/v2/security/SemanticsAvailableActionsResolver.java(2 hunks)src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.java(1 hunks)src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.5-to-5.0.0.6.sql(1 hunks)src/main/resources/crafter/studio/upgrade/pipelines.yaml(1 hunks)src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml(2 hunks)src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-20T19:07:36.327Z
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/4.x-to-5.0.new-schema.sql:0-0
Timestamp: 2025-08-20T19:07:36.327Z
Learning: In CrafterCMS 4.x-to-5.x upgrade scripts, the previous_path and last_published_on columns exist in the item table during the initial migration phases and are only dropped later in the cleanup script (5.0-t3-to-5.0-t4.cleanup.sql). The upgrade sequence is: 5.0-t → 5.0-t2 (new schema) → 5.0-t3 (publisher upgrade) → 5.0-t4 (populate item target) → 5.0-t5 (cleanup).
Applied to files:
src/main/resources/crafter/studio/upgrade/pipelines.yamlsrc/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.5-to-5.0.0.6.sql
📚 Learning: 2025-08-20T19:08:06.459Z
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/5.0-t2-to-5.0-t3.publisher-upgrade.sql:0-0
Timestamp: 2025-08-20T19:08:06.459Z
Learning: In the 4.x-to-5.x publisher upgrade migration script (5.0-t2-to-5.0-t3.publisher-upgrade.sql), the item_publish_item table insert intentionally filters to only link PENDING items (publish_state = 1) and excludes COMPLETED items from the item relationship. This is by design in the migration process.
Applied to files:
src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml
📚 Learning: 2025-08-20T19:09:04.909Z
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/5.0-t2-to-5.0-t3.publisher-upgrade.sql:28-31
Timestamp: 2025-08-20T19:09:04.909Z
Learning: In CrafterCMS 5.0 publisher upgrade scripts, the publish_package and publish_item tables intentionally use different bitmaps for their state values. This is by design - they are not meant to share a common state bitmap layout.
Applied to files:
src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml
📚 Learning: 2025-07-31T16:13:39.952Z
Learnt from: jmendeza
PR: craftercms/studio#3768
File: src/main/api/studio-api.yaml:10561-10569
Timestamp: 2025-07-31T16:13:39.952Z
Learning: In CrafterCMS Studio, move operations are represented as `RENAMED` in the `WriteContentResultItem.action` enum for historical reasons, even if the move does not involve a rename.
Applied to files:
src/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.javasrc/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml
🧬 Code graph analysis (1)
src/main/java/org/craftercms/studio/api/v2/dal/Item.java (1)
src/main/java/org/craftercms/studio/api/v1/constant/StudioConstants.java (1)
StudioConstants(28-196)
🔇 Additional comments (9)
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml (2)
372-386: Force state=0 for folders on move — correctSpecial‑casing folders to keep state at 0 while moving is consistent with the PR goal and prevents accidental state bit pollution.
388-402: Copy keeps folder state at 0 — correctThe CASE for
stateand NULLpreview_urlon folders looks good.src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml (1)
133-158: Exclude folders from initial publish state updates — correctThis aligns publishing state transitions with the new folder semantics.
src/main/java/org/craftercms/studio/api/v2/dal/Item.java (1)
138-140: Getter override for folder state — correctReturning 0 for folders via
getState()neatly centralizes the new semantics and keeps MyBatis writes consistent.src/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.java (1)
85-87: Removed ITEM_UNLOCK from folder actions — correctMatches the decision to drop states/locking semantics on folders.
src/main/resources/crafter/studio/upgrade/pipelines.yaml (1)
803-807: Approve: SQL upgrade script present and pipeline step placement correctReferenced script exists at src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.5-to-5.0.0.6.sql; step correctly follows 5.0.0.4→5.0.0.5.
src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java (1)
71-78: Workflow-state gating skipped for folders — LGTMResult initialization and conditional workflow-state ANDing align with “folders have no state.”
src/main/java/org/craftercms/studio/api/v2/security/SemanticsAvailableActionsResolver.java (2)
2-2: Header year update — LGTM
30-39: API change (Item → ContentItem): internal callers updated; document breaking changeInternal implementation and call sites in this repo were updated to ContentItem (SemanticsAvailableActionsResolverImpl; calls in ContentServiceInternalImpl). This remains a public source/binary‑breaking API change — add release notes/migration docs and verify external consumers/implementors are updated.
jmendeza
force-pushed
the
feature/7724
branch
from
9cce28f to
1b783df
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.5-to-5.0.0.6.sql (1)
17-20: Mismatch between script name/content and pipeline usage. Version update will regress db_version.This script sets _meta.version to 5.0.0.6, but pipelines.yaml runs it for currentVersion 5.0.0.6 → nextVersion 5.0.0.7. Executing this step would downgrade version metadata and is incorrect sequencing.
Fix by either:
- Referencing this script from the 5.0.0.5 → 5.0.0.6 step (and remove it from 5.0.0.6 → 5.0.0.7), or
- Create a new 5.0.0.6-to-5.0.0.7.sql that sets the version to '5.0.0.7' and update pipelines.yaml accordingly.
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml (4)
348-354: Exclude folders from bulk state mutations to avoid reintroducing state bits.These UPDATEs can set or clear bits on folders, violating the invariant. Guard them by system_type.
Apply this diff:
<update id="setStatesBySiteAndPathBulk"> - UPDATE item SET state = state | #{statesBitMap} - WHERE site_id = #{siteId} AND path IN + UPDATE item SET state = state | #{statesBitMap} + WHERE site_id = #{siteId} + AND system_type <> '${systemTypeFolder}' + AND path IN <foreach collection="paths" item="path" index="index" open="(" separator="," close=")"> #{path} </foreach> </update> <update id="resetStatesBySiteAndPathBulk"> - UPDATE item SET state = state & ~#{statesBitMap} - WHERE site_id = #{siteId} AND path IN + UPDATE item SET state = state & ~#{statesBitMap} + WHERE site_id = #{siteId} + AND system_type <> '${systemTypeFolder}' + AND path IN <foreach collection="paths" item="path" index="index" open="(" separator="," close=")"> #{path} </foreach> </update> <update id="updateStatesBySiteAndPathBulk"> - UPDATE item set state = (state | #{onStatesBitMap}) & ~#{offStatesBitMap} - WHERE site_id = #{siteId} AND path IN + UPDATE item set state = (state | #{onStatesBitMap}) & ~#{offStatesBitMap} + WHERE site_id = #{siteId} + AND system_type <> '${systemTypeFolder}' + AND path IN <foreach collection="paths" item="path" index="index" open="(" separator="," close=")"> #{path} </foreach> </update>Also applies to: 356-362, 364-370
515-527: Guard query-based state updates to skip folders.updateStatesByQuery may set bits on folders. Add a filter.
Apply this diff:
<update id="updateStatesByQuery"> UPDATE item i INNER JOIN site s ON i.site_id = s.id SET i.state = (i.state | #{onStatesBitMap}) & ~#{offStatesBitMap} WHERE s.site_id = #{siteId} AND s.deleted = 0 AND i.ignored = 0 + AND i.system_type <> '${systemTypeFolder}' <if test="path != null"> AND i.path RLIKE(#{path}) </if> <if test="statesBitMap != null"> AND (i.state & #{statesBitMap}) > 0 </if> </update>
664-673: Avoid mutating folder states in sync tasks.Both moveItemForSyncTask and updateItemForSyncTask alter state unconditionally. Keep folders at 0.
Apply this diff:
<update id="moveItemForSyncTask"> UPDATE item SET path = REPLACE(path, #{previousPath}, #{newPath}), locked_by = NULL, - state = (state | #{onStatesBitMap}) & ~#{offStatesBitMap} + state = CASE + WHEN system_type = '${systemTypeFolder}' THEN 0 + ELSE (state | #{onStatesBitMap}) & ~#{offStatesBitMap} + END WHERE site_id = #{siteId} AND (path = #{previousPath} OR path LIKE CONCAT(#{previousPath}, '/%')); </update> <update id="updateItemForSyncTask"> UPDATE item SET preview_url = #{previewUrl}, - state = (state | #{onStatesBitMap}) & ~#{offStatesBitMap}, + state = CASE + WHEN system_type = '${systemTypeFolder}' THEN 0 + ELSE (state | #{onStatesBitMap}) & ~#{offStatesBitMap} + END, last_modified_by = #{lastModifiedBy}, last_modified_on = #{lastModifiedOn}, label = #{label}, content_type_id = #{contentTypeId}, system_type = #{systemType}, mime_type = #{mimeType}, size = #{size}, locked_by = NULL, ignored = #{ignored} WHERE site_id = #{siteId} AND path = #{path}; </update>Also applies to: 675-692
557-565: Normalize getItemStates to return 0 for folders.Callers of getItemStates expect folder states to be dropped; returning raw DB bits for folders can cause inconsistencies.
Apply this diff:
<select id="getItemStates" resultType="org.craftercms.studio.api.v2.dal.ItemPathAndState"> - SELECT i.path, i.state + SELECT i.path, + CASE WHEN i.system_type = '${systemTypeFolder}' THEN 0 ELSE i.state END AS state FROM item i INNER JOIN site s ON i.site_id = s.id WHERE s.site_id = #{siteId} AND i.path IN <foreach collection="paths" item="path" index="index" open="(" separator="," close=")"> #{path} </foreach> </select>src/main/java/org/craftercms/studio/api/v2/security/SemanticsAvailableActionsResolver.java (1)
33-36: Fix outdated @param description fordetailedItem.The Javadoc still describes
detailedItemas just “Item,” which is now misleading after the type change toContentItem. Please update the parameter description accordingly.- * @param detailedItem Item + * @param detailedItem content item details
🧹 Nitpick comments (2)
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml (1)
468-475: Use systemTypeFolder parameter instead of hard-coded 'folder'.For consistency and configurability, prefer '${systemTypeFolder}' across queries.
Apply this diff:
- AND i.system_type <> 'folder' + AND i.system_type <> '${systemTypeFolder}' ... - AND i.system_type <> 'folder' + AND i.system_type <> '${systemTypeFolder}' ... - AND i.system_type <> 'folder' + AND i.system_type <> '${systemTypeFolder}' ... - AND system_type = 'folder' + AND system_type = '${systemTypeFolder}'Also applies to: 483-495, 553-555, 705-707
src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java (1)
886-900: Ensure parent directories exist before file creation.createEmptyFiles() may pass paths whose parent dirs don’t exist. Add mkdirs() before createNewFile() to avoid failures.
Apply this diff:
private void addEmptyFile(Repository repo, String siteId, String path) throws ServiceLayerException { try { File file = new File(repo.getDirectory().getParent(), path); - if (!file.exists() && !file.createNewFile()) { + if (!file.exists()) { + File parentDir = file.getParentFile(); + if (parentDir != null) { + parentDir.mkdirs(); + } + if (!file.createNewFile()) { logger.error("Failed to create file to site '{}' path '{}'", siteId, path); throw new ServiceLayerException(format("Failed to create file to site '%s' path '%s'", siteId, path)); + } } helper.addFiles(repo, siteId, path);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/org/craftercms/studio/api/v2/dal/Item.java(3 hunks)src/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.java(1 hunks)src/main/java/org/craftercms/studio/api/v2/security/SemanticsAvailableActionsResolver.java(2 hunks)src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java(0 hunks)src/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.java(1 hunks)src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.5-to-5.0.0.6.sql(1 hunks)src/main/resources/crafter/studio/upgrade/pipelines.yaml(1 hunks)src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml(2 hunks)src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml(1 hunks)
💤 Files with no reviewable changes (1)
- 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/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml
- src/main/java/org/craftercms/studio/api/v2/dal/Item.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/4.x-to-5.0.new-schema.sql:0-0
Timestamp: 2025-08-20T19:07:36.327Z
Learning: In CrafterCMS 4.x-to-5.x upgrade scripts, the previous_path and last_published_on columns exist in the item table during the initial migration phases and are only dropped later in the cleanup script (5.0-t3-to-5.0-t4.cleanup.sql). The upgrade sequence is: 5.0-t → 5.0-t2 (new schema) → 5.0-t3 (publisher upgrade) → 5.0-t4 (populate item target) → 5.0-t5 (cleanup).
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/5.0-t2-to-5.0-t3.publisher-upgrade.sql:28-31
Timestamp: 2025-08-20T19:09:04.909Z
Learning: In CrafterCMS 5.0 publisher upgrade scripts, the publish_package and publish_item tables intentionally use different bitmaps for their state values. This is by design - they are not meant to share a common state bitmap layout.
📚 Learning: 2025-08-20T19:07:36.327Z
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/4.x-to-5.0.new-schema.sql:0-0
Timestamp: 2025-08-20T19:07:36.327Z
Learning: In CrafterCMS 4.x-to-5.x upgrade scripts, the previous_path and last_published_on columns exist in the item table during the initial migration phases and are only dropped later in the cleanup script (5.0-t3-to-5.0-t4.cleanup.sql). The upgrade sequence is: 5.0-t → 5.0-t2 (new schema) → 5.0-t3 (publisher upgrade) → 5.0-t4 (populate item target) → 5.0-t5 (cleanup).
Applied to files:
src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.5-to-5.0.0.6.sql
📚 Learning: 2025-07-31T16:13:39.952Z
Learnt from: jmendeza
PR: craftercms/studio#3768
File: src/main/api/studio-api.yaml:10561-10569
Timestamp: 2025-07-31T16:13:39.952Z
Learning: In CrafterCMS Studio, move operations are represented as `RENAMED` in the `WriteContentResultItem.action` enum for historical reasons, even if the move does not involve a rename.
Applied to files:
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml
📚 Learning: 2025-07-31T15:58:07.963Z
Learnt from: jmendeza
PR: craftercms/studio#3768
File: src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java:1603-1644
Timestamp: 2025-07-31T15:58:07.963Z
Learning: In CrafterCMS Studio GitContentRepositoryImpl, the writeContent method should use fail-fast behavior where the entire operation fails immediately if the collection is empty or if any individual ContentWriteItem write fails. This ensures transactional semantics - either all writes succeed or the operation fails completely.
Applied to files:
src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Travis CI - Pull Request
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml (2)
372-386: LGTM: preserve zero state for folders on move.The CASE that forces folders’ state to 0 on move aligns with the “drop states for folders” invariant.
388-401: LGTM: preserve zero state for folders on copy.The conditional state assignment to 0 for folders on copy is correct and consistent.
src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java (1)
348-364: Folder creation now matches the “stateless folders” contractSetting newly created folders to state
0aligns with the broader “drop states for folders” change and keeps the DAO upserts consistent with the DB migration that normalizes folder rows. Looks good.
src/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.java
Show resolved
Hide resolved
src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java
Show resolved
Hide resolved
src/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml (2)
372-386: Normalize folder state to 0 across all state-mutating paths (not just move)Good change here. However, other queries can still set non-zero states on folders, breaking the “folders have state=0” invariant and potentially leaking folders into state-based queries (e.g., unpublished paths).
Apply the same CASE pattern to all updates that mutate state, or exclude folders explicitly. At minimum:
- setStatesBySiteAndPathBulk
- resetStatesBySiteAndPathBulk
- updateStatesBySiteAndPathBulk
- updateStatesByQuery
- moveItemForSyncTask
- updateItemForSyncTask
Example pattern to apply:
UPDATE item SET state = CASE WHEN system_type = '${systemTypeFolder}' THEN 0 ELSE (state | #{onStatesBitMap}) & ~#{offStatesBitMap} END ...Also consider filtering folders out from getUnpublishedPathsInternal:
AND i.system_type <> '${systemTypeFolder}'
388-401: Copy: keep folder state zero at insert, and align the rest of the DAOSetting state=0 for folders on copy is correct. Please propagate the same invariant to other state-mutating statements in this mapper (see previous comment) to avoid reintroducing non-zero states on folders later via bulk updates or sync tasks.
🧹 Nitpick comments (3)
src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java (1)
350-351: Drop magic number; prefer explicit 0L for clarityState is a long. Using 0L avoids implicit widening and reads clearer. Optionally add a brief comment that folders have no workflow state.
- .withState(0) + .withState(0L)src/main/java/org/craftercms/studio/api/v2/dal/Item.java (1)
28-29: Masking folder state to 0 in getter: OK; use 0L for consistencyLogic is correct and null-safe. Minor nit: return 0L to match the long type.
- return CONTENT_TYPE_FOLDER.equals(systemType) ? 0 : state; + return CONTENT_TYPE_FOLDER.equals(systemType) ? 0L : state;Also applies to: 138-140
src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java (1)
886-894: Idempotent empty-file creation: LGTM; add a tiny race-safe tweakCurrent change avoids failing when the file already exists. A minor improvement to handle TOCTOU between exists() and createNewFile():
- if (!file.exists() && !file.createNewFile()) { + if (file.exists()) { + // no-op + } else if (!file.createNewFile() && !file.exists()) { logger.error("Failed to create file to site '{}' path '{}'", siteId, path); throw new ServiceLayerException(String.format("Failed to create file to site '%s' path '%s'", siteId, path)); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/org/craftercms/studio/api/v2/dal/Item.java(3 hunks)src/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.java(1 hunks)src/main/java/org/craftercms/studio/api/v2/security/SemanticsAvailableActionsResolver.java(2 hunks)src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java(0 hunks)src/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.java(1 hunks)src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.5-to-5.0.0.6.sql(1 hunks)src/main/resources/crafter/studio/upgrade/pipelines.yaml(1 hunks)src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml(2 hunks)src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-20T19:08:06.459Z
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/5.0-t2-to-5.0-t3.publisher-upgrade.sql:0-0
Timestamp: 2025-08-20T19:08:06.459Z
Learning: In the 4.x-to-5.x publisher upgrade migration script (5.0-t2-to-5.0-t3.publisher-upgrade.sql), the item_publish_item table insert intentionally filters to only link PENDING items (publish_state = 1) and excludes COMPLETED items from the item relationship. This is by design in the migration process.
Applied to files:
src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml
📚 Learning: 2025-08-20T19:09:04.909Z
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/5.0-t2-to-5.0-t3.publisher-upgrade.sql:28-31
Timestamp: 2025-08-20T19:09:04.909Z
Learning: In CrafterCMS 5.0 publisher upgrade scripts, the publish_package and publish_item tables intentionally use different bitmaps for their state values. This is by design - they are not meant to share a common state bitmap layout.
Applied to files:
src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml
📚 Learning: 2025-08-20T19:07:36.327Z
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/4.x-to-5.0.new-schema.sql:0-0
Timestamp: 2025-08-20T19:07:36.327Z
Learning: In CrafterCMS 4.x-to-5.x upgrade scripts, the previous_path and last_published_on columns exist in the item table during the initial migration phases and are only dropped later in the cleanup script (5.0-t3-to-5.0-t4.cleanup.sql). The upgrade sequence is: 5.0-t → 5.0-t2 (new schema) → 5.0-t3 (publisher upgrade) → 5.0-t4 (populate item target) → 5.0-t5 (cleanup).
Applied to files:
src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.5-to-5.0.0.6.sqlsrc/main/resources/crafter/studio/upgrade/pipelines.yaml
📚 Learning: 2025-07-31T16:13:39.952Z
Learnt from: jmendeza
PR: craftercms/studio#3768
File: src/main/api/studio-api.yaml:10561-10569
Timestamp: 2025-07-31T16:13:39.952Z
Learning: In CrafterCMS Studio, move operations are represented as `RENAMED` in the `WriteContentResultItem.action` enum for historical reasons, even if the move does not involve a rename.
Applied to files:
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xmlsrc/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.java
📚 Learning: 2025-07-31T15:58:07.963Z
Learnt from: jmendeza
PR: craftercms/studio#3768
File: src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java:1603-1644
Timestamp: 2025-07-31T15:58:07.963Z
Learning: In CrafterCMS Studio GitContentRepositoryImpl, the writeContent method should use fail-fast behavior where the entire operation fails immediately if the collection is empty or if any individual ContentWriteItem write fails. This ensures transactional semantics - either all writes succeed or the operation fails completely.
Applied to files:
src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml (1)
157-158: No change:${systemTypeFolder}is supplied via MyBatis configuration (see src/main/resources/crafter/studio/database-context.xml), not a DAO parameter.Likely an incorrect or invalid review comment.
src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.5-to-5.0.0.6.sql (1)
17-20: LGTM: normalizes folder states and bumps versionThe UPDATE is idempotent and aligns with dropping workflow states for folders. Ensure the pipeline runs this only on 5.0.0.5→5.0.0.6 (not 5.0.0.6→5.0.0.7), as addressed in pipelines.yaml.
src/main/resources/crafter/studio/upgrade/pipelines.yaml (1)
811-816: Incorrect script referenced for 5.0.0.6→5.0.0.7; will re-run prior upgrade and downgrade _meta.versionThis points to 5.0.0.5-to-5.0.0.6.sql under the 5.0.0.6→5.0.0.7 step, which would set version back to 5.0.0.6. Use the correct script for this step, or remove this block and attach the 5.0.0.5→5.0.0.6 script to the prior step instead.
Apply this if you have a 5.0.0.6-to-5.0.0.7.sql script:
- - currentVersion: 5.0.0.6 - nextVersion: 5.0.0.7 - operations: - - type: dbScriptUpgrader - filename: upgrade/5.0.x/5.0.0.5-to-5.0.0.6.sql + - currentVersion: 5.0.0.6 + nextVersion: 5.0.0.7 + operations: + - type: dbScriptUpgrader + filename: upgrade/5.0.x/5.0.0.6-to-5.0.0.7.sqlIf that script doesn’t exist, remove this block and move the dbScriptUpgrader (5.0.0.5-to-5.0.0.6.sql) into the 5.0.0.5→5.0.0.6 step next to the existing globalRepo operation. Based on learnings
Run this to verify the presence of the intended script:
#!/bin/bash set -euo pipefail echo "Looking for 5.0.0.6-to-5.0.0.7.sql:" fd -a '5\.0\.0\.6-to-5\.0\.0\.7\.sql' src/main/resources | sed 's/^/ /' || true echo "Known script(s):" fd -a '5\.0\.0\.5-to-5\.0\.0\.6\.sql' src/main/resources | sed 's/^/ /' || truesrc/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.java (1)
85-86: Removing ITEM_UNLOCK from FOLDER breaks unlock for non-admins/lock ownersSince folder available-actions skip workflow-state filtering, dropping ITEM_UNLOCK from the folder mask removes the ability for permitted users (including lock owners) to unlock folders. Restore ITEM_UNLOCK here or ensure it’s reintroduced downstream before permission ANDing.
- CONTENT_UPLOAD + FOLDER_CREATE + CONTENT_DELETE; + CONTENT_UPLOAD + FOLDER_CREATE + CONTENT_DELETE + ITEM_UNLOCK;src/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.java (1)
283-285: Folders must persist with state = 0We’re still calling
ItemState.savedAndClosed(...)before writing the new folder, which persists non‑zero bits. This contradicts the “drop states for folder items” objective and reintroduces workflow/lock flags the rest of the PR now assumes are gone (e.g., the available-actions resolver skips masking for folders). Please zero the state at creation time.- item.setState(ItemState.savedAndClosed(item.getState())); - item.setSystemType(CONTENT_TYPE_FOLDER); + item.setState(0L); + item.setSystemType(CONTENT_TYPE_FOLDER);src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java (1)
71-78: Skipping workflow-state mask for folders is consistent with "folders have state=0"Given folders no longer carry lock/state and FOLDER possible-actions dropped ITEM_UNLOCK, bypassing the workflow mask for folders is correct. No further gating needed.
Please confirm no UI/clients still expect “Unlock” on folders and that folder lock APIs remain no-ops.
jmendeza
force-pushed
the
feature/7724
branch
from
1b783df to
a636344
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml (1)
372-386: Keep folder states at 0 across all state-mutating statementsGreat: move/copy now force state=0 for folders (Lines 375-376, 393-395). However, several other updates can reintroduce non-zero states for folders:
- setStatesBySiteAndPathBulk (Lines 348-354)
- resetStatesBySiteAndPathBulk (Lines 356-362)
- updateStatesBySiteAndPathBulk (Lines 364-370)
- updateStatesByQuery (Lines 515-527)
- moveItemForSyncTask (Lines 664-673)
Wrap their state assignments with CASE (or filter out folders) to preserve the invariant that folder state is always 0.
-UPDATE item SET state = state | #{statesBitMap} +UPDATE item +SET state = CASE WHEN system_type = '${systemTypeFolder}' + THEN 0 + ELSE state | #{statesBitMap} + END WHERE site_id = #{siteId} AND path IN (...) -UPDATE item SET state = state & ~#{statesBitMap} +UPDATE item +SET state = CASE WHEN system_type = '${systemTypeFolder}' + THEN 0 + ELSE state & ~#{statesBitMap} + END WHERE site_id = #{siteId} AND path IN (...) -UPDATE item set state = (state | #{onStatesBitMap}) & ~#{offStatesBitMap} +UPDATE item +SET state = CASE WHEN system_type = '${systemTypeFolder}' + THEN 0 + ELSE (state | #{onStatesBitMap}) & ~#{offStatesBitMap} + END WHERE site_id = #{siteId} AND path IN (...) -UPDATE item i INNER JOIN site s ON i.site_id = s.id -SET i.state = (i.state | #{onStatesBitMap}) & ~#{offStatesBitMap} +UPDATE item i INNER JOIN site s ON i.site_id = s.id +SET i.state = CASE WHEN i.system_type = '${systemTypeFolder}' + THEN 0 + ELSE (i.state | #{onStatesBitMap}) & ~#{offStatesBitMap} + END WHERE s.site_id = #{siteId} ... -UPDATE item -SET ..., - state = (state | #{onStatesBitMap}) & ~#{offStatesBitMap} +UPDATE item +SET ..., + state = CASE WHEN system_type = '${systemTypeFolder}' + THEN 0 + ELSE (state | #{onStatesBitMap}) & ~#{offStatesBitMap} + END WHERE site_id = #{siteId} AND (path = #{previousPath} OR path LIKE CONCAT(#{previousPath}, '/%'));Also applies to: 388-401, 348-370, 515-527, 664-673
🧹 Nitpick comments (6)
src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java (1)
889-894: Idempotent empty-file creation is good; ensure parent dirs exist to avoid IOExceptionIf parent folders are missing, createNewFile will fail. Proactively create parent directories.
private void addEmptyFile(Repository repo, String siteId, String path) throws ServiceLayerException { try { File file = new File(repo.getDirectory().getParent(), path); + // Ensure parent directories exist to avoid createNewFile() failing + Path parent = file.toPath().getParent(); + if (parent != null) { + Files.createDirectories(parent); + } - if (!file.exists() && !file.createNewFile()) { + if (!file.exists() && !file.createNewFile()) { logger.error("Failed to create file to site '{}' path '{}'", siteId, path); throw new ServiceLayerException(format("Failed to create file to site '%s' path '%s'", siteId, path)); } helper.addFiles(repo, siteId, path);src/main/java/org/craftercms/studio/api/v2/dal/Item.java (1)
21-29: Remove unused non-static importStudioConstants (non-static) import is unused; keep only the static import to reduce noise.
-import org.craftercms.studio.api.v1.constant.StudioConstants; import org.craftercms.studio.model.rest.Person; @@ -import static org.craftercms.studio.api.v1.constant.StudioConstants.CONTENT_TYPE_FOLDER; +import static org.craftercms.studio.api.v1.constant.StudioConstants.CONTENT_TYPE_FOLDER;src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.6-to-5.0.0.7.sql (1)
17-17: Upgrade step looks correct; consider reducing unnecessary writesAdd state <> 0 predicate to avoid updating already-normalized rows on reruns and speed large instances.
-UPDATE item SET state = 0 WHERE system_type = 'folder' ; +UPDATE item SET state = 0 WHERE system_type = 'folder' AND state <> 0;src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml (1)
393-395: Prefer parameter binding (#{}) over string substitution (${})Using ${state} and ${userId} injects values directly into SQL. Even if numeric, prefer #{} to leverage prepared statements and reduce risk.
- CASE WHEN i.system_type = '${systemTypeFolder}' THEN 0 ELSE ${state} END, - null, ${userId}, NOW(), ${userId}, + CASE WHEN i.system_type = '${systemTypeFolder}' THEN 0 ELSE #{state} END, + null, #{userId}, NOW(), #{userId},src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java (2)
71-77: Make workflow mask skip decision resilientToday folders always report state=0, so skipping the workflow mask by type works. To future‑proof, skip only when state is 0, regardless of type.
- long result = userPermissionsBitmap & systemTypeBitmap; - if (!CONTENT_TYPE_FOLDER.equals(detailedItem.getSystemType())) { - long workflowStateBitmap = getPossibleActionsForItemState(detailedItem.getState(), - hasUnlockPermission(detailedItem.getLockOwner(), detailedItem.getState(), siteId, detailedItem.getPath(), username)); - result &= workflowStateBitmap; - } + long result = userPermissionsBitmap & systemTypeBitmap; + long itemState = detailedItem.getState(); + boolean skipWorkflowMask = itemState == 0L; + if (!skipWorkflowMask) { + long workflowStateBitmap = getPossibleActionsForItemState(itemState, + hasUnlockPermission(detailedItem.getLockOwner(), itemState, siteId, detailedItem.getPath(), username)); + result &= workflowStateBitmap; + }
192-195: Use CONTENT_TYPE_FOLDER constant instead of string literalKeep folder checks consistent across codebase.
- if ("folder".equals(itemSystemType)) { + if (CONTENT_TYPE_FOLDER.equals(itemSystemType)) { blobStorePath = appendIfMissing(itemPath, "/"); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/org/craftercms/studio/api/v2/dal/Item.java(3 hunks)src/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.java(1 hunks)src/main/java/org/craftercms/studio/api/v2/security/SemanticsAvailableActionsResolver.java(2 hunks)src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java(0 hunks)src/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.java(1 hunks)src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.6-to-5.0.0.7.sql(1 hunks)src/main/resources/crafter/studio/upgrade/pipelines.yaml(1 hunks)src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml(2 hunks)src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml
- src/main/resources/crafter/studio/upgrade/pipelines.yaml
- src/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.java
- src/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.java
- src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-31T16:13:39.952Z
Learnt from: jmendeza
PR: craftercms/studio#3768
File: src/main/api/studio-api.yaml:10561-10569
Timestamp: 2025-07-31T16:13:39.952Z
Learning: In CrafterCMS Studio, move operations are represented as `RENAMED` in the `WriteContentResultItem.action` enum for historical reasons, even if the move does not involve a rename.
Applied to files:
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml
📚 Learning: 2025-08-20T19:07:36.327Z
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/4.x-to-5.0.new-schema.sql:0-0
Timestamp: 2025-08-20T19:07:36.327Z
Learning: In CrafterCMS 4.x-to-5.x upgrade scripts, the previous_path and last_published_on columns exist in the item table during the initial migration phases and are only dropped later in the cleanup script (5.0-t3-to-5.0-t4.cleanup.sql). The upgrade sequence is: 5.0-t → 5.0-t2 (new schema) → 5.0-t3 (publisher upgrade) → 5.0-t4 (populate item target) → 5.0-t5 (cleanup).
Applied to files:
src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.6-to-5.0.0.7.sql
📚 Learning: 2025-07-31T15:58:07.963Z
Learnt from: jmendeza
PR: craftercms/studio#3768
File: src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java:1603-1644
Timestamp: 2025-07-31T15:58:07.963Z
Learning: In CrafterCMS Studio GitContentRepositoryImpl, the writeContent method should use fail-fast behavior where the entire operation fails immediately if the collection is empty or if any individual ContentWriteItem write fails. This ensures transactional semantics - either all writes succeed or the operation fails completely.
Applied to files:
src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java
🧬 Code graph analysis (1)
src/main/java/org/craftercms/studio/api/v2/dal/Item.java (1)
src/main/java/org/craftercms/studio/api/v1/constant/StudioConstants.java (1)
StudioConstants(28-196)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Travis CI - Pull Request
🔇 Additional comments (3)
src/main/java/org/craftercms/studio/api/v2/dal/Item.java (1)
139-140: Folder state normalization via getter is aligned with PR goalReturning 0 for folders from getState() matches “drop states for folder items” and keeps callers consistent.
src/main/java/org/craftercms/studio/api/v2/security/SemanticsAvailableActionsResolver.java (1)
38-40: No action needed All implementors and call sites have been updated to useContentItem.src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.6-to-5.0.0.7.sql (1)
1-19: Pipeline wiring verified
pipelines.yamlcorrectly referencesupgrade/5.0.x/5.0.0.6-to-5.0.0.7.sqlbetween versions 5.0.0.6 and 5.0.0.7.
jmendeza
force-pushed
the
feature/7724
branch
from
a636344 to
247434a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/craftercms/studio/api/v2/security/SemanticsAvailableActionsResolver.java (1)
30-37: Fix Javadoc param type (Item → ContentItem).The Javadoc still says "Item" for detailedItem.
Apply this diff:
- * @param detailedItem Item + * @param detailedItem ContentItem
🧹 Nitpick comments (1)
src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java (1)
193-195: Use CONTENT_TYPE_FOLDER constant instead of string literal.Prefer the constant for consistency and to avoid drift.
Apply this diff:
- if ("folder".equals(itemSystemType)) { + if (CONTENT_TYPE_FOLDER.equals(itemSystemType)) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/org/craftercms/studio/api/v2/dal/Item.java(1 hunks)src/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.java(1 hunks)src/main/java/org/craftercms/studio/api/v2/security/SemanticsAvailableActionsResolver.java(2 hunks)src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java(1 hunks)src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java(0 hunks)src/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.java(1 hunks)src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.6-to-5.0.0.7.sql(1 hunks)src/main/resources/crafter/studio/upgrade/pipelines.yaml(1 hunks)src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml(2 hunks)src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/craftercms/studio/api/v2/dal/Item.java
👮 Files not reviewed due to content moderation or server errors (6)
- src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.6-to-5.0.0.7.sql
- src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java
- src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.java
- src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml
- src/main/resources/crafter/studio/upgrade/pipelines.yaml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/4.x-to-5.0.new-schema.sql:0-0
Timestamp: 2025-08-20T19:07:36.327Z
Learning: In CrafterCMS 4.x-to-5.x upgrade scripts, the previous_path and last_published_on columns exist in the item table during the initial migration phases and are only dropped later in the cleanup script (5.0-t3-to-5.0-t4.cleanup.sql). The upgrade sequence is: 5.0-t → 5.0-t2 (new schema) → 5.0-t3 (publisher upgrade) → 5.0-t4 (populate item target) → 5.0-t5 (cleanup).
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/5.0-t2-to-5.0-t3.publisher-upgrade.sql:28-31
Timestamp: 2025-08-20T19:09:04.909Z
Learning: In CrafterCMS 5.0 publisher upgrade scripts, the publish_package and publish_item tables intentionally use different bitmaps for their state values. This is by design - they are not meant to share a common state bitmap layout.
📚 Learning: 2025-07-31T16:13:39.952Z
Learnt from: jmendeza
PR: craftercms/studio#3768
File: src/main/api/studio-api.yaml:10561-10569
Timestamp: 2025-07-31T16:13:39.952Z
Learning: In CrafterCMS Studio, move operations are represented as `RENAMED` in the `WriteContentResultItem.action` enum for historical reasons, even if the move does not involve a rename.
Applied to files:
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml
📚 Learning: 2025-08-20T19:08:06.459Z
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/5.0-t2-to-5.0-t3.publisher-upgrade.sql:0-0
Timestamp: 2025-08-20T19:08:06.459Z
Learning: In the 4.x-to-5.x publisher upgrade migration script (5.0-t2-to-5.0-t3.publisher-upgrade.sql), the item_publish_item table insert intentionally filters to only link PENDING items (publish_state = 1) and excludes COMPLETED items from the item relationship. This is by design in the migration process.
Applied to files:
src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml
📚 Learning: 2025-08-20T19:09:04.909Z
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/5.0-t2-to-5.0-t3.publisher-upgrade.sql:28-31
Timestamp: 2025-08-20T19:09:04.909Z
Learning: In CrafterCMS 5.0 publisher upgrade scripts, the publish_package and publish_item tables intentionally use different bitmaps for their state values. This is by design - they are not meant to share a common state bitmap layout.
Applied to files:
src/main/resources/org/craftercms/studio/api/v2/dal/publish/PublishDAO.xml
📚 Learning: 2025-08-20T19:07:36.327Z
Learnt from: jmendeza
PR: craftercms/studio#3800
File: src/main/resources/crafter/studio/database/upgrade/5.0.x/4.x-to-5.x/4.x-to-5.0.new-schema.sql:0-0
Timestamp: 2025-08-20T19:07:36.327Z
Learning: In CrafterCMS 4.x-to-5.x upgrade scripts, the previous_path and last_published_on columns exist in the item table during the initial migration phases and are only dropped later in the cleanup script (5.0-t3-to-5.0-t4.cleanup.sql). The upgrade sequence is: 5.0-t → 5.0-t2 (new schema) → 5.0-t3 (publisher upgrade) → 5.0-t4 (populate item target) → 5.0-t5 (cleanup).
Applied to files:
src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.6-to-5.0.0.7.sql
📚 Learning: 2025-07-31T15:58:07.963Z
Learnt from: jmendeza
PR: craftercms/studio#3768
File: src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java:1603-1644
Timestamp: 2025-07-31T15:58:07.963Z
Learning: In CrafterCMS Studio GitContentRepositoryImpl, the writeContent method should use fail-fast behavior where the entire operation fails immediately if the collection is empty or if any individual ContentWriteItem write fails. This ensures transactional semantics - either all writes succeed or the operation fails completely.
Applied to files:
src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java
🧬 Code graph analysis (1)
src/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.java (2)
src/main/java/org/craftercms/studio/impl/v2/utils/security/SecurityUtils.java (1)
SecurityUtils(29-83)src/main/java/org/craftercms/studio/impl/v2/utils/DateUtils.java (1)
DateUtils(29-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Travis CI - Pull Request
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
src/main/java/org/craftercms/studio/api/v2/security/ContentItemPossibleActionsConstants.java (1)
85-86: KeepITEM_UNLOCKin the folder bitmask.With the folder bitmap now missing
ITEM_UNLOCK, any caller that ANDs user permissions with the system-type mask (the resolver still does) clears the unlock bit for folders. That prevents non-admin users—including the lock owner—from releasing a locked folder, which is a regression from current behaviour and breaks the unlock flow. Please restoreITEM_UNLOCK(or otherwise reintroduce it before the AND) so folder unlock remains possible.- public static final long FOLDER = CONTENT_COPY + CONTENT_CREATE + CONTENT_PASTE + CONTENT_RENAME + CONTENT_CUT + - CONTENT_UPLOAD + FOLDER_CREATE + CONTENT_DELETE; + public static final long FOLDER = CONTENT_COPY + CONTENT_CREATE + CONTENT_PASTE + CONTENT_RENAME + CONTENT_CUT + + CONTENT_UPLOAD + FOLDER_CREATE + CONTENT_DELETE + ITEM_UNLOCK;src/main/java/org/craftercms/studio/api/v2/security/SemanticsAvailableActionsResolver.java (2)
2-2: LGTM on copyright update.
38-40: No lingering references to removed Item-based overload All callers and implementations use thecalculateContentItemAvailableActions(String, String, ContentItem)signature; noItem-typed overload remains.src/main/java/org/craftercms/studio/impl/v2/security/SemanticsAvailableActionsResolverImpl.java (1)
71-77: Critical: Locked folders bypass workflow gating.You skip the workflow-state mask for all folders. Locked folders will expose actions to users without unlock permission.
Apply this diff to only skip the mask when the folder state is truly 0:
- long result = userPermissionsBitmap & systemTypeBitmap; - if (!CONTENT_TYPE_FOLDER.equals(detailedItem.getSystemType())) { - long workflowStateBitmap = getPossibleActionsForItemState(detailedItem.getState(), - hasUnlockPermission(detailedItem.getLockOwner(), detailedItem.getState(), siteId, detailedItem.getPath(), username)); - result &= workflowStateBitmap; - } + long result = userPermissionsBitmap & systemTypeBitmap; + long itemState = detailedItem.getState(); + String systemType = detailedItem.getSystemType(); + boolean skipWorkflowMask = CONTENT_TYPE_FOLDER.equals(systemType) && itemState == 0L; + if (!skipWorkflowMask) { + long workflowStateBitmap = getPossibleActionsForItemState(itemState, + hasUnlockPermission(detailedItem.getLockOwner(), itemState, siteId, detailedItem.getPath(), username)); + result &= workflowStateBitmap; + }src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml (1)
372-396: Consistent zero-state handling for folders looks goodThe CASE guard keeps folder rows from inheriting stale workflow bits during move/copy, which aligns with the rest of the PR’s “state = 0” strategy. Nicely done.
2 participants
craftercms/craftercms#7724
Drop states for folder items
Summary by CodeRabbit
Bug Fixes
Chores