Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 41 additions & 13 deletions src/main/java/org/craftercms/studio/api/v2/dal/ItemDAO.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.craftercms.studio.api.v2.dal.item.LightItem;
import org.springframework.transaction.annotation.Transactional;

import java.time.Instant;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -261,7 +262,8 @@ default void moveItem(String siteId, String previousPath, String newPath,
Long parentId, String newPreviewUrl,
String label, long userId) {
moveItemInternal(siteId, previousPath, newPath, parentId,
newPreviewUrl, label, SAVE_AND_CLOSE_ON_MASK, SAVE_AND_CLOSE_OFF_MASK, userId);
newPreviewUrl, label, SAVE_AND_CLOSE_ON_MASK, SAVE_AND_CLOSE_OFF_MASK,
Instant.now(), userId, null, null, null, null);
updatePreviousPath(siteId, previousPath, newPath);
}

Expand Down Expand Up @@ -290,12 +292,20 @@ default void moveItem(String siteId, String previousPath, String newPath,
* @param offStatesBitMap state bitmap to flip off
* @param userId user id of the user performing the move operation
*/
void moveItemInternal(@Param(SITE_ID) String siteId, @Param(PREVIOUS_PATH) String previousPath, @Param(NEW_PATH) String newPath,
void moveItemInternal(@Param(SITE_ID) String siteId,
@Param(PREVIOUS_PATH) String previousPath,
@Param(NEW_PATH) String newPath,
@Param(PARENT_ID) Long parentId,
@Param(NEW_PREVIEW_URL) String newPreviewUrl, @Param(LABEL) String label,
@Param(NEW_PREVIEW_URL) String newPreviewUrl,
@Param(LABEL) String label,
@Param(ON_STATES_BIT_MAP) long onStatesBitMap,
@Param(OFF_STATES_BIT_MAP) long offStatesBitMap,
@Param(USER_ID) long userId);
@Param(LAST_MODIFIED_ON) Instant lastModifiedOn,
@Param(USER_ID) long userId,
@Param(CONTENT_TYPE_ID) String contentTypeId,
@Param(SYSTEM_TYPE) String contentTypeClass,
@Param(MIME_TYPE) String mimeType,
@Param(SIZE) Long contentSize);

/**
* Get content items for given paths
Expand Down Expand Up @@ -586,15 +596,33 @@ default void updateParentId(long siteId, Collection<String> paths) {
/**
* 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);
}
Comment on lines 596 to +624
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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:

  1. Transactional boundary / atomicity
    moveItem is annotated with @Transactional while moveItemForSyncTask is not, even though both execute moveItemInternal followed by updatePreviousPath. Unless the caller of moveItemForSyncTask is guaranteed to be in a wider transaction, this can leave the system in a partially updated state (path moved but previous_path not updated, or vice versa).
    Consider mirroring moveItem and annotating moveItemForSyncTask with @Transactional to keep both updates atomic.

  2. Parameter/Javadoc inconsistencies and minor API polish

    • The Javadoc block still describes the old, shorter signature and doesn’t mention parentId, newPreviewUrl, label, userId, or lastModifiedOn, and the order of documented params no longer matches the method signature.
    • contentSize is a primitive long here but Long in moveItemInternal; if you ever need to pass “unknown”/null (as done in the regular moveItem), you’ll want Long here as well for consistency.

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 -80

Length 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.java

Length 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 -100

Length 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.java

Length 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 -20

Length 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 2

Length 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 -5

Length 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.java

Length 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.java

Length 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.



/**
* Update item query for sync task
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

import static java.lang.String.format;
import static org.apache.commons.io.FilenameUtils.getFullPathNoEndSeparator;
import static org.apache.commons.lang3.StringUtils.removeEnd;
import static org.apache.commons.lang3.Strings.CS;
import static org.craftercms.studio.api.v1.constant.DmConstants.SLASH_INDEX_FILE;
import static org.craftercms.studio.api.v1.constant.StudioConstants.FILE_SEPARATOR;

Expand Down Expand Up @@ -112,7 +112,7 @@ public static boolean matchesPatterns(String uri, List<String> patterns) {
* @return path of the parent item
*/
public static String getParentUrl(String path) {
return getFullPathNoEndSeparator(removeEnd(path, SLASH_INDEX_FILE));
return getFullPathNoEndSeparator(CS.removeEnd(path, SLASH_INDEX_FILE));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import org.eclipse.jgit.api.errors.GitAPIException;
import org.eclipse.jgit.diff.DiffConfig;
import org.eclipse.jgit.diff.DiffEntry;
import org.eclipse.jgit.diff.RenameDetector;
import org.eclipse.jgit.errors.StopWalkException;
import org.eclipse.jgit.internal.storage.file.LockFile;
import org.eclipse.jgit.lib.*;
Expand Down Expand Up @@ -290,7 +291,11 @@ private List<RepoOperation> processDiffEntry(Git git, List<DiffEntry> diffEntrie
long startMark = logger.isDebugEnabled() ? System.currentTimeMillis() : 0;
List<RepoOperation> toReturn = new ArrayList<>();

for (DiffEntry diffEntry : diffEntries) {
RenameDetector renameDetector = new RenameDetector(git.getRepository());
renameDetector.addAll(diffEntries);
renameDetector.setRenameScore(10);
// Note that this rename score needs to be configured in history and any git diff'ing command
for (DiffEntry diffEntry : renameDetector.compute()) {
// Update the paths to have a preceding separator
String pathNew = FILE_SEPARATOR + diffEntry.getNewPath();
String pathOld = FILE_SEPARATOR + diffEntry.getOldPath();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.apache.commons.collections4.ListUtils;
import org.apache.commons.io.FilenameUtils;
import org.apache.commons.lang.ArrayUtils;
import org.apache.commons.lang3.Strings;
import org.apache.ibatis.session.ExecutorType;
import org.apache.ibatis.session.SqlSession;
import org.apache.ibatis.session.SqlSessionFactory;
Expand All @@ -46,6 +47,7 @@
import org.craftercms.studio.api.v2.utils.DalUtils;
import org.craftercms.studio.api.v2.utils.StudioConfiguration;
import org.craftercms.studio.api.v2.utils.StudioUtils;
import org.craftercms.studio.impl.v1.util.ContentUtils;
import org.craftercms.studio.impl.v2.utils.DependencyUtils;
import org.craftercms.studio.impl.v2.utils.TimeUtils;
import org.dom4j.Document;
Expand Down Expand Up @@ -74,8 +76,8 @@
import static java.time.Instant.now;
import static java.util.Comparator.comparing;
import static java.util.Objects.isNull;
import static org.apache.commons.lang3.StringUtils.EMPTY;
import static org.apache.commons.lang3.StringUtils.isNotEmpty;
import static org.apache.commons.lang3.ArrayUtils.contains;
import static org.apache.commons.lang3.StringUtils.*;
import static org.apache.commons.lang3.Strings.CS;
import static org.craftercms.studio.api.v1.constant.DmConstants.*;
import static org.craftercms.studio.api.v1.constant.StudioConstants.CONTENT_TYPE_FOLDER;
Expand All @@ -84,6 +86,7 @@
import static org.craftercms.studio.api.v2.dal.AuditLog.createAuditLogEntry;
import static org.craftercms.studio.api.v2.dal.AuditLogConstants.*;
import static org.craftercms.studio.api.v2.dal.ItemState.*;
import static org.craftercms.studio.api.v2.dal.RepoOperation.Action.MOVE;
import static org.craftercms.studio.api.v2.utils.StudioConfiguration.CONFIGURATION_PATH_PATTERNS;
import static org.craftercms.studio.api.v2.utils.StudioConfiguration.REPO_SYNC_CANCELLED_PACKAGE_COMMENT;
import static org.craftercms.studio.api.v2.utils.StudioUtils.getPublishPackageLockKey;
Expand All @@ -97,7 +100,7 @@ public class SyncFromRepositoryTask implements ApplicationEventPublisherAware {
private static final Logger logger = LoggerFactory.getLogger(SyncFromRepositoryTask.class);
private static final String DEFAULT_CANCELLED_PACKAGE_COMMENT = "Cancelled because of conflicts with changes from repository sync process";

private static final Set<RepoOperation.Action> CREATED_PATH_ACTIONS = Set.of(RepoOperation.Action.CREATE, RepoOperation.Action.COPY, RepoOperation.Action.MOVE);
private static final Set<RepoOperation.Action> CREATED_PATH_ACTIONS = Set.of(RepoOperation.Action.CREATE, RepoOperation.Action.COPY, MOVE);
private static final String EMPTY_FILE_END = FILE_SEPARATOR + EMPTY_FILE;

private final SitesService sitesService;
Expand Down Expand Up @@ -399,12 +402,12 @@ private void syncDatabaseWithRepo(Site site, List<RepoOperation> repoOperationsD
* Get the paths for the actions that created paths in the repo, i.e.: create, copy, move
* Update or delete will not affect parent id updates
*/
private List<String> getCreatedPaths(List<RepoOperation> chunk) {
return chunk.stream()
.filter(repoOperation -> CREATED_PATH_ACTIONS.contains(repoOperation.getAction()))
.map(RepoOperation::getPath)
.filter(p -> !p.endsWith(EMPTY_FILE_END))
.toList();
protected List<String> getCreatedPaths(List<RepoOperation> operations) {
return operations.stream()
.filter(op -> CREATED_PATH_ACTIONS.contains(op.getAction()))
.map(op -> MOVE.equals(op.getAction()) ? op.getMoveToPath() : op.getPath())
.filter(p -> !p.endsWith(EMPTY_FILE_END))
.toList();
}

/**
Expand Down Expand Up @@ -657,26 +660,29 @@ private void processUpdate(ItemDAO itemDao, DependencyDAO dependencyDao, SqlSess
private void processMove(ItemDAO itemDao, DependencyDAO dependencyDao, SqlSession sqlSession,
Site site, RepoOperation repoOperation, User user,
Set<String> allAncestors) throws SiteNotFoundException {
ItemMetadata metadata = getItemMetadata(site.getSiteId(), repoOperation.getMoveToPath());
processAncestors(itemDao, site.getSiteId(), repoOperation.getMoveToPath(), user.getId(),
repoOperation.getDateTime(), allAncestors);
String oldPath = CS.removeEnd(repoOperation.getPath(), EMPTY_FILE_END);
String newPath = CS.removeEnd(repoOperation.getMoveToPath(), EMPTY_FILE_END);
ItemMetadata metadata = getItemMetadata(site.getSiteId(), newPath);
processAncestors(itemDao, site.getSiteId(), newPath, user.getId(),
repoOperation.getDateTime(), allAncestors);
long onStateBitMap = SAVE_AND_CLOSE_ON_MASK;
long offStateBitmap = SAVE_AND_CLOSE_OFF_MASK;
if (metadata.disabled) {
onStateBitMap = onStateBitMap | DISABLED.value;
} else {
offStateBitmap = offStateBitmap | DISABLED.value;
}
if (!ArrayUtils.contains(IGNORE_FILES, FilenameUtils.getName(repoOperation.getPath())) &&
!ArrayUtils.contains(IGNORE_FILES, FilenameUtils.getName(repoOperation.getMoveToPath()))) {
itemDao.moveItemForSyncTask(site.getSiteId(), repoOperation.getPath(), repoOperation.getMoveToPath(), onStateBitMap, offStateBitmap);

updateItemRow(itemDao, site.getId(),
repoOperation.getPath(), metadata.previewUrl, onStateBitMap, offStateBitmap, user.getId(),
repoOperation.getDateTime(), metadata.label, metadata.contentTypeId,
contentService.getContentTypeClass(site.getSiteId(), repoOperation.getPath()),
StudioUtils.getMimeType(FilenameUtils.getName(repoOperation.getPath())),
contentRepository.getContentSize(site.getSiteId(), repoOperation.getPath()));
if (!ArrayUtils.contains(IGNORE_FILES, oldPath) &&
!ArrayUtils.contains(IGNORE_FILES, newPath)) {
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));
// itemDao.moveItem(site.getSiteId(),oldPath, newPath, null, metadata.previewUrl,metadata.label, user.getId());

DependencyUtils.updateDependencies(site.getSiteId(), repoOperation.getMoveToPath(),
repoOperation.getPath(), dependencyServiceInternal, dependencyDao, sqlSession);
Expand Down Expand Up @@ -706,7 +712,7 @@ private void updateItemRow(ItemDAO itemDao, long siteId, String path, String pre
Long size) {
Timestamp sqlTsLastModified = new Timestamp(lastModifiedOn.toInstant().toEpochMilli());
String fileName = FilenameUtils.getName(path);
boolean ignored = org.apache.commons.lang3.ArrayUtils.contains(IGNORE_FILES, fileName);
boolean ignored = contains(IGNORE_FILES, fileName);
itemDao.updateItemForSyncTask(siteId, path, previewUrl, onStatesBitMap, offStatesBitMap, lastModifiedBy, sqlTsLastModified.toString(), label, contentTypeId, systemType, mimeType, size, ignored);
}

Expand Down
40 changes: 26 additions & 14 deletions src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,19 @@
i.state = CASE WHEN i.system_type = '${systemTypeFolder}' THEN 0 ELSE (i.state | #{onStatesBitMap}) &amp; ~#{offStatesBitMap} END,
i.preview_url = IF(i.system_type = '${systemTypeFolder}', NULL, #{newPreviewUrl}),
i.last_modified_by = #{userId},
i.last_modified_on = NOW()
i.last_modified_on = #{lastModifiedOn}
<if test="contentTypeId != null">
, i.content_type_id = #{contentTypeId}
</if>
<if test="systemType != null">
, i.system_type = #{systemType},
</if>
<if test="mimeType != null">
, i.mime_type = #{mimeType},
</if>
<if test="size != null">
, i.size = #{size}
</if>
<if test="label != null">
, i.label = #{label}
</if>
Expand Down Expand Up @@ -666,19 +678,19 @@
WHERE item.id = updates.childId;
</update>

<update id="moveItemForSyncTask">
UPDATE item
SET
path = REPLACE(path, #{previousPath}, #{newPath}),
locked_by = NULL,
state = CASE
WHEN system_type = '${systemTypeFolder}' THEN 0
ELSE (state | #{onStatesBitMap}) &amp; ~#{offStatesBitMap}
END
WHERE
site_id = #{siteId}
AND (path = #{previousPath} OR path LIKE CONCAT(#{previousPath}, '/%'));
</update>
<!-- <update id="moveItemForSyncTask">-->
<!-- UPDATE item-->
<!-- SET-->
<!-- path = REPLACE(path, #{previousPath}, #{newPath}),-->
<!-- locked_by = NULL,-->
<!-- state = CASE-->
<!-- WHEN system_type = '${systemTypeFolder}' THEN 0-->
<!-- ELSE (state | #{onStatesBitMap}) &amp; ~#{offStatesBitMap}-->
<!-- END-->
<!-- WHERE-->
<!-- site_id = #{siteId}-->
<!-- AND (path = #{previousPath} OR path LIKE CONCAT(#{previousPath}, '/%'));-->
<!-- </update>-->

<update id="updateItemForSyncTask">
UPDATE item
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Copyright (C) 2007-2025 Crafter Software Corporation. All Rights Reserved.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as published by
* the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package org.craftercms.studio.impl.v2.sync;

import org.craftercms.studio.api.v2.dal.RepoOperation;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.InjectMocks;
import org.mockito.junit.MockitoJUnitRunner;

import java.util.List;

import static org.junit.jupiter.api.Assertions.assertEquals;

@RunWith(MockitoJUnitRunner.class)
public class SyncFromRepositoryTaskTest {

@InjectMocks
protected SyncFromRepositoryTask task;

@Test
public void getCreatedPathsTest() {
RepoOperation op1 = new RepoOperation(RepoOperation.Action.CREATE, "/path1",
null, null, "123");
RepoOperation op2 = new RepoOperation(RepoOperation.Action.MOVE, "/old-one",
null, "/new-one", "123");

List<String> createdPaths = task.getCreatedPaths(List.of(op1, op2));
assertEquals(createdPaths.size(), 2);
assertEquals(createdPaths.getFirst(), "/path1");
assertEquals(createdPaths.getLast(), "/new-one");
}
}