Site history API#3885
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (16)
WalkthroughThis PR adds a new content history endpoint Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as ContentController
participant Service as ContentService
participant ServiceInternal as ContentServiceInternalImpl
participant Repository as GitContentRepository
participant Git as Git/RevWalk
participant Audit as AuditDAO
Client->>Controller: GET /api/2/content/{siteId}/history?start=HEAD&limit=10
Controller->>Service: getHistory(siteId, start, limit)
Service->>ServiceInternal: getHistory(siteId, start, limit)
ServiceInternal->>Repository: getHistory(siteId, start, limit)
Repository->>Git: RevWalk (bounded, TOPO, RevFilter limit)
Git-->>Repository: commits
Repository-->>ServiceInternal: List<RepositoryVersion>
ServiceInternal->>Audit: getCommitAuthors(siteId, path?)
Audit-->>ServiceInternal: author mappings
ServiceInternal->>ServiceInternal: populateAuthor(history)
ServiceInternal-->>Service: Collection<RepositoryVersion>
Service-->>Controller: Collection<RepositoryVersion>
Controller-->>Client: ResultList<RepositoryVersion> (200)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 6
🤖 Fix all issues with AI agents
In
`@src/main/java/org/craftercms/studio/api/v2/service/content/ContentService.java`:
- Around line 320-322: Add a Javadoc comment for the getHistory method to match
the rest of the interface (describe purpose, parameters siteId, start, limit,
return value, and potential ServiceLayerException) and fix the minor formatting
by inserting a space before the throws keyword so the signature reads "int
limit) throws ServiceLayerException"; update the method declaration in
ContentService (getHistory) accordingly.
In
`@src/main/java/org/craftercms/studio/api/v2/service/repository/RepositoryManagementService.java`:
- Around line 30-34: Remove the unused imports RepositoryVersion and Collection
from this interface; specifically delete the import statements for
org.craftercms.studio.model.history.RepositoryVersion and java.util.Collection
(they are not referenced by RepositoryManagementService's method signatures),
then run/organize imports so only required imports (e.g., List,
RepositoryStatus, DiffConflictedFile, IOException) remain.
In
`@src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java`:
- Around line 332-340: Add the missing `@ValidSiteId` annotation to the siteId
PathVariable on the log(...) method so it matches other endpoints and performs
site validation; then address the unused start parameter by either wiring it
through to the contentService.getHistory call and implementing commitFrom
handling in GitContentRepositoryImpl.getHistory (so it actually paginates from
the provided commit) or, if you don't intend to support non-HEAD pagination,
remove the start parameter (and its default) from the log(...) signature and
callers and update any API docs accordingly.
In
`@src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java`:
- Around line 1417-1451: getHistory currently ignores the commitFrom parameter
and uses HEAD unconditionally, and the anonymous RevFilter.clone() returns null
which can trigger NPEs; change the revWalk.start to use commitFrom when non-null
(resolve commitFrom vs HEAD before calling revWalk.markStart) so
pagination/starting point works, and replace RevFilter.clone() to return a new
instance (resetting count to 0 and preserving the same limit behavior) rather
than null (or throw UnsupportedOperationException if you prefer non-cloneable),
ensuring the anonymous filter uses the final limit value from getHistory.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java`:
- Around line 248-252: Add the missing site readiness and parameter annotations
to the getHistory method in ContentServiceImpl: annotate the method with
`@RequireSiteReady` (or `@RequireSiteExists` to match project convention) and
annotate the siteId parameter with `@SiteId` so permission and site checks behave
consistently with other methods like getContentVersionHistory; keep the existing
`@HasPermission` and method signature otherwise and only add the two annotations
to the getHistory declaration.
🧹 Nitpick comments (7)
src/main/java/org/craftercms/studio/impl/v2/service/repository/RepositoryManagementServiceImpl.java (1)
39-43: Unused imports added.Neither
RepositoryVersionnorCollectionappear to be used in this file. If these imports were added in preparation for future changes, consider removing them until they're actually needed to keep the code clean.♻️ Suggested fix
-import org.craftercms.studio.model.history.RepositoryVersion; import java.beans.ConstructorProperties; -import java.util.Collection; import java.util.List;src/main/java/org/craftercms/studio/impl/v2/service/repository/internal/RepositoryManagementServiceInternalImpl.java (1)
54-54: Unused import.
RepositoryVersionis not used in this file. Consider removing it until it's actually needed.src/main/java/org/craftercms/studio/controller/rest/v2/RepositoryManagementController.java (1)
21-23: Unused imports added.The imports
NotEmpty,NotNull,Positive, andRepositoryVersionare not used in this controller. Based on the PR objective (Site history API), the new history endpoint may be in a different controller (likelyContentControllerbased on the AI summary mentioning/api/2/content/{siteId}/history).Consider removing these unused imports to keep the code clean.
♻️ Suggested fix
import jakarta.servlet.http.HttpServletResponse; import jakarta.validation.Valid; -import jakarta.validation.constraints.NotEmpty; -import jakarta.validation.constraints.NotNull; -import jakarta.validation.constraints.Positive; import org.craftercms.commons.validation.annotations.param.ValidExistingContentPath; ... import org.craftercms.studio.api.v2.service.repository.RepositoryManagementService; -import org.craftercms.studio.model.history.RepositoryVersion; import org.craftercms.studio.model.rest.*;Also applies to: 39-39
src/test/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepositoryTest.java (1)
479-490: Test passesnullforRepositoryVersion- consider using a mock.The test constructs
ItemVersionwithnullwhich works for verifying the list size, but may hide issues if anyItemVersionmethods access the underlyingRepositoryVersion. Consider using a mockRepositoryVersionfor more robust testing.♻️ Optional improvement
+ `@Mock` + private RepositoryVersion mockRepositoryVersion; + `@Test` public void getContentVersionHistoryTest() throws GitAPIException, ServiceLayerException, IOException { - ItemVersion version1 = new ItemVersion(null); - ItemVersion version2 = new ItemVersion(null); + ItemVersion version1 = new ItemVersion(mockRepositoryVersion); + ItemVersion version2 = new ItemVersion(mockRepositoryVersion);src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java (1)
333-333: Consider renaming method for consistency.The method name
logdoesn't align with the endpoint path/historyor the existing pattern in this controller (e.g.,getHistoryat line 323). Consider renaming togetSiteHistoryorgetRepositoryHistoryfor consistency.src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (2)
34-41: Field naming may be confusing:committerpopulated fromAuthorIdent.The field
committer(line 34) is populated fromrevCommit.getAuthorIdent().getName()(line 41). In Git terminology, author and committer are distinct concepts. While usinggetAuthorIdent()is correct for Studio's use case (tracking who made the change), the field namecommittermay cause confusion.Consider renaming to
gitAuthoror documenting this distinction in the field comment.
27-68: Consider addingtoString()for debugging.Adding a
toString()method would aid debugging and logging. ThePersonclass in the codebase already implements one (see relevant snippet).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v2/repository/GitContentRepository.javasrc/main/java/org/craftercms/studio/api/v2/service/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v2/service/repository/RepositoryManagementService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RepositoryManagementController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.javasrc/main/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepository.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/repository/RepositoryManagementServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/repository/internal/RepositoryManagementServiceInternalImpl.javasrc/main/java/org/craftercms/studio/model/history/ItemVersion.javasrc/main/java/org/craftercms/studio/model/history/RepositoryVersion.javasrc/main/resources/org/craftercms/studio/api/v2/dal/AuditDAO.xmlsrc/test/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepositoryTest.java
🧰 Additional context used
🧠 Learnings (7)
📓 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.
📚 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/java/org/craftercms/studio/impl/v2/service/repository/internal/RepositoryManagementServiceInternalImpl.javasrc/main/java/org/craftercms/studio/model/history/RepositoryVersion.javasrc/main/java/org/craftercms/studio/api/v2/service/content/ContentService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/impl/v2/service/repository/RepositoryManagementServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/api/v2/repository/GitContentRepository.javasrc/main/java/org/craftercms/studio/api/v2/service/repository/RepositoryManagementService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RepositoryManagementController.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/api/studio-api.yamlsrc/test/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepositoryTest.javasrc/main/java/org/craftercms/studio/model/history/ItemVersion.javasrc/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.javasrc/main/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepository.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/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
📚 Learning: 2025-07-31T15:58:07.963Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 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/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java
📚 Learning: 2025-08-12T22:25:56.964Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 3799
File: src/main/resources/org/craftercms/studio/api/v2/dal/DependencyDAO.xml:75-80
Timestamp: 2025-08-12T22:25:56.964Z
Learning: In the CrafterCMS Studio codebase, input validation for MyBatis queries is performed in Java code before calling the queries, rather than adding defensive checks in the SQL templates. For example, paths collections are validated to be non-empty in Java before calling queries like `getPublishingSoftDependenciesForList` in DependencyDAO.xml.
Applied to files:
src/main/java/org/craftercms/studio/controller/rest/v2/RepositoryManagementController.java
📚 Learning: 2025-12-05T16:39:10.400Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 3860
File: src/main/api/studio-api.yaml:9244-9304
Timestamp: 2025-12-05T16:39:10.400Z
Learning: In repo craftercms/studio (OpenAPI spec src/main/api/studio-api.yaml), the GET endpoint /api/2/system/maintenance is a public endpoint (no authentication). The OpenAPI should add `security: []` to this operation and must not include 401/403 responses.
Applied to files:
src/main/api/studio-api.yaml
📚 Learning: 2025-06-05T17:46:44.344Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 3768
File: src/main/java/org/craftercms/studio/api/v2/content/LifeCycleContent.java:103-109
Timestamp: 2025-06-05T17:46:44.344Z
Learning: In CrafterCMS Studio's LifeCycleContent class, the `amended` flag indicates that content sent by the API client has been updated by the lifecycle controller. The `repoPath` represents the path of the main item sent by the client. When writing content via the `write` method: if the path equals `repoPath`, it means the lifecycle controller is amending the original client content (amended=true); if the path differs from `repoPath`, it means the lifecycle controller is adding new content at a different location (amended=false).
Applied to files:
src/main/api/studio-api.yaml
🧬 Code graph analysis (10)
src/main/java/org/craftercms/studio/impl/v2/service/repository/internal/RepositoryManagementServiceInternalImpl.java (1)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
src/main/java/org/craftercms/studio/model/rest/Person.java (1)
Person(29-102)
src/main/java/org/craftercms/studio/api/v2/service/content/ContentService.java (1)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)
src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java (1)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)
src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java (1)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)
src/main/java/org/craftercms/studio/api/v2/repository/GitContentRepository.java (1)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)
src/main/java/org/craftercms/studio/api/v2/service/repository/RepositoryManagementService.java (1)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)
src/main/java/org/craftercms/studio/controller/rest/v2/RepositoryManagementController.java (1)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)
src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java (3)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)src/main/java/org/craftercms/studio/model/history/ItemVersion.java (1)
ItemVersion(24-65)src/main/java/org/craftercms/studio/api/v2/dal/CommitAuthor.java (1)
CommitAuthor(21-40)
src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java (1)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (10)
src/main/resources/org/craftercms/studio/api/v2/dal/AuditDAO.xml (1)
189-191: LGTM! Conditional path filter enables site-wide history queries.The change correctly makes the
pathfilter optional, allowing the query to return commit authors across all paths when no specific path is provided. This aligns with the new Site history API requirement. The pattern is consistent with other conditional filters in this mapper (e.g.,siteId,siteName,target).src/main/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepository.java (1)
819-822: LGTM!The
getHistorymethod correctly delegates to the local repository, following the same delegation pattern used throughout this class. The implementation is consistent with other similar methods.src/main/java/org/craftercms/studio/api/v2/repository/GitContentRepository.java (1)
200-212: LGTM! Well-documented new API method.The new
getHistorymethod signature is well-designed with clear documentation explaining the expected behavior (equivalent togit log --first-parent --max-count=limit commitFrom). The method follows existing patterns in this interface for history-related operations.src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java (1)
68-68: LGTM!The new
SITE_HISTORYconstant follows the established naming pattern and is correctly placed within the Content controller section.src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java (2)
457-467: LGTM! Site history retrieval with author population.The implementation correctly delegates to the repository layer and populates author information from audit data. The
pathparameter is appropriately passed asnullfor repository-wide history, distinguishing it from item-specific history.
439-455: LGTM! Batch processing for author population.The batching approach with
Lists.partitionis efficient for handling large histories. The implementation correctly maps commit authors to versions.src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java (2)
1350-1350: LGTM! ItemVersion construction refactored to use RepositoryVersion.The construction now correctly wraps the git commit in a
RepositoryVersion, aligning with the composition-based approach introduced in this PR.
105-106: LGTM!The wildcard import for
RevSortconstants is appropriate given the multiple usages throughout the file.src/main/java/org/craftercms/studio/model/history/ItemVersion.java (2)
26-35: Clean composition refactor.Using composition with a final
RepositoryVersionfield is a good approach to encapsulate version metadata and avoid field duplication. The design allowsnullwhich appears intentional based on test usage patterns.
61-64: Good use of@JsonUnwrappedfor API compatibility.The annotation correctly flattens
RepositoryVersionfields into the parent JSON structure, maintaining backward compatibility with existing API consumers. Jackson gracefully handles the null case by simply omitting the unwrapped properties.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
jmendeza
force-pushed
the
feature/8433
branch
5 times, most recently
from
900feec to
d2e49c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java`:
- Around line 1417-1451: RevWalk created in getHistory is not closed and the
anonymous RevFilter's clone() returns this sharing mutable count; fix by
wrapping RevWalk revWalk = new RevWalk(git.getRepository()) in a
try-with-resources so it is closed, and change the RevFilter implementation (the
anonymous class passed to revWalk.setRevFilter) so its clone() returns a new
instance with count reset (or throw UnsupportedOperationException) instead of
returning this, ensuring each filter copy starts with its own count.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java`:
- Around line 248-254: In ContentServiceImpl.remove the redundant
`@RequireSiteExists` annotation from the getHistory(`@SiteId` String siteId, String
start, int limit) method so it only has `@RequireSiteReady` and
`@HasPermission`(...) like the other methods; also remove the now-unused
RequireSiteExists import if it becomes unused after the change.
♻️ Duplicate comments (1)
src/main/api/studio-api.yaml (1)
5763-5816: Fix v2 endpoint using v1 error response (api1BadRequest).
/api/2/content/{siteId}/historyis a v2 API but its400references#/components/responses/api1BadRequest, which likely returns the legacy{message: ...}shape and is inconsistent with the v2ApiResponse-based errors.Proposed fix
/api/2/content/{siteId}/history: get: @@ responses: '200': @@ '400': - $ref: '#/components/responses/api1BadRequest' + $ref: '#/components/responses/BadRequest' '401': $ref: '#/components/responses/Unauthorized' '403': $ref: '#/components/responses/Forbidden' '404': $ref: '#/components/responses/NotFound' '500': $ref: '#/components/responses/InternalServerError'
🧹 Nitpick comments (3)
src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java (1)
1442-1442: Consider validatingcommitFrombefore resolving.If
commitFromis null or an invalid reference,repo.resolve(commitFrom)returns null, causingparseCommit()to fail with an unclear error. Consider adding validation or defaulting toHEADwhencommitFromis not provided.src/main/api/studio-api.yaml (2)
10548-10563: Consider addingrequiredfields forItemVersion(and/or inRepositoryVersion).
WithallOf, client codegen often benefits from explicitrequiredlists; otherwise everything becomes nullable/optional even when the backend always provides fields likeversionNumberandmodifiedDate.
10882-10900:RepositoryVersion.versionNumberis described as a commit id—add a pattern (or rename) to avoid ambiguity.
Elsewhere (e.g.,commitIdin requests) you already constrain commit hashes; doing the same here helps validation and client expectations.Proposed improvement (non-breaking)
RepositoryVersion: type: object properties: versionNumber: type: string description: the commit id + pattern: '^[0-9a-f]{40}$' committer: type: string description: the committer of the commit according to git log author: $ref: '#/components/schemas/Person' description: corresponds to the Studio user author of the commit (if any) modifiedDate: type: string format: date-time description: the date of the commit comment: type: string description: the comment of the commitIf short SHAs are allowed in practice, consider relaxing to
^[0-9a-f]{7,40}$instead (but align it with backend behavior).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v2/repository/GitContentRepository.javasrc/main/java/org/craftercms/studio/api/v2/service/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v2/service/repository/RepositoryManagementService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RepositoryManagementController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.javasrc/main/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepository.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/repository/RepositoryManagementServiceImpl.javasrc/main/java/org/craftercms/studio/model/history/ItemVersion.javasrc/main/java/org/craftercms/studio/model/history/RepositoryVersion.javasrc/main/resources/org/craftercms/studio/api/v2/dal/AuditDAO.xmlsrc/test/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepositoryTest.java
💤 Files with no reviewable changes (2)
- src/main/java/org/craftercms/studio/api/v2/service/repository/RepositoryManagementService.java
- src/main/java/org/craftercms/studio/controller/rest/v2/RepositoryManagementController.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/org/craftercms/studio/impl/v2/service/repository/RepositoryManagementServiceImpl.java
- src/main/java/org/craftercms/studio/api/v2/repository/GitContentRepository.java
- src/main/java/org/craftercms/studio/api/v2/service/content/ContentService.java
- src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java
🧰 Additional context used
🧠 Learnings (7)
📓 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.
📚 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/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepository.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.javasrc/main/api/studio-api.yamlsrc/test/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepositoryTest.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/model/history/ItemVersion.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/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java
📚 Learning: 2025-07-31T15:58:07.963Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 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.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java
📚 Learning: 2025-06-05T17:46:44.344Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 3768
File: src/main/java/org/craftercms/studio/api/v2/content/LifeCycleContent.java:103-109
Timestamp: 2025-06-05T17:46:44.344Z
Learning: In CrafterCMS Studio's LifeCycleContent class, the `amended` flag indicates that content sent by the API client has been updated by the lifecycle controller. The `repoPath` represents the path of the main item sent by the client. When writing content via the `write` method: if the path equals `repoPath`, it means the lifecycle controller is amending the original client content (amended=true); if the path differs from `repoPath`, it means the lifecycle controller is adding new content at a different location (amended=false).
Applied to files:
src/main/api/studio-api.yaml
📚 Learning: 2025-06-05T17:40:42.320Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 3768
File: src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java:183-183
Timestamp: 2025-06-05T17:40:42.320Z
Learning: The v1 ContentService interface methods are planned to be deprecated and replaced by v2 content service methods. Breaking changes to v1 APIs are more acceptable given the planned migration to v2.
Applied to files:
src/main/api/studio-api.yaml
📚 Learning: 2025-12-05T16:39:10.400Z
Learnt from: jmendeza
Repo: craftercms/studio PR: 3860
File: src/main/api/studio-api.yaml:9244-9304
Timestamp: 2025-12-05T16:39:10.400Z
Learning: In repo craftercms/studio (OpenAPI spec src/main/api/studio-api.yaml), the GET endpoint /api/2/system/maintenance is a public endpoint (no authentication). The OpenAPI should add `security: []` to this operation and must not include 401/403 responses.
Applied to files:
src/main/api/studio-api.yaml
🧬 Code graph analysis (4)
src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java (3)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)src/main/java/org/craftercms/studio/model/history/ItemVersion.java (1)
ItemVersion(24-65)src/main/java/org/craftercms/studio/api/v2/dal/CommitAuthor.java (1)
CommitAuthor(21-40)
src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java (1)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)
src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java (1)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)
src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java (1)
src/main/java/org/craftercms/studio/model/history/RepositoryVersion.java (1)
RepositoryVersion(27-69)
⏰ 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 (12)
src/main/resources/org/craftercms/studio/api/v2/dal/AuditDAO.xml (1)
189-191: LGTM!The conditional path filter correctly makes the
primary_target_valueclause optional, allowinggetCommitAuthorsto be used for both path-specific and site-wide history queries. The MyBatis conditional syntax is properly structured.src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java (1)
1350-1350: LGTM!The refactored construction properly wraps
RevCommitdata inRepositoryVersionbefore passing toItemVersion, aligning with the new history data model.src/main/java/org/craftercms/studio/model/history/ItemVersion.java (1)
24-64: Clean refactoring to wrapRepositoryVersion.The restructured
ItemVersionproperly encapsulates repository version metadata via composition. The@JsonUnwrappedannotation ensures backward-compatible JSON serialization by flatteningRepositoryVersionfields. The retainedpath,oldPath, andrevertibleproperties appropriately extend the base version data with item-specific context.src/main/api/studio-api.yaml (1)
5763-5816: Replaceapi1BadRequestreference with proper v2 error response definition.
The 400 response at line 5807 references#/components/responses/api1BadRequest, which is a v1 error response. Replace it with the corresponding v2 error response definition (typicallyBadRequestor similar v2-compatible response).The
operationId: getRepositoryLogis unique with no duplicates, and/api/2/repository/loghas been successfully removed.Likely an incorrect or invalid review comment.
src/main/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepository.java (1)
819-822: LGTM!The delegation pattern is consistent with other repository-level operations in this class. History retrieval operates at the git commit level rather than individual file level, so bypassing blob store handling is correct.
src/test/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepositoryTest.java (1)
479-490: LGTM!The test correctly adapts to the new
ItemVersion(RepositoryVersion)constructor. Usingnullfor theRepositoryVersionparameter is appropriate here since the test focuses on verifying proxy delegation behavior, not the internal structure ofItemVersion.src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java (2)
68-68: LGTM!The
SITE_HISTORYconstant is well-placed in the Content section and follows the existing naming conventions. The path/{siteId}/historyaligns with the RESTful pattern used by other site-scoped endpoints.
114-115: Good reorganization.Moving
CORRUPTEDandREPAIRconstants to the Repository section improves semantic grouping, as these operations relate to repository-level concerns rather than audit-specific functionality.src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java (3)
431-455: Well-structured batch processing for author population.The
populateAuthormethod efficiently handles large history sets by:
- Using batch partitioning (
FETCH_AUTHOR_FROM_COMMITS_BATCH_SIZE = 1000) to avoid oversized DB queries- Filtering null version numbers before querying
- Using a map for O(1) author lookups within each batch
The dual-purpose design (passing
nullpath for repository history vs. specific path for content item history) is clean.
457-467: LGTM!The
getHistorymethod follows the established pattern of other service methods:
- Retrieves the site entity for validation and ID lookup
- Delegates to the repository layer for git history
- Enriches results with author information from audit data
- Wraps repository exceptions appropriately
419-429: LGTM!The refactored
getContentVersionHistorycorrectly adapts to the newItemVersionmodel by extractingRepositoryVersioninstances for author population while preserving the original return type.src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java (1)
333-341: LGTM!The new site history endpoint is well-implemented:
@ValidSiteIdproperly applied to the path variable for consistent validation@Positiveensures a valid limit value@NotEmptyprevents explicitly emptystartvalues while allowing the defaultRESULT_KEY_HISTORYis properly defined in ResultConstants and follows the collection pattern withsetEntities()The design cleanly separates site-level history (
history()) from item-level history (getContentVersionHistory()at line 328).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
jmendeza
force-pushed
the
feature/8433
branch
2 times, most recently
from
107a51a to
9c12841
Compare
jmendeza
force-pushed
the
feature/8433
branch
from
9c12841 to
1ab1de3
Compare
2 participants
Site history API
craftercms/craftercms#8433
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.