Site exists API. Cleanup #3886
Site exists API. Cleanup #3886
Conversation
WalkthroughAdds GET /api/2/sites/{siteId}/exists; removes legacy getAllAvailableSites usages; replaces RepositoryCleanupJob with SitesService.garbageCollectRepositories(); migrates callers and wiring from v1 SiteService to v2 SitesService across controllers, services, repos, config, scripts, and tests. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@src/main/java/org/craftercms/studio/controller/rest/v2/UsersController.java`:
- Around line 521-523: The controller's deleteUserProperties method currently
accepts a potentially empty List properties which can cause the DAO's
deleteUserProperties to generate invalid SQL (property_key IN ()) in MyBatis;
either reapply validation by annotating the properties parameter with `@NotEmpty`
(e.g., change `@RequestParam` List<@NotBlank String> properties to List<@NotBlank
`@NotEmpty` String> properties) or add a guard in the service/controller before
invoking the DAO: check if properties.isEmpty() and return a successful no-op
Result or throw a validation error, ensuring the DAO.deleteUserProperties(...)
is never called with an empty collection.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java`:
- Around line 96-100: The internal call this.exists(siteId) inside
SitesServiceImpl.getSite(...) bypasses Spring AOP so the `@HasPermission` on
exists(...) won’t be applied on self-invocation; to fix, either annotate
getSite(...) with the same permission annotation (add `@HasPermission`(type =
DefaultPermission.class, action = PERMISSION_CONTENT_READ) to getSite) so
permission is enforced on every invocation, or change the code to call the
proxied bean (inject SitesService or ApplicationContext and call
sitesService.exists(siteId) instead of this.exists(siteId)) to ensure the AOP
interceptor runs; refer to getSite(), exists(), SitesServiceImpl,
`@HasPermission`, PERMISSION_CONTENT_READ and `@SiteId` when making the change.
🧹 Nitpick comments (5)
src/main/java/org/craftercms/studio/api/v2/service/site/SitesService.java (1)
217-221: Minor: Improve Javadoc grammar.The Javadoc comment has a grammatical issue. Consider rephrasing for clarity.
📝 Suggested Javadoc improvement
/** - * Git Garbage collect global repository all site repositories (sandbox and published) + * Run Git garbage collection on the global repository and all site repositories (sandbox and published) */ void garbageCollectRepositories();src/main/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImpl.java (2)
66-68: Inconsistent use of commons-lang vs commons-lang3.Line 66 imports
EMPTYfromorg.apache.commons.lang.StringUtils(commons-lang 2.x), while lines 67-68 useorg.apache.commons.lang3.StringUtils(commons-lang3). For consistency, prefer using commons-lang3 throughout.♻️ Suggested fix
-import static org.apache.commons.lang.StringUtils.EMPTY; +import static org.apache.commons.lang3.StringUtils.EMPTY;
556-568: Consider adding error handling for per-site garbage collection.The
garbageCollectRepositories()method iterates over all READY sites and callsgarbageCollectGitRepositories()for each. If GC fails for one site, the exception will propagate and subsequent sites won't be processed. Consider wrapping individual site GC calls in try-catch to ensure all sites are processed.♻️ Suggested improvement with error handling
`@Override` public void garbageCollectRepositories() { blobAwareRepository.garbageCollectGitRepositories(EMPTY); getSitesByState(READY).forEach(site -> { String siteId = site.getSiteId(); - blobAwareRepository.garbageCollectGitRepositories(siteId); + try { + blobAwareRepository.garbageCollectGitRepositories(siteId); + } catch (Exception e) { + logger.error("Failed to garbage collect repositories for site '{}'", siteId, e); + } }); }src/main/api/studio-api.yaml (1)
2034-2062: Add summary/operationId/description (and align security note).This endpoint is missing the usual OpenAPI metadata used throughout the spec. Please add at least a
summaryandoperationId, and align thedescription(andsecurityif applicable) with the actual permission requirements so the 401/403 responses remain consistent.♻️ Suggested doc metadata (adjust permission/security to match implementation)
/api/2/sites/{siteId}/exists: get: tags: - sites + summary: Check if a site exists + operationId: siteExists + description: Checks whether a site exists for the given siteId. parameters: - name: siteId in: path schema: type: string required: truesrc/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java (1)
44-44: Unused import:Site.State.READYis not referenced in this file.Consider removing this import unless it's intended for upcoming changes in this PR.
Suggested fix
-import static org.craftercms.studio.api.v2.dal.Site.State.READY;
src/main/java/org/craftercms/studio/controller/rest/v2/UsersController.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public boolean exists(String siteId) { | ||
| @HasPermission(type = DefaultPermission.class, action = PERMISSION_CONTENT_READ) | ||
| public boolean exists(@SiteId String siteId) { | ||
| return sitesServiceInternal.exists(siteId); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java | sed -n '90,140p'Repository: craftercms/studio
Length of output: 2273
🏁 Script executed:
#!/bin/bash
# Search for calls to getSite() to understand usage patterns
echo "=== getSite() method calls ==="
rg -n 'getSite\s*\(' --type java -C 1 | head -80
echo -e "\n=== getSite() method definitions ==="
rg -n 'public.*getSite\s*\(' --type java -A 3Repository: craftercms/studio
Length of output: 12646
🏁 Script executed:
#!/bin/bash
# Find the SitesService interface
find . -type f -name "*SitesService.java" | grep -v Impl | grep v2
# Show the interface
echo "=== SitesService interface ==="
cat -n $(find . -type f -name "SitesService.java" | grep v2 | grep -v Impl) | sed -n '1,150p'Repository: craftercms/studio
Length of output: 6127
🏁 Script executed:
cat -n src/main/java/org/craftercms/studio/api/v2/service/site/SitesService.java | sed -n '145,200p'Repository: craftercms/studio
Length of output: 2284
🏁 Script executed:
#!/bin/bash
# Check if there are REST endpoints or other public entry points calling getSite()
echo "=== Checking for REST controller methods calling getSite() ==="
rg -n 'GetMapping|PostMapping|PutMapping|DeleteMapping' --type java -A 10 | grep -B 10 'getSite' | head -50
echo -e "\n=== Check if other service methods calling getSite() have `@HasPermission` ==="
rg -B 5 '\.getSite\(' src/main/java --type java | grep -E '(`@HasPermission`|public|private)' | head -40Repository: craftercms/studio
Length of output: 13630
🏁 Script executed:
#!/bin/bash
# Check if SitesController has a REST endpoint for getSite
echo "=== SitesController methods ==="
rg -n 'GetMapping|PostMapping' src/main/java/org/craftercms/studio/controller/rest/v2/SitesController.java -A 5 | head -100
echo -e "\n=== Search for explicit getSite REST endpoint ==="
rg -n 'getSite|get-site' src/main/java/org/craftercms/studio/controller/rest/v2/SitesController.java -B 3 -A 3Repository: craftercms/studio
Length of output: 3091
🏁 Script executed:
#!/bin/bash
# Check if there are any patterns where service methods call other service's methods
echo "=== Check how getSite is used in service layer ==="
rg -n 'sitesService\.getSite|\.getSite\(siteId\)' src/main/java/org/craftercms/studio --type java -B 2 | head -80
echo -e "\n=== Check if getSite is used in REST controllers ==="
rg -n 'getSite' src/main/java/org/craftercms/studio/controller/rest/v2 --type javaRepository: craftercms/studio
Length of output: 16421
Permission enforcement on exists() is a good security addition.
The @HasPermission annotation with PERMISSION_CONTENT_READ and @SiteId properly secures the method. However, note that getSite() (lines 128-133) calls this.exists(siteId) internally. Due to Spring AOP's self-invocation limitation, the permission check will not be enforced on this internal call. While getSite() currently lacks a direct REST endpoint and is primarily used within the service layer where callers typically have their own permission validation, this could be a concern if getSite() becomes part of a public-facing API in the future. Consider adding explicit @HasPermission to getSite() if it should require permission enforcement at all invocation points, not just when called externally.
🤖 Prompt for AI Agents
In
`@src/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java`
around lines 96 - 100, The internal call this.exists(siteId) inside
SitesServiceImpl.getSite(...) bypasses Spring AOP so the `@HasPermission` on
exists(...) won’t be applied on self-invocation; to fix, either annotate
getSite(...) with the same permission annotation (add `@HasPermission`(type =
DefaultPermission.class, action = PERMISSION_CONTENT_READ) to getSite) so
permission is enforced on every invocation, or change the code to call the
proxied bean (inject SitesService or ApplicationContext and call
sitesService.exists(siteId) instead of this.exists(siteId)) to ensure the AOP
interceptor runs; refer to getSite(), exists(), SitesServiceImpl,
`@HasPermission`, PERMISSION_CONTENT_READ and `@SiteId` when making the change.
jmendeza
force-pushed
the
feature/6061
branch
from
599ab94 to
ec62170
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImpl.java`:
- Line 66: Replace the v2 StringUtils EMPTY import with the commons-lang3
version to match the other imports: change the static import of EMPTY from
org.apache.commons.lang.StringUtils to org.apache.commons.lang3.StringUtils so
it aligns with the existing static imports of isEmpty/isNotEmpty used in
SitesServiceInternalImpl and maintains consistency across the class.
♻️ Duplicate comments (1)
src/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java (1)
96-100: Permission check won’t apply to self‑invocation fromgetSite.
getSite()callsexists(siteId)within the same bean, so Spring AOP won’t enforce@HasPermissionon that internal call. IfgetSitecan be reached without its own permission checks, consider annotatinggetSiteor calling the proxied bean.Spring AOP self-invocation advice not applied to internal method calls?
🧹 Nitpick comments (1)
src/main/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImpl.java (1)
562-567: Isolate per-site GC failures so one site doesn’t stop the sweep.If a single repository GC throws, the remaining sites won’t be processed. Consider logging and continuing per-site.
♻️ Suggested update
public void garbageCollectRepositories() { blobAwareRepository.garbageCollectGitRepositories(EMPTY); getSitesByState(READY).forEach(site -> { String siteId = site.getSiteId(); - blobAwareRepository.garbageCollectGitRepositories(siteId); + try { + blobAwareRepository.garbageCollectGitRepositories(siteId); + } catch (Exception e) { + logger.warn("Failed to garbage collect repository for site '{}'", siteId, e); + } }); }
src/main/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImpl.java
Outdated
Show resolved
Hide resolved
jmendeza
force-pushed
the
feature/6061
branch
from
ec62170 to
9b3ec34
Compare
…ed to the type param
jmendeza
force-pushed
the
feature/6061
branch
from
9b3ec34 to
1892c50
Compare
1 participant
Site exists API. Cleanup
craftercms/craftercms#6061
Summary by CodeRabbit
New Features
Improvements
Refactor
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.