Sync from repo rename detection (WIP) #3849
Conversation
WalkthroughThe changes enhance move operations across the DAO and sync layers to preserve content metadata and last-modified timestamps. The Changes
Sequence Diagram(s)sequenceDiagram
participant Sync as SyncFromRepositoryTask
participant Repo as GitContentRepositoryImpl
participant DAO as ItemDAO
participant DB as Database
Sync->>Repo: Process diff entries
Repo->>Repo: Apply RenameDetector.compute()
Repo-->>Sync: Transformed diff entries (with renames detected)
loop For each MOVE operation
Sync->>Sync: Compute oldPath and newPath
Sync->>DAO: moveItemForSyncTask(newPath metadata,<br/>lastModifiedOn, contentType fields...)
DAO->>DB: moveItemInternal with expanded fields
DB-->>DAO: Update item + metadata
DAO-->>Sync: Move complete
Sync->>Sync: Invalidate configuration cache
Sync->>Sync: Update dependencies
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
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)
382-402: Fix malformed dynamic SQL inmoveItemInternalSET clauseIn the dynamic SET block,
systemTypeandmimeTypebranches end with a comma while each conditional also starts with a leading comma:<if test="systemType != null"> , i.system_type = #{systemType}, </if> <if test="mimeType != null"> , i.mime_type = #{mimeType}, </if>When both conditions are true, this generates SQL like:
..., i.system_type = ?, , i.mime_type = ?, ...which is invalid due to the double commas. The trailing commas should be removed so each optional fragment contributes exactly one leading comma. For example:
- <if test="systemType != null"> - , i.system_type = #{systemType}, - </if> - <if test="mimeType != null"> - , i.mime_type = #{mimeType}, - </if> + <if test="systemType != null"> + , i.system_type = #{systemType} + </if> + <if test="mimeType != null"> + , i.mime_type = #{mimeType} + </if>
714-727:updateMovedFoldersdoes not update the root folder pathIn
updateMovedFolders, the CASE expression includes:WHEN path = #{sourcePath} THEN #{targetPath} WHEN path LIKE CONCAT(TRIM(TRAILING '/' FROM #{sourcePath}), '/%') THEN ...but the
WHEREclause is:WHERE site_id = #{siteId} AND system_type = 'folder' AND path LIKE CONCAT(TRIM(TRAILING '/' FROM #{sourcePath}), '/%');This WHERE condition excludes the row where
path = #{sourcePath}, so the first CASE branch is never hit and the root folder itself is never updated—only its descendants are. If the intent is to move the folder and its subtree, the WHERE should include the root path as well, e.g.:- AND system_type = 'folder' - AND path LIKE CONCAT(TRIM(TRAILING '/' FROM #{sourcePath}), '/%'); + AND system_type = 'folder' + AND ( + path = #{sourcePath} + OR path LIKE CONCAT(TRIM(TRAILING '/' FROM #{sourcePath}), '/%') + );src/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.java (1)
663-691: Fix IGNORE_FILES check inprocessMoveto use file names, not full pathsIn
processMove:String oldPath = CS.removeEnd(repoOperation.getPath(), EMPTY_FILE_END); String newPath = CS.removeEnd(repoOperation.getMoveToPath(), EMPTY_FILE_END); if (!ArrayUtils.contains(IGNORE_FILES, oldPath) && !ArrayUtils.contains(IGNORE_FILES, newPath)) { // ... }
IGNORE_FILESis defined as a set of file names (e.g.,.keep,.DS_Store), but here it’s compared against full paths (/site/.../.keep). This will almost never match, so ignore rules won’t apply for MOVE operations, unlike CREATE/UPDATE which useFilenameUtils.getName(...).To keep behavior consistent and actually respect ignore files during moves, the check should be based on the file name, for both old and new paths. For example:
- if (!ArrayUtils.contains(IGNORE_FILES, oldPath) && - !ArrayUtils.contains(IGNORE_FILES, newPath)) { + String oldName = FilenameUtils.getName(oldPath); + String newName = FilenameUtils.getName(newPath); + if (!ArrayUtils.contains(IGNORE_FILES, oldName) && + !ArrayUtils.contains(IGNORE_FILES, newName)) { itemDao.moveItemForSyncTask(site.getSiteId(), oldPath, newPath, null, metadata.previewUrl, metadata.label, user.getId(), repoOperation.getDateTime().toInstant(), onStateBitMap, offStateBitmap, metadata.contentTypeId, contentService.getContentTypeClass(site.getSiteId(), newPath), StudioUtils.getMimeType(FilenameUtils.getName(newPath)), contentRepository.getContentSize(site.getSiteId(), newPath));
🧹 Nitpick comments (6)
src/test/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTaskTest.java (1)
27-46: Test coverage for MOVE ingetCreatedPathslooks good; minor assert style nitThe test correctly verifies that MOVE contributes the destination path (
/new-one) and that created paths are ordered as expected. Only minor nit: for readability, preferassertEquals(2, createdPaths.size())so the expected value is first.src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java (1)
62-63: RenameDetector integration is sound; consider making rename score configurableUsing
RenameDetectorand mappingDiffEntry.ChangeType.RENAMEtoRepoOperationwithAction.MOVEcorrectly enables rename/move detection in the diff pipeline. The rest ofprocessDiffEntrystill behaves as before for ADD/MODIFY/DELETE/COPY.The only concern is the hard‑coded
renameDetector.setRenameScore(10), which is a very low threshold and may over‑detect renames compared to other git/history views. Given the comment about keeping this in sync with history and other diffing commands, consider pulling the score from configuration or aligning it with your existingDiffConfig/rename settings.Also applies to: 287-299
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml (1)
681-693: Commented‑outmoveItemForSyncTaskmappingThe XML mapping for
moveItemForSyncTaskis fully commented out. If the Java mapper now delegates via a default method tomoveItemInternal, this is fine; otherwise, calls tomoveItemForSyncTaskwill fail at runtime due to a missing statement.Please confirm that the Java
ItemDAOinterface no longer expects an XML statement with this id; if so, consider deleting this commented block entirely to avoid confusion.src/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.java (2)
709-717:updateItemRowignoredflag uses sharedcontainshelper correctlyThe refactor to compute the
ignoredflag via the staticcontains(IGNORE_FILES, fileName)keeps the same semantics as before while centralizing on the lang3ArrayUtilshelper. This is consistent with other usages that rely on file names instead of full paths.
256-263: Confirm workflow cancellation behavior for MOVE operations after rename detectionWith rename detection enabled, a file rename now yields a single
RepoOperationwith action MOVE (oldpath, newmoveToPath) instead of a DELETE+CREATE pair. The existingcancelWorkflowlogic still calls:for (RepoOperation repoOperation : operationsFromDelta) { String path = repoOperation.getPath(); cancelAllPackagesForPath(site.getSiteId(), path); }so it only cancels packages for the source path, whereas previously a DELETE+CREATE sequence would have cancelled for both old and new paths. If you rely on cancelling workflow items that reference the new path as well, you may need to extend this to also consider
moveToPathwhenaction == MOVE.Please review whether cancelling only on the old path is sufficient for your sync semantics, and adjust if necessary (e.g., by also cancelling
repoOperation.getMoveToPath()for MOVE actions).Also applies to: 505-521
src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.java (1)
26-26: Extended move metadata andInstantusage: mapping & docs need follow‑upThe extension of
moveItem/moveItemInternalto takeInstant lastModifiedOnand content metadata (contentTypeId,contentTypeClass,mimeType,contentSize) looks good and theInstant.now()default for UI‑driven moves is reasonable.Two follow‑ups to consider:
MyBatis mapping for
Instantand new fields
Ensure the correspondingItemDAO.xmlmappings forLAST_MODIFIED_ON,CONTENT_TYPE_ID,SYSTEM_TYPE,MIME_TYPE, andSIZEare updated to:
- Accept
java.time.InstantforLAST_MODIFIED_ON(proper JDBC type or type handler), and- Match the new parameter names.
Also note thatupdateItemForSyncTaskstill uses@Param(LAST_MODIFIED_ON) String lastModifiedOn; if both flows hit the same column, confirm that the mapper handles both types consistently or plan to migrate that method toInstantas well.Javadoc alignment
The Javadoc formoveItemInternal(Lines 282‑294) no longer documentslastModifiedOnand the content metadata parameters. Updating it to reflect the new signature will reduce confusion for future maintainers.Also applies to: 260-267, 295-308
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.java(4 hunks)src/main/java/org/craftercms/studio/impl/v1/util/ContentUtils.java(2 hunks)src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java(2 hunks)src/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.java(8 hunks)src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml(2 hunks)src/test/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTaskTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: alhambrav
Repo: craftercms/studio PR: 3814
File: src/main/api/studio-api.yaml:7-7
Timestamp: 2025-09-22T21:12:13.850Z
Learning: Repo: craftercms/studio — Versioning workflow: maintainers update pom.xml, docs, and runtime version endpoints close to release; PRs that only bump OpenAPI info.version (src/main/api/studio-api.yaml) are acceptable without immediate pom/docs changes.
Learnt from: jmendeza
Repo: craftercms/studio PR: 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.
📚 Learning: 2025-10-03T19:54:12.143Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 3828
File: src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java:95-95
Timestamp: 2025-10-03T19:54:12.143Z
Learning: In the craftercms/studio project, `org.apache.commons.lang3.Strings.CS` from Apache Commons Lang3 (version 3.18.0+) is used for null-safe, case-sensitive string operations. The static import `import static org.apache.commons.lang3.Strings.CS;` is the correct approach for this utility, not a custom CS class.
Applied to files:
src/main/java/org/craftercms/studio/impl/v1/util/ContentUtils.javasrc/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.java
📚 Learning: 2025-11-13T00:55:32.420Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 3844
File: src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/dependency/resolver-config.xml:47-48
Timestamp: 2025-11-13T00:55:32.420Z
Learning: In CrafterCMS (craftercms/studio repository), paths like /myindex.xml, /reindex.xml, or /subindex.xml that contain "index.xml" as a substring are not valid paths. The dependency resolver regex patterns that exclude any href containing "index.xml" substring are working as intended.
Applied to files:
src/main/java/org/craftercms/studio/impl/v1/util/ContentUtils.java
📚 Learning: 2025-07-31T16:13:39.952Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 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/dal/ItemDAO.javasrc/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.java
📚 Learning: 2025-08-20T19:07:36.327Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 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/org/craftercms/studio/api/v2/dal/ItemDAO.xml
📚 Learning: 2025-09-22T21:12:13.850Z
Learnt from: alhambrav
Repo: craftercms/studio PR: 3814
File: src/main/api/studio-api.yaml:7-7
Timestamp: 2025-09-22T21:12:13.850Z
Learning: Repo: craftercms/studio — Versioning workflow: maintainers update pom.xml, docs, and runtime version endpoints close to release; PRs that only bump OpenAPI info.version (src/main/api/studio-api.yaml) are acceptable without immediate pom/docs changes.
Applied to files:
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xmlsrc/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.java
📚 Learning: 2025-09-05T18:02:53.138Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 3806
File: src/main/resources/crafter/studio/groovy/whitelist:122-124
Timestamp: 2025-09-05T18:02:53.138Z
Learning: CrafterCMS 4.x requires Java 21, so Studio projects inheriting from the CrafterCMS parent POM automatically get Java 21 support without needing to declare it locally.
Applied to files:
src/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.java
📚 Learning: 2025-06-05T19:30:35.603Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 3768
File: src/main/resources/crafter/studio/studio-services-context.xml:428-436
Timestamp: 2025-06-05T19:30:35.603Z
Learning: In the CrafterCMS Studio codebase, ContentServiceInternalImpl (V2) depends on AuditServiceInternalImpl, but AuditServiceInternalImpl depends on ContentServiceImpl (V1), not V2. This means there's currently no circular dependency between V2 services. This will become an issue when ContentService V1 is retired and will need to be addressed at that time as part of the V1 to V2 migration strategy.
Applied to files:
src/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.java
🧬 Code graph analysis (2)
src/test/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTaskTest.java (1)
src/main/java/org/craftercms/studio/api/v2/dal/RepoOperation.java (1)
RepoOperation(21-111)
src/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.java (3)
src/main/java/org/craftercms/studio/impl/v1/util/ContentUtils.java (1)
ContentUtils(39-186)src/main/java/org/craftercms/studio/api/v2/dal/RepoOperation.java (1)
RepoOperation(21-111)src/main/java/org/craftercms/studio/api/v2/utils/StudioUtils.java (1)
StudioUtils(48-243)
⏰ 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/java/org/craftercms/studio/impl/v1/util/ContentUtils.java (1)
34-36: Use ofStrings.CSingetParentUrlis consistent and safeSwitching to
CS.removeEndkeeps behavior (null‑safe, case‑sensitive) and matches the wider project use ofStrings.CS. No issues from a correctness standpoint.
Based on learningsAlso applies to: 114-116
src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml (1)
695-712:updateItemForSyncTaskfield updates look coherent; verifylastModifiedOntypeThe extended
updateItemForSyncTasknow updatescontent_type_id,system_type,mime_type, andsizein addition to the previous fields, which aligns with the richer metadata coming from the sync task. The overall SET clause is syntactically correct.One thing to double‑check is that the Java mapper method’s
lastModifiedOnparameter type matches what MyBatis expects for thelast_modified_oncolumn (e.g.,Instant,Timestamp, orString) so you don’t get subtle type‑conversion issues at runtime.src/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.java (1)
103-105:getCreatedPathscorrectly switches MOVE to use destination pathThe new
CREATED_PATH_ACTIONSset andgetCreatedPathsimplementation now treat MOVE operations as creating their destination path (moveToPath) instead of the source path, while still including CREATE and COPY and filtering out.keep/empty‑file entries viaEMPTY_FILE_END. That matches how parent IDs and empty files should be updated after a move.Also applies to: 393-411
| /** | ||
| * Move item query for sync task | ||
| * | ||
| * @param siteId site identifier | ||
| * @param previousPath previous path | ||
| * @param newPath new path | ||
| * @param onStatesBitMap state bitmap to flip on | ||
| * @param offStatesBitMap state bitmap to flip off | ||
| */ | ||
| void moveItemForSyncTask(@Param(SITE_ID) String siteId, @Param(PREVIOUS_PATH) String previousPath, @Param(NEW_PATH) String newPath, | ||
| @Param(ON_STATES_BIT_MAP) long onStatesBitMap, | ||
| @Param(OFF_STATES_BIT_MAP) long offStatesBitMap); | ||
| * @param siteId site identifier | ||
| * @param previousPath previous path | ||
| * @param newPath new path | ||
| * @param onStatesBitMap state bitmap to flip on | ||
| * @param offStatesBitMap state bitmap to flip off | ||
| * @param contentTypeId | ||
| * @param contentTypeClass | ||
| * @param mimeType | ||
| * @param contentSize | ||
| */ | ||
| // void moveItemForSyncTask(@Param(SITE_ID) long siteId, @Param(PREVIOUS_PATH) String previousPath, @Param(NEW_PATH) String newPath, | ||
| // @Param(ON_STATES_BIT_MAP) long onStatesBitMap, | ||
| // @Param(OFF_STATES_BIT_MAP) long offStatesBitMap); | ||
|
|
||
| default void moveItemForSyncTask(String siteId, String previousPath, String newPath, | ||
| Long parentId, String newPreviewUrl, | ||
| String label, long userId, Instant lastModifiedOn, | ||
| long onStatesBitMap, long offStatesBitMap, | ||
| String contentTypeId, String contentTypeClass, | ||
| String mimeType, long contentSize) { | ||
| moveItemInternal(siteId, previousPath, newPath, parentId, | ||
| newPreviewUrl, label, onStatesBitMap, offStatesBitMap, | ||
| lastModifiedOn, userId, | ||
| contentTypeId, contentTypeClass, mimeType, contentSize); | ||
| updatePreviousPath(siteId, previousPath, newPath); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
moveItemForSyncTask should likely be transactional and Javadoc needs updating
The new default moveItemForSyncTask nicely reuses moveItemInternal and updates previousPath, but a couple of details stand out:
-
Transactional boundary / atomicity
moveItemis annotated with@TransactionalwhilemoveItemForSyncTaskis not, even though both executemoveItemInternalfollowed byupdatePreviousPath. Unless the caller ofmoveItemForSyncTaskis guaranteed to be in a wider transaction, this can leave the system in a partially updated state (path moved butprevious_pathnot updated, or vice versa).
Consider mirroringmoveItemand annotatingmoveItemForSyncTaskwith@Transactionalto keep both updates atomic. -
Parameter/Javadoc inconsistencies and minor API polish
- The Javadoc block still describes the old, shorter signature and doesn’t mention
parentId,newPreviewUrl,label,userId, orlastModifiedOn, and the order of documented params no longer matches the method signature. contentSizeis a primitivelonghere butLonginmoveItemInternal; if you ever need to pass “unknown”/null(as done in the regularmoveItem), you’ll wantLonghere as well for consistency.
- The Javadoc block still describes the old, shorter signature and doesn’t mention
Updating the annotation and Javadoc, and aligning the contentSize type with moveItemInternal, will make the API more robust and easier to reason about.
I'll verify the review comment by examining the relevant methods in the codebase.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for moveItem method definition and its @Transactional annotation
rg -n -A 20 "^\s*(?:@|void|default)\s+.*moveItem\s*\(" src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.java | head -60
Length of output: 963
🏁 Script executed:
#!/bin/bash
# Search for moveItemInternal method signature
rg -n -A 10 "moveItemInternal" src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.java | head -80Length of output: 1439
🏁 Script executed:
#!/bin/bash
# Search for updatePreviousPath method
rg -n -A 5 "updatePreviousPath" src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.javaLength of output: 572
🏁 Script executed:
#!/bin/bash
# Check for any @Transactional annotations in the file and their locations
rg -n "@Transactional" src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.java -B 5 | head -100Length of output: 506
🏁 Script executed:
#!/bin/bash
# Search for the entire moveItemForSyncTask method and surrounding context
rg -n -B 5 -A 25 "void moveItemForSyncTask" src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.javaLength of output: 1536
🏁 Script executed:
#!/bin/bash
# Check class-level @Transactional or @Repository annotations
rg -n -B 50 "public interface ItemDAO|class ItemDAO" src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.java | grep -E "(@Transactional|@Repository|public interface|public class)" | head -20Length of output: 29
🏁 Script executed:
#!/bin/bash
# Find contentSize parameter types across related methods
rg -n "contentSize" src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.java -B 2 -A 2Length of output: 854
🏁 Script executed:
#!/bin/bash
# Check if there's a concrete implementation class that implements this interface
fd -e java -x rg -l "implements ItemDAO" {} \; 2>/dev/null | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the file structure to understand if ItemDAO is an interface or abstract class
head -50 src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.javaLength of output: 1855
🏁 Script executed:
#!/bin/bash
# Get line count and check for implementation details
rg -n "default void|@Transactional.*moveItem|void moveItem" src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.javaLength of output: 715
Add @Transactional annotation, update Javadoc, and align contentSize type with moveItemInternal
The moveItemForSyncTask method combines two database operations (moveItemInternal and updatePreviousPath) without transaction boundaries. moveItem is marked @Transactional (line 260) for atomicity, but moveItemForSyncTask (line 613) lacks this annotation. Without it, partial updates can occur if the caller isn't in a wider transaction. Additionally, moveItemInternal accepts Long contentSize (line 308), but moveItemForSyncTask uses primitive long (line 618), preventing null handling consistency. The Javadoc (lines 604–608) documents only the original signature and omits parentId, newPreviewUrl, label, userId, and lastModifiedOn.
🤖 Prompt for AI Agents
In src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.java around lines
596-624, the default moveItemForSyncTask method is missing a @Transactional
boundary, its Javadoc is out of date, and contentSize's primitive long type is
inconsistent with moveItemInternal's Long; update the method to be annotated
with @Transactional, change the contentSize parameter to Long to match
moveItemInternal (allowing null), and expand the Javadoc to document parentId,
newPreviewUrl, label, userId, and lastModifiedOn so the signature and docs are
aligned and the two DB operations run atomically.
1 participant
craftercms/craftercms#6854
Sync from repo rename detection (WIP)
This PR is incomplete. Adding it here to be reviewed and completed for a future version, once rename detection feature is discussed.
Summary by CodeRabbit
New Features
Bug Fixes
Tests