Refactor bootstrap process to prevent site checks from blocking the startup#3922
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors startup/bootstrap into an event-driven manager with per-site readiness; adds a ChangesGroup/User organization-scope removal
Event-driven bootstrap & per-site readiness
RequireSiteBootstrapComplete annotation, aspect, exception, and handlers
Legacy v1 service/filter/upgrade removals and cleanup
Sequence Diagram(s)sequenceDiagram
participant Context as ApplicationContext
participant BootstrapMgr as BootstrapManagerImpl
participant EventPub as ApplicationEventPublisher
participant RepoCleanup as RepositoryStartupCleanup
participant SitesUpg as StudioUpgradeManagerImpl
participant StateProv as SiteBootstrapStateProviderImpl
participant Aspect as RequireSiteBootstrapCompleteAnnotationHandler
participant Service as ContentServiceImpl
Note over Context,BootstrapMgr: on ContextRefreshedEvent (root only)
Context->>BootstrapMgr: ContextRefreshedEvent
BootstrapMgr->>EventPub: publish StartSitesBootstrapEvent (async)
BootstrapMgr->>EventPub: publish StartSystemUpgradeEvent
EventPub->>RepoCleanup: StartSitesBootstrapEvent -> unlockRepositories()
RepoCleanup->>RepoCleanup: unlockRepositories() (`@LogExecutionTime`)
EventPub->>SitesUpg: StartSystemUpgradeEvent
SitesUpg->>SitesUpg: upgradeSiteConfiguration(...)
SitesUpg->>StateProv: markSiteAsReady(siteId)
Note over Service,Aspect: incoming site-scoped request
Service->>Aspect: intercepted by `@RequireSiteBootstrapComplete`
Aspect->>StateProv: isSiteReady(siteId)?
alt site ready
Aspect->>Service: proceed
else not ready
Aspect->>Service: throw SiteBootstrapNotCompleteException -> handled by ExceptionHandlers (503)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapComplete.java (1)
27-33: 💤 Low valueRemove
@Inheritedand narrow the Javadoc scope toMETHOD-only targeting.The
@Inheritedmarker has no practical effect here:
- It applies only to
ElementType.TYPEtargets;METHODtargets ignore it per the Java specification.- The annotation is placed on implementation methods, not interfaces, so even class-level inheritance doesn't apply.
- The AspectJ handler resolves the annotation via Spring's
AnnotationUtils.findAnnotation(), which handles class-hierarchy traversal independently and doesn't rely on@Inherited.Additionally, the Javadoc on line 27 states "the annotated method," but
@Target({METHOD, ElementType.TYPE})permits both method and type targets. Either narrow@Targetto{METHOD}alone or update the wording to include type-level usage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapComplete.java` around lines 27 - 33, Remove the `@Inherited` marker and restrict the annotation target to METHOD only for RequireSiteBootstrapComplete; update the Javadoc to refer only to "the annotated method" so documentation matches the new `@Target`({METHOD}) and implementation. Locate the RequireSiteBootstrapComplete annotation declaration, delete the `@Inherited` import/usage, change the `@Target` to only METHOD, and adjust the Javadoc sentence to explicitly state it applies to methods.src/main/java/org/craftercms/studio/api/v2/utils/spring/context/BootstrapManager.java (1)
28-66: 💤 Low valueConsider tightening the return type from
ObjecttoApplicationEvent.Three methods (
onContextRefresh,onCleanUpRepositories,onStartSystemUpgrade) returnObjectdocumented as "the next event to be published". All current implementations returnApplicationEventsubtypes, and Spring's@EventListenersupports forwarding any returned value as the next event. Constraining the return type toorg.springframework.context.ApplicationEventmakes the contract self-documenting, improves type safety, and prevents accidental non-event returns while still allowingnullto skip publishing.♻️ Proposed refactor
+import org.springframework.context.ApplicationEvent; import org.springframework.context.event.ContextRefreshedEvent; @@ - Object onContextRefresh(); + ApplicationEvent onContextRefresh(); @@ - Object onCleanUpRepositories(); + ApplicationEvent onCleanUpRepositories(); @@ - Object onStartSystemUpgrade(); + ApplicationEvent onStartSystemUpgrade();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/craftercms/studio/api/v2/utils/spring/context/BootstrapManager.java` around lines 28 - 66, Update the BootstrapManager interface to tighten the return types for event-forwarding methods: change onContextRefresh(), onCleanUpRepositories(), and onStartSystemUpgrade() to return org.springframework.context.ApplicationEvent (allowing null to mean "no event"); update the import to include ApplicationEvent and then adjust all implementing classes to match the new signatures and return the appropriate ApplicationEvent subclasses (or null) from those methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapCompleteAnnotationHandler.java`:
- Around line 73-77: The current handler silently proceeds when
StudioAnnotationUtils.getAnnotationValue(pjp, method, SiteId.class,
String.class) returns empty, which bypasses the bootstrap gate; change
RequireSiteBootstrapCompleteAnnotationHandler so that if siteId is null/empty
you fail closed by throwing an unchecked exception (e.g., IllegalStateException
or a specific configuration/security exception) instead of calling pjp.proceed()
and remove the logger.debug-only path; ensure the exception message names the
missing `@SiteId` on the annotated method (use
method.getDeclaringClass().getName() and method.getName() for context) so
callers can correct the configuration.
In `@src/main/java/org/craftercms/studio/api/v2/dal/GroupDAO.java`:
- Around line 40-46: Javadoc for getAllGroupsTotal is incorrect: update the
method's JavaDoc `@return` description to state it returns the total count of
groups (an int) matching the provided keyword rather than "List of groups";
ensure the JavaDoc comment above int getAllGroupsTotal(`@Param`(KEYWORD) String
keyword) clearly says something like "the total number of groups matching the
keyword" and remove any reference to a list.
In
`@src/main/java/org/craftercms/studio/api/v2/utils/spring/context/SiteBootstrapStateProvider.java`:
- Around line 21-36: Add explicit thread-safety contract to the
SiteBootstrapStateProvider interface by updating its Javadoc to state that
implementations of SiteBootstrapStateProvider (and its methods markSiteAsReady
and isSiteReady) must be safe for concurrent use from multiple threads
(concurrent calls to isSiteReady and markSiteAsReady). If your codebase supports
annotations, also add a `@ThreadSafe` annotation to the interface. Finally, update
the existing implementation(s) that currently use a plain HashSet to a
thread-safe structure (e.g., ConcurrentHashMap.newKeySet() or other concurrent
set) to satisfy the documented requirement.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java`:
- Around line 98-102: The deleteSite method in SitesServiceImpl is guarded by
`@RequireSiteBootstrapComplete` which prevents removing sites that never completed
bootstrap; remove the `@RequireSiteBootstrapComplete` annotation from the
deleteSite(`@SiteId` String siteId) method so deletion remains possible for
failed/stuck bootstraps, keep the existing `@HasPermission`(type =
DefaultPermission.class, action = PERMISSION_DELETE_SITE) and rely on
sitesServiceInternal.deleteSite(siteId) to perform permission and existence
checks (add an explicit existence/permission check inside
sitesServiceInternal.deleteSite if not already present).
In
`@src/main/java/org/craftercms/studio/impl/v2/upgrade/StudioUpgradeManagerImpl.java`:
- Around line 134-135: The current call to
siteBootstrapStateProvider.markSiteAsReady(context.getTarget()) runs even when
upgradeSiteConfiguration((StudioUpgradeContext) context) swallows failures;
change the flow so markSiteAsReady is only invoked after a fully successful
upgrade: modify upgradeSiteConfiguration (or its caller) to surface failures
(throw or return a success flag) and check that result before calling
markSiteAsReady, or wrap the upgrade sequence in try/catch and call
markSiteAsReady only on success while letting exceptions propagate on failure;
refer to upgradeSiteConfiguration(...) and
siteBootstrapStateProvider.markSiteAsReady(...) to locate the change.
In
`@src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java`:
- Around line 68-118: Replace the current pattern of treating kickoff events as
completion signals: define new completion events (e.g.,
CleanupRepositoriesCompletedEvent, SystemUpgradeCompletedEvent,
SitesUpgradeCompletedEvent), update the worker classes
RepositoryStartupCleanup.unlockRepositories(),
StudioUpgradeManagerImpl.startUpgrade(), and
StudioUpgradeManagerImpl.startSitesUpgrade() to publish their respective
CompletedEvent when their work actually finishes, and change
BootstrapManagerImpl to stop listening for CleanupRepositoriesEvent,
StartSystemUpgradeEvent, and StartSitesUpgradeEvent as "done" signals and
instead listen for the corresponding CompletedEvent(s) before releasing
upgradeSemaphore or setting systemReady; ensure upgradeSemaphore.release() moves
from onStartSystemUpgrade() to the handler for SystemUpgradeCompletedEvent and
remove premature logging/returns that marked bootstrap finished until all
CompletedEvent handlers have run.
In
`@src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/SiteBootstrapStateProviderImpl.java`:
- Around line 28-42: The readySites field is a plain HashSet and is accessed
concurrently by SiteBootstrapStateProviderImpl methods markSiteAsReady and
isSiteReady; replace readySites with a thread-safe Set (for example, initialize
readySites as Collections.newSetFromMap(new ConcurrentHashMap<>()) or use
ConcurrentSkipListSet) so concurrent add/contains are safe and visible without
extra synchronization; update the readySites declaration and any usages
(getReadySites, markSiteAsReady, isSiteReady) to use the concurrent set type.
---
Nitpick comments:
In
`@src/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapComplete.java`:
- Around line 27-33: Remove the `@Inherited` marker and restrict the annotation
target to METHOD only for RequireSiteBootstrapComplete; update the Javadoc to
refer only to "the annotated method" so documentation matches the new
`@Target`({METHOD}) and implementation. Locate the RequireSiteBootstrapComplete
annotation declaration, delete the `@Inherited` import/usage, change the `@Target`
to only METHOD, and adjust the Javadoc sentence to explicitly state it applies
to methods.
In
`@src/main/java/org/craftercms/studio/api/v2/utils/spring/context/BootstrapManager.java`:
- Around line 28-66: Update the BootstrapManager interface to tighten the return
types for event-forwarding methods: change onContextRefresh(),
onCleanUpRepositories(), and onStartSystemUpgrade() to return
org.springframework.context.ApplicationEvent (allowing null to mean "no event");
update the import to include ApplicationEvent and then adjust all implementing
classes to match the new signatures and return the appropriate ApplicationEvent
subclasses (or null) from those methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3824cb64-4a00-4ba9-8029-7363b657ce13
📒 Files selected for processing (71)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/aws/elastictranscoder/ElasticTranscoder.javasrc/main/java/org/craftercms/studio/api/v1/aws/mediaconvert/MediaConvert.javasrc/main/java/org/craftercms/studio/api/v1/constant/StudioConstants.javasrc/main/java/org/craftercms/studio/api/v1/service/AbstractRegistrableService.javasrc/main/java/org/craftercms/studio/api/v1/service/ServicesManager.javasrc/main/java/org/craftercms/studio/api/v1/service/aws/ElasticTranscoderService.javasrc/main/java/org/craftercms/studio/api/v1/service/aws/MediaConvertService.javasrc/main/java/org/craftercms/studio/api/v1/service/aws/S3Service.javasrc/main/java/org/craftercms/studio/api/v1/service/webdav/WebDavService.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/AbstractFilter.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/AllFilter.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/ComponentFilter.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/DmFilterWrapperImpl.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/DocumentFilter.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/PageFilter.javasrc/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapComplete.javasrc/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapCompleteAnnotationHandler.javasrc/main/java/org/craftercms/studio/api/v2/dal/Group.javasrc/main/java/org/craftercms/studio/api/v2/dal/GroupDAO.javasrc/main/java/org/craftercms/studio/api/v2/dal/QueryParameterNames.javasrc/main/java/org/craftercms/studio/api/v2/exception/OrganizationNotFoundException.javasrc/main/java/org/craftercms/studio/api/v2/exception/SiteBootstrapNotCompleteException.javasrc/main/java/org/craftercms/studio/api/v2/service/security/GroupService.javasrc/main/java/org/craftercms/studio/api/v2/service/security/UserService.javasrc/main/java/org/craftercms/studio/api/v2/service/security/internal/GroupServiceInternal.javasrc/main/java/org/craftercms/studio/api/v2/service/security/internal/OrganizationServiceInternal.javasrc/main/java/org/craftercms/studio/api/v2/service/security/internal/UserServiceInternal.javasrc/main/java/org/craftercms/studio/api/v2/utils/spring/context/BootstrapManager.javasrc/main/java/org/craftercms/studio/api/v2/utils/spring/context/SiteBootstrapStateProvider.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ExceptionHandlers.javasrc/main/java/org/craftercms/studio/controller/rest/v2/GroupsController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/UsersController.javasrc/main/java/org/craftercms/studio/impl/v1/aws/elastictranscoder/ElasticTranscoderImpl.javasrc/main/java/org/craftercms/studio/impl/v1/aws/elastictranscoder/TranscoderProfileMapper.javasrc/main/java/org/craftercms/studio/impl/v1/aws/mediaconvert/MediaConvertImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/aws/ElasticTranscoderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/aws/MediaConvertServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/aws/S3ServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentItemIdGeneratorImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmContentLifeCycleServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/deployment/DmPublishServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/transaction/TransactionServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/webdav/WebDavServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/repository/GitStartupConfig.javasrc/main/java/org/craftercms/studio/impl/v2/repository/RepositoryStartupCleanup.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/GroupServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/UserServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/internal/GroupServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/internal/OrganizationServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/internal/UserServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/StudioUpgradeManagerImpl.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/plugin/DescriptorV2UpgradeOperation.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/AddSiteUuidOperation.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManager.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/context/SiteBootstrapStateProviderImpl.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/event/StartSitesBootstrapEvent.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/event/StartSitesUpgradeEvent.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/event/StartSystemUpgradeEvent.javasrc/main/java/org/craftercms/studio/model/rest/ApiResponse.javasrc/main/resources/crafter/studio/security/common.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/crafter/studio/studio-upgrade-context.xmlsrc/main/resources/crafter/studio/studio-websocket-context.xmlsrc/main/resources/org/craftercms/studio/api/v2/dal/GroupDAO.xmlsrc/main/webapp/default-site/scripts/api/ProjectServices.groovy
💤 Files with no reviewable changes (31)
- src/main/java/org/craftercms/studio/api/v1/util/filter/AllFilter.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/AbstractFilter.java
- src/main/java/org/craftercms/studio/impl/v1/aws/elastictranscoder/TranscoderProfileMapper.java
- src/main/java/org/craftercms/studio/impl/v1/service/aws/ElasticTranscoderServiceImpl.java
- src/main/java/org/craftercms/studio/api/v1/aws/elastictranscoder/ElasticTranscoder.java
- src/main/java/org/craftercms/studio/impl/v1/service/webdav/WebDavServiceImpl.java
- src/main/java/org/craftercms/studio/api/v2/service/security/internal/OrganizationServiceInternal.java
- src/main/java/org/craftercms/studio/api/v1/service/aws/ElasticTranscoderService.java
- src/main/java/org/craftercms/studio/impl/v1/service/transaction/TransactionServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v1/aws/elastictranscoder/ElasticTranscoderImpl.java
- src/main/java/org/craftercms/studio/impl/v1/service/aws/MediaConvertServiceImpl.java
- src/main/java/org/craftercms/studio/api/v1/constant/StudioConstants.java
- src/main/java/org/craftercms/studio/impl/v2/service/security/internal/OrganizationServiceInternalImpl.java
- src/main/java/org/craftercms/studio/api/v1/aws/mediaconvert/MediaConvert.java
- src/main/java/org/craftercms/studio/impl/v1/aws/mediaconvert/MediaConvertImpl.java
- src/main/java/org/craftercms/studio/api/v2/exception/OrganizationNotFoundException.java
- src/main/java/org/craftercms/studio/api/v1/service/aws/S3Service.java
- src/main/java/org/craftercms/studio/api/v1/service/webdav/WebDavService.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/ComponentFilter.java
- src/main/java/org/craftercms/studio/api/v1/service/AbstractRegistrableService.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/DocumentFilter.java
- src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/plugin/DescriptorV2UpgradeOperation.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/DmFilterWrapperImpl.java
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManager.java
- src/main/java/org/craftercms/studio/impl/v1/service/aws/S3ServiceImpl.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/PageFilter.java
- src/main/java/org/craftercms/studio/api/v1/service/aws/MediaConvertService.java
- src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/AddSiteUuidOperation.java
- src/main/java/org/craftercms/studio/api/v1/service/ServicesManager.java
- src/main/webapp/default-site/scripts/api/ProjectServices.groovy
- src/main/resources/crafter/studio/studio-upgrade-context.xml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/craftercms/studio/impl/v2/repository/GitStartupConfig.java (1)
20-20: ⚡ Quick winRemove unused import.
The
CleanupRepositoriesEventimport is no longer used after switching the event listener toContextRefreshedEvent.♻️ Proposed fix to remove unused import
-import org.craftercms.studio.impl.v2.utils.spring.event.CleanupRepositoriesEvent;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/craftercms/studio/impl/v2/repository/GitStartupConfig.java` at line 20, Remove the unused import CleanupRepositoriesEvent from GitStartupConfig: open the GitStartupConfig class and delete the line importing org.craftercms.studio.impl.v2.utils.spring.event.CleanupRepositoriesEvent since the listener now uses ContextRefreshedEvent and the import is unused; after removal, run a build or organize imports to ensure no other references remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapCompleteAnnotationHandler.java`:
- Around line 68-71: The current handler in
RequireSiteBootstrapCompleteAnnotationHandler returns pjp.proceed() when the
annotation resolution returns null, which allows a fail-open path; change this
to fail-closed by throwing an unchecked exception (e.g., IllegalStateException
or a custom unchecked ConfigurationException) instead of calling pjp.proceed().
Update the null-check block that currently references annotation, logger, method
and pjp.proceed() to log the failure and throw the chosen unchecked exception
with contextual details (class name and method name) so the bootstrap gate
cannot be bypassed when annotation resolution fails.
---
Nitpick comments:
In
`@src/main/java/org/craftercms/studio/impl/v2/repository/GitStartupConfig.java`:
- Line 20: Remove the unused import CleanupRepositoriesEvent from
GitStartupConfig: open the GitStartupConfig class and delete the line importing
org.craftercms.studio.impl.v2.utils.spring.event.CleanupRepositoriesEvent since
the listener now uses ContextRefreshedEvent and the import is unused; after
removal, run a build or organize imports to ensure no other references remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdc96808-7192-4e76-aa4b-df4552ed3534
📒 Files selected for processing (70)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/aws/elastictranscoder/ElasticTranscoder.javasrc/main/java/org/craftercms/studio/api/v1/aws/mediaconvert/MediaConvert.javasrc/main/java/org/craftercms/studio/api/v1/constant/StudioConstants.javasrc/main/java/org/craftercms/studio/api/v1/service/AbstractRegistrableService.javasrc/main/java/org/craftercms/studio/api/v1/service/ServicesManager.javasrc/main/java/org/craftercms/studio/api/v1/service/aws/ElasticTranscoderService.javasrc/main/java/org/craftercms/studio/api/v1/service/aws/MediaConvertService.javasrc/main/java/org/craftercms/studio/api/v1/service/aws/S3Service.javasrc/main/java/org/craftercms/studio/api/v1/service/webdav/WebDavService.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/AbstractFilter.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/AllFilter.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/ComponentFilter.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/DmFilterWrapperImpl.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/DocumentFilter.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/PageFilter.javasrc/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapComplete.javasrc/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapCompleteAnnotationHandler.javasrc/main/java/org/craftercms/studio/api/v2/dal/Group.javasrc/main/java/org/craftercms/studio/api/v2/dal/GroupDAO.javasrc/main/java/org/craftercms/studio/api/v2/dal/QueryParameterNames.javasrc/main/java/org/craftercms/studio/api/v2/exception/OrganizationNotFoundException.javasrc/main/java/org/craftercms/studio/api/v2/exception/SiteBootstrapNotCompleteException.javasrc/main/java/org/craftercms/studio/api/v2/service/security/GroupService.javasrc/main/java/org/craftercms/studio/api/v2/service/security/UserService.javasrc/main/java/org/craftercms/studio/api/v2/service/security/internal/GroupServiceInternal.javasrc/main/java/org/craftercms/studio/api/v2/service/security/internal/OrganizationServiceInternal.javasrc/main/java/org/craftercms/studio/api/v2/service/security/internal/UserServiceInternal.javasrc/main/java/org/craftercms/studio/api/v2/utils/spring/context/BootstrapManager.javasrc/main/java/org/craftercms/studio/api/v2/utils/spring/context/SiteBootstrapStateProvider.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ExceptionHandlers.javasrc/main/java/org/craftercms/studio/controller/rest/v2/GroupsController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/UsersController.javasrc/main/java/org/craftercms/studio/impl/v1/aws/elastictranscoder/ElasticTranscoderImpl.javasrc/main/java/org/craftercms/studio/impl/v1/aws/elastictranscoder/TranscoderProfileMapper.javasrc/main/java/org/craftercms/studio/impl/v1/aws/mediaconvert/MediaConvertImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/aws/ElasticTranscoderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/aws/MediaConvertServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/aws/S3ServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentItemIdGeneratorImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmContentLifeCycleServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/deployment/DmPublishServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/transaction/TransactionServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/webdav/WebDavServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/repository/GitStartupConfig.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/GroupServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/UserServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/internal/GroupServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/internal/OrganizationServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/internal/UserServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/StudioUpgradeManagerImpl.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/plugin/DescriptorV2UpgradeOperation.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/AddSiteUuidOperation.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManager.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/context/SiteBootstrapStateProviderImpl.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/event/StartSitesBootstrapEvent.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/event/StartSitesUpgradeEvent.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/event/StartSystemUpgradeEvent.javasrc/main/java/org/craftercms/studio/model/rest/ApiResponse.javasrc/main/resources/crafter/studio/security/common.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/crafter/studio/studio-upgrade-context.xmlsrc/main/resources/crafter/studio/studio-websocket-context.xmlsrc/main/resources/org/craftercms/studio/api/v2/dal/GroupDAO.xmlsrc/main/webapp/default-site/scripts/api/ProjectServices.groovy
💤 Files with no reviewable changes (31)
- src/main/java/org/craftercms/studio/api/v1/service/aws/S3Service.java
- src/main/java/org/craftercms/studio/api/v1/service/aws/ElasticTranscoderService.java
- src/main/java/org/craftercms/studio/api/v1/aws/mediaconvert/MediaConvert.java
- src/main/java/org/craftercms/studio/api/v1/service/aws/MediaConvertService.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/PageFilter.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/ComponentFilter.java
- src/main/java/org/craftercms/studio/api/v1/aws/elastictranscoder/ElasticTranscoder.java
- src/main/java/org/craftercms/studio/impl/v1/service/webdav/WebDavServiceImpl.java
- src/main/webapp/default-site/scripts/api/ProjectServices.groovy
- src/main/java/org/craftercms/studio/api/v1/service/AbstractRegistrableService.java
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManager.java
- src/main/java/org/craftercms/studio/impl/v1/aws/mediaconvert/MediaConvertImpl.java
- src/main/java/org/craftercms/studio/impl/v1/service/aws/MediaConvertServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v2/service/security/internal/OrganizationServiceInternalImpl.java
- src/main/java/org/craftercms/studio/api/v2/exception/OrganizationNotFoundException.java
- src/main/java/org/craftercms/studio/impl/v1/service/aws/ElasticTranscoderServiceImpl.java
- src/main/java/org/craftercms/studio/api/v2/service/security/internal/OrganizationServiceInternal.java
- src/main/java/org/craftercms/studio/api/v1/constant/StudioConstants.java
- src/main/java/org/craftercms/studio/api/v1/service/ServicesManager.java
- src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/AddSiteUuidOperation.java
- src/main/java/org/craftercms/studio/api/v1/service/webdav/WebDavService.java
- src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/plugin/DescriptorV2UpgradeOperation.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/DmFilterWrapperImpl.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/DocumentFilter.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/AllFilter.java
- src/main/java/org/craftercms/studio/impl/v1/aws/elastictranscoder/TranscoderProfileMapper.java
- src/main/resources/crafter/studio/studio-upgrade-context.xml
- src/main/java/org/craftercms/studio/impl/v1/service/aws/S3ServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v1/service/transaction/TransactionServiceImpl.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/AbstractFilter.java
- src/main/java/org/craftercms/studio/impl/v1/aws/elastictranscoder/ElasticTranscoderImpl.java
✅ Files skipped from review due to trivial changes (2)
- src/main/resources/crafter/studio/studio-websocket-context.xml
- src/main/api/studio-api.yaml
🚧 Files skipped from review as they are similar to previous changes (35)
- src/main/java/org/craftercms/studio/model/rest/ApiResponse.java
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/SiteBootstrapStateProviderImpl.java
- src/main/resources/crafter/studio/security/common.xml
- src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java
- src/main/resources/org/craftercms/studio/api/v2/dal/GroupDAO.xml
- src/main/java/org/craftercms/studio/api/v2/dal/QueryParameterNames.java
- src/main/java/org/craftercms/studio/api/v2/utils/spring/context/BootstrapManager.java
- src/main/java/org/craftercms/studio/api/v2/service/security/internal/GroupServiceInternal.java
- src/main/java/org/craftercms/studio/impl/v2/service/security/internal/UserServiceInternalImpl.java
- src/main/java/org/craftercms/studio/impl/v2/service/security/UserServiceImpl.java
- src/main/java/org/craftercms/studio/api/v2/service/security/UserService.java
- src/main/java/org/craftercms/studio/controller/rest/v2/UsersController.java
- src/main/java/org/craftercms/studio/api/v2/utils/spring/context/SiteBootstrapStateProvider.java
- src/main/java/org/craftercms/studio/api/v2/dal/GroupDAO.java
- src/main/java/org/craftercms/studio/controller/rest/v2/GroupsController.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v2/service/security/GroupServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java
- src/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapComplete.java
- src/main/java/org/craftercms/studio/impl/v2/upgrade/StudioUpgradeManagerImpl.java
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/event/StartSitesBootstrapEvent.java
- src/main/java/org/craftercms/studio/impl/v1/service/deployment/DmPublishServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/event/StartSystemUpgradeEvent.java
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java
- src/main/java/org/craftercms/studio/impl/v2/service/security/internal/GroupServiceInternalImpl.java
- src/main/java/org/craftercms/studio/api/v2/dal/Group.java
- src/main/java/org/craftercms/studio/controller/rest/v2/ExceptionHandlers.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/DmContentLifeCycleServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/event/StartSitesUpgradeEvent.java
- src/main/java/org/craftercms/studio/api/v2/service/security/GroupService.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentItemIdGeneratorImpl.java
- src/main/java/org/craftercms/studio/api/v2/service/security/internal/UserServiceInternal.java
- src/main/java/org/craftercms/studio/api/v2/exception/SiteBootstrapNotCompleteException.java
- src/main/resources/crafter/studio/studio-services-context.xml
- src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java
jmendeza
force-pushed
the
feature/8652-4x
branch
2 times, most recently
from
e1c88f0 to
d4fc4b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java (2)
65-70: 💤 Low valueMisleading log message and dual event emission worth a brief comment.
Two small clarity concerns in
onContextRefresh():
- The log line
"Start temporary files cleanup ..."does not describe what this method actually triggers (sites bootstrap + system upgrade); it appears to be leftover from a previous iteration. Consider updating it (e.g.,"Starting bootstrap process...").- This method both explicitly publishes
StartSitesBootstrapEventand returnsStartSystemUpgradeEvent(which Spring publishes via the listener return value). The dual mechanism is subtle and easy to miss on future edits — a one-line comment noting that the returned event is auto-published by Spring would help maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java` around lines 65 - 70, The log message in onContextRefresh() is misleading and there's no comment explaining the dual-event mechanism: update the second logger.info call to a clearer message such as "Starting bootstrap process..." (or similar) to reflect that this method triggers site bootstrap and system upgrade, and add a one-line comment above the return stating that the returned StartSystemUpgradeEvent is auto-published by Spring via listener return values; locate onContextRefresh(), the applicationEventPublisher.publishEvent(new StartSitesBootstrapEvent(this)) call, and the return new StartSystemUpgradeEvent(this) to apply these changes.
36-37: 💤 Low valueClass Javadoc contradicts
@Order(2)ononContextRefresh().The class-level Javadoc states that all methods should use
@Orderwith the default value to keep events firing in the right order, butonContextRefresh()uses@Order(2). Either update the Javadoc to call out this intentional exception (and explain why this listener must run at order2relative to otherContextRefreshedEventlisteners), or drop the explicit value to match the documented convention.Also applies to: 62-62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java` around lines 36 - 37, Update the class Javadoc in BootstrapManagerImpl to remove the contradiction with the `@Order` usage: either remove the comment that mandates the default `@Order` value for all methods, or explicitly document the intentional exception for onContextRefresh() by naming the method and explaining why it must run with `@Order`(2) relative to other ContextRefreshedEvent listeners; alternatively, if no special ordering is required, remove the explicit `@Order`(2) from onContextRefresh() so it uses the default order and matches the existing Javadoc.src/main/java/org/craftercms/studio/impl/v2/service/security/UserServiceImpl.java (1)
307-310: ⚡ Quick winSkip per-site group lookups for system admins.
isSysAdminis known before the loop, butgroupServiceInternal.getSiteGroups(...)is still called for every site. Fast-path sysadmins to avoid unnecessary per-site lookups.♻️ Proposed refactor
- for (org.craftercms.studio.api.v2.dal.Site site : allSites) { - List<NormalizedGroup> siteGroups = groupServiceInternal.getSiteGroups(site.getSiteId()); - if (isSysAdmin || userGroups.stream().anyMatch(userGroup -> - siteGroups.contains(new NormalizedGroup(userGroup.getGroupName()))) - ) { - sites.add(new Site(site)); - } - } + for (org.craftercms.studio.api.v2.dal.Site site : allSites) { + if (isSysAdmin) { + sites.add(new Site(site)); + continue; + } + List<NormalizedGroup> siteGroups = groupServiceInternal.getSiteGroups(site.getSiteId()); + if (userGroups.stream().anyMatch(userGroup -> + siteGroups.contains(new NormalizedGroup(userGroup.getGroupName())))) { + sites.add(new Site(site)); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/craftercms/studio/impl/v2/service/security/UserServiceImpl.java` around lines 307 - 310, The loop over allSites is calling groupServiceInternal.getSiteGroups(site.getSiteId()) for every site even when isSysAdmin is true; change the logic to short-circuit when isSysAdmin to avoid per-site lookups: either if (isSysAdmin) add/mark allSites as accessible before the loop and skip calling groupServiceInternal.getSiteGroups, or inside the loop check if (isSysAdmin) continue/accept the site without invoking groupServiceInternal.getSiteGroups(site.getSiteId()); reference symbols: isSysAdmin, allSites, groupServiceInternal.getSiteGroups(...), userGroups and NormalizedGroup.src/main/java/org/craftercms/studio/model/Site.java (1)
27-33: ⚡ Quick winConsider adding a no-arg constructor for DTO best practices.
The
Siteclass is used as a response DTO in REST endpoints (e.g.,UsersController.getUserSites()returnsPaginatedResultList<Site>). While no existing code currently instantiatesSitewith a no-arg constructor, adding one is standard practice for DTOs to ensure compatibility with serialization frameworks and future refactoring. The DALSiteclass already follows this pattern.💡 Suggested improvement
public class Site implements Entity { + public Site() { + } + public Site(org.craftercms.studio.api.v2.dal.Site site) { siteId = site.getSiteId(); uuid = site.getSiteUuid(); name = site.getName(); desc = site.getDescription(); state = site.getState(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/craftercms/studio/model/Site.java` around lines 27 - 33, Add a public no-argument constructor to the Site DTO class to follow standard DTO patterns and support serialization frameworks; update the Site class (which currently only has the Site(org.craftercms.studio.api.v2.dal.Site) constructor) to include a public Site() {} alongside the existing constructor so endpoints like UsersController.getUserSites() returning PaginatedResultList<Site> can be deserialized reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java`:
- Around line 87-92: In the catch for InterruptedException around the
upgradeSemaphore.acquire() call in BootstrapManagerImpl, restore the thread's
interrupt status before rethrowing: call Thread.currentThread().interrupt()
inside the catch, then log and wrap/throw the RuntimeException as now so
upstream code can observe the interruption; ensure this change is applied in the
same catch block handling InterruptedException for the
upgradeSemaphore.acquire() invocation.
---
Nitpick comments:
In
`@src/main/java/org/craftercms/studio/impl/v2/service/security/UserServiceImpl.java`:
- Around line 307-310: The loop over allSites is calling
groupServiceInternal.getSiteGroups(site.getSiteId()) for every site even when
isSysAdmin is true; change the logic to short-circuit when isSysAdmin to avoid
per-site lookups: either if (isSysAdmin) add/mark allSites as accessible before
the loop and skip calling groupServiceInternal.getSiteGroups, or inside the loop
check if (isSysAdmin) continue/accept the site without invoking
groupServiceInternal.getSiteGroups(site.getSiteId()); reference symbols:
isSysAdmin, allSites, groupServiceInternal.getSiteGroups(...), userGroups and
NormalizedGroup.
In
`@src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java`:
- Around line 65-70: The log message in onContextRefresh() is misleading and
there's no comment explaining the dual-event mechanism: update the second
logger.info call to a clearer message such as "Starting bootstrap process..."
(or similar) to reflect that this method triggers site bootstrap and system
upgrade, and add a one-line comment above the return stating that the returned
StartSystemUpgradeEvent is auto-published by Spring via listener return values;
locate onContextRefresh(), the applicationEventPublisher.publishEvent(new
StartSitesBootstrapEvent(this)) call, and the return new
StartSystemUpgradeEvent(this) to apply these changes.
- Around line 36-37: Update the class Javadoc in BootstrapManagerImpl to remove
the contradiction with the `@Order` usage: either remove the comment that mandates
the default `@Order` value for all methods, or explicitly document the intentional
exception for onContextRefresh() by naming the method and explaining why it must
run with `@Order`(2) relative to other ContextRefreshedEvent listeners;
alternatively, if no special ordering is required, remove the explicit `@Order`(2)
from onContextRefresh() so it uses the default order and matches the existing
Javadoc.
In `@src/main/java/org/craftercms/studio/model/Site.java`:
- Around line 27-33: Add a public no-argument constructor to the Site DTO class
to follow standard DTO patterns and support serialization frameworks; update the
Site class (which currently only has the
Site(org.craftercms.studio.api.v2.dal.Site) constructor) to include a public
Site() {} alongside the existing constructor so endpoints like
UsersController.getUserSites() returning PaginatedResultList<Site> can be
deserialized reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3de91095-9228-470b-a60a-bdf4d6146d8c
📒 Files selected for processing (21)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapComplete.javasrc/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapCompleteAnnotationHandler.javasrc/main/java/org/craftercms/studio/api/v2/dal/Site.javasrc/main/java/org/craftercms/studio/api/v2/dal/SiteDAO.javasrc/main/java/org/craftercms/studio/api/v2/exception/SiteBootstrapNotCompleteException.javasrc/main/java/org/craftercms/studio/api/v2/service/site/SitesService.javasrc/main/java/org/craftercms/studio/api/v2/utils/spring/context/SiteBootstrapStateProvider.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ExceptionHandlers.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/UserServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/StudioUpgradeManagerImpl.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/context/SiteBootstrapStateProviderImpl.javasrc/main/java/org/craftercms/studio/model/Site.javasrc/main/java/org/craftercms/studio/model/rest/ApiResponse.javasrc/main/resources/crafter/studio/security/common.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v2/dal/SiteDAO.xml
✅ Files skipped from review due to trivial changes (2)
- src/main/java/org/craftercms/studio/api/v2/dal/Site.java
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/SiteBootstrapStateProviderImpl.java
🚧 Files skipped from review as they are similar to previous changes (11)
- src/main/java/org/craftercms/studio/model/rest/ApiResponse.java
- src/main/java/org/craftercms/studio/api/v2/exception/SiteBootstrapNotCompleteException.java
- src/main/resources/crafter/studio/security/common.xml
- src/main/java/org/craftercms/studio/api/v2/utils/spring/context/SiteBootstrapStateProvider.java
- src/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapComplete.java
- src/main/java/org/craftercms/studio/impl/v2/upgrade/StudioUpgradeManagerImpl.java
- src/main/api/studio-api.yaml
- src/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapCompleteAnnotationHandler.java
- src/main/java/org/craftercms/studio/controller/rest/v2/ExceptionHandlers.java
- src/main/resources/crafter/studio/studio-services-context.xml
- src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java (1)
87-91:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore interrupt status when continuing after
InterruptedException.When catching
InterruptedException, the thread's interrupt flag should be restored before continuing so that upstream code or executors can observe the interruption. AddThread.currentThread().interrupt();in the catch block before proceeding with sites upgrade.🛡️ Proposed fix
try { upgradeSemaphore.acquire(); } catch (InterruptedException e) { logger.warn("Interrupted while waiting for system upgrade to complete, starting sites upgrade anyway", e); + Thread.currentThread().interrupt(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java` around lines 87 - 91, The catch block around upgradeSemaphore.acquire() in BootstrapManagerImpl currently swallows InterruptedException; update the catch to restore the thread interrupt status by calling Thread.currentThread().interrupt() before logging/continuing so upstream code can observe the interruption (i.e., inside the catch (InterruptedException e) { Thread.currentThread().interrupt(); logger.warn(...); } for the upgradeSemaphore.acquire() call).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java`:
- Line 62: The class javadoc for BootstrapManagerImpl claims all methods should
use the {`@link` Order} annotation with the default value, but the method
annotated with `@Order`(2) breaks that rule; either revert `@Order`(2) to the
default (LOWEST_PRECEDENCE) on the annotated method or update the
BootstrapManagerImpl class javadoc to explicitly document this exception and the
reason `@Order`(2) is required for that entry point (mention `@Order`(2) and
LOWEST_PRECEDENCE in the explanation).
---
Duplicate comments:
In
`@src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java`:
- Around line 87-91: The catch block around upgradeSemaphore.acquire() in
BootstrapManagerImpl currently swallows InterruptedException; update the catch
to restore the thread interrupt status by calling
Thread.currentThread().interrupt() before logging/continuing so upstream code
can observe the interruption (i.e., inside the catch (InterruptedException e) {
Thread.currentThread().interrupt(); logger.warn(...); } for the
upgradeSemaphore.acquire() call).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 439fa0e9-e487-4d40-b974-952f3789f719
📒 Files selected for processing (21)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapComplete.javasrc/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapCompleteAnnotationHandler.javasrc/main/java/org/craftercms/studio/api/v2/dal/Site.javasrc/main/java/org/craftercms/studio/api/v2/dal/SiteDAO.javasrc/main/java/org/craftercms/studio/api/v2/exception/SiteBootstrapNotCompleteException.javasrc/main/java/org/craftercms/studio/api/v2/service/site/SitesService.javasrc/main/java/org/craftercms/studio/api/v2/utils/spring/context/SiteBootstrapStateProvider.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ExceptionHandlers.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/security/UserServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/StudioUpgradeManagerImpl.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/context/SiteBootstrapStateProviderImpl.javasrc/main/java/org/craftercms/studio/model/Site.javasrc/main/java/org/craftercms/studio/model/rest/ApiResponse.javasrc/main/resources/crafter/studio/security/common.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v2/dal/SiteDAO.xml
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/SiteBootstrapStateProviderImpl.java
🚧 Files skipped from review as they are similar to previous changes (19)
- src/main/resources/org/craftercms/studio/api/v2/dal/SiteDAO.xml
- src/main/java/org/craftercms/studio/api/v2/exception/SiteBootstrapNotCompleteException.java
- src/main/java/org/craftercms/studio/model/rest/ApiResponse.java
- src/main/resources/crafter/studio/security/common.xml
- src/main/java/org/craftercms/studio/api/v2/service/site/SitesService.java
- src/main/java/org/craftercms/studio/api/v2/dal/SiteDAO.java
- src/main/java/org/craftercms/studio/impl/v2/upgrade/StudioUpgradeManagerImpl.java
- src/main/java/org/craftercms/studio/impl/v2/service/security/UserServiceImpl.java
- src/main/java/org/craftercms/studio/api/v2/dal/Site.java
- src/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapCompleteAnnotationHandler.java
- src/main/java/org/craftercms/studio/controller/rest/v2/ExceptionHandlers.java
- src/main/resources/crafter/studio/studio-services-context.xml
- src/main/java/org/craftercms/studio/model/Site.java
- src/main/java/org/craftercms/studio/api/v2/utils/spring/context/SiteBootstrapStateProvider.java
- src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java
- src/main/api/studio-api.yaml
- src/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteBootstrapComplete.java
- src/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImpl.java
Member
Author
|
@coderabbitai would you say this PR is ready to move forward? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather some details about the current state of the PR before giving you a thorough assessment. 🏁 Script executed: Length of output: 202522
Current status from GitHub
What to do next to move it forward
If you want, I can take another pass focused on:
When CI is green and threads are resolved, I’ll be comfortable saying it’s ready to move forward. |
Member
|
@coderabbitai I'm about to send this to a human reviewer, can you have a final look and see if there are any issues in the code? |
🧩 Analysis chainLet me do a thorough final pass on the key code changes before you send this to a human reviewer. 🏁 Script executed: Length of output: 10034 🏁 Script executed: Length of output: 13845 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImplTest.java (1)
63-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse consistent annotation checking approach: apply
AnnotationUtils.getAnnotationto all checks for robustness.Lines 63 and 69 use
method.isAnnotationPresent()for direct checks, while line 89 usesAnnotationUtils.getAnnotation()for meta-annotation support. Since@RequireContentExistsis itself meta-annotated with@RequireSiteExists, and@RequireSiteReadyis meta-annotated with@RequireSiteState, usingAnnotationUtilsconsistently across all three checks will make the tests more resilient to future annotation hierarchy changes and clearer about intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImplTest.java` at line 63, Replace direct checks using method.isAnnotationPresent(...) with AnnotationUtils.getAnnotation(method, ...) and assertNotNull to ensure meta-annotation support; specifically change the assertions that check RequireContentExists and RequireSiteReady (which currently use method.isAnnotationPresent) to use AnnotationUtils.getAnnotation(method, RequireContentExists.class) and AnnotationUtils.getAnnotation(method, RequireSiteReady.class) respectively and assert that the returned annotation is not null, matching the existing use of AnnotationUtils.getAnnotation for RequireSiteExists/RequireSiteState.
🧹 Nitpick comments (1)
src/test/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImplTest.java (1)
89-89: 💤 Low valueAdd space after comma for consistency.
Minor formatting: add a space after the comma in the method call.
📐 Proposed formatting fix
- assertNotNull(AnnotationUtils.getAnnotation(method,RequireSiteExists.class)); + assertNotNull(AnnotationUtils.getAnnotation(method, RequireSiteExists.class));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImplTest.java` at line 89, The assertion call AnnotationUtils.getAnnotation(method,RequireSiteExists.class) is missing a space after the comma; update the invocation in ContentServiceImplTest (the assertNotNull line referencing AnnotationUtils.getAnnotation, RequireSiteExists.class and method) to add a space after the comma so it reads AnnotationUtils.getAnnotation(method, RequireSiteExists.class) for consistent formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/test/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImplTest.java`:
- Line 63: Replace direct checks using method.isAnnotationPresent(...) with
AnnotationUtils.getAnnotation(method, ...) and assertNotNull to ensure
meta-annotation support; specifically change the assertions that check
RequireContentExists and RequireSiteReady (which currently use
method.isAnnotationPresent) to use AnnotationUtils.getAnnotation(method,
RequireContentExists.class) and AnnotationUtils.getAnnotation(method,
RequireSiteReady.class) respectively and assert that the returned annotation is
not null, matching the existing use of AnnotationUtils.getAnnotation for
RequireSiteExists/RequireSiteState.
---
Nitpick comments:
In
`@src/test/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImplTest.java`:
- Line 89: The assertion call
AnnotationUtils.getAnnotation(method,RequireSiteExists.class) is missing a space
after the comma; update the invocation in ContentServiceImplTest (the
assertNotNull line referencing AnnotationUtils.getAnnotation,
RequireSiteExists.class and method) to add a space after the comma so it reads
AnnotationUtils.getAnnotation(method, RequireSiteExists.class) for consistent
formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cadc6aa-3ac2-4e79-91ae-fc9e7c734da1
📒 Files selected for processing (2)
src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.javasrc/test/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImplTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/BootstrapManagerImpl.java
sumerjabri
approved these changes
May 13, 2026
Draft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor bootstrap process to prevent site checks from blocking the startup
craftercms/craftercms#8652