APIs cleanup#3914
Open
jmendeza wants to merge 39 commits intocraftercms:developfrom
Open
Conversation
jmendeza
force-pushed
the
feature/6061-content-types-cleanup
branch
from
a2be4c8 to
fd42c1b
Compare
|
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:
WalkthroughThis PR executes a comprehensive v1-to-v2 API migration, removing legacy v1 service interfaces, implementations, and data models while introducing a new v2-centric schema and service layer. Key changes include: updating OpenAPI schema to reflect structured ContentType with dependencies and metadata, adding new v2 ContentTypeService methods and ContentType model, removing deprecated v1 DAL classes and utilities, rewiring Spring beans from v1 to v2 services, migrating MyBatis mappers away from v1 patterns, and updating implementation code to use new v2 APIs throughout. ChangesV2 Content Type Model & Service Layer
V1→V2 Implementation Migration & V1 API Cleanup
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/resources/org/craftercms/studio/api/v1/dal/SiteFeedMapper.xml (1)
58-65:⚠️ Potential issue | 🟠 MajorGuard
updateSiteagainst “all null” updates (possible invalid SQL).Right now,
updateSiteonly conditionally setsnameanddescription. If both arenull, the<set>can become empty, which may produce invalid SQL likeUPDATE site SET WHERE site_id = ....Please ensure callers never invoke
updateSitewhen both fields are null (or split into separate mapper methods), and/or add a defensive guard upstream in the service layer.Suggested direction (upstream/service validation)
- siteFeedMapper.updateSite(siteId, name, description); + if (name == null && description == null) { + return; // or throw IllegalArgumentException + } + siteFeedMapper.updateSite(siteId, name, description);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/org/craftercms/studio/api/v1/dal/SiteFeedMapper.xml` around lines 58 - 65, The mapper updateSite can produce invalid SQL if both name and description are null; add an explicit guard in the service layer before calling the mapper (e.g., in the SiteService method that invokes updateSite) to validate input and return or throw a clear error (IllegalArgumentException/BadRequest) when both fields are null; ensure the check references the same parameters passed to updateSite (name and description) and short-circuit the call so updateSite is never invoked with both nulls.src/main/java/org/craftercms/studio/impl/v1/service/security/SecurityServiceImpl.java (1)
84-101:⚠️ Potential issue | 🔴 CriticalRestore content-type
allowed-rolesenforcement ingetUserPermissions().
StudioAbstractAccessDecisionVoter.hasPermission()still delegates to this method, but this implementation now only merges the site/global permission maps. That drops the content-type-level deny for/site/...items, so a user with generic path access can reach content whose form-definition explicitly excludes their role. Based on learnings,allowed-rolesis a supported content-type restriction that now lives inform-definition.xmlafter the content-type merge upgrade.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/impl/v1/service/security/SecurityServiceImpl.java` around lines 84 - 101, The merged permissions logic in getUserPermissions(String site, String path, String user) drops content-type-level allowed-roles checks; restore enforcement by checking the content-type's form-definition allowed-roles for site paths and filtering/denying permissions accordingly. Specifically, after building the site roles (addUserRoles) and before returning merged permissions, load the content-type/form-definition for the given site+path (use the same resolution logic you have for form-definition.xml), read its allowed-roles, and if allowed-roles is present restrict the computed permissions (from populateUserPermissions and populateUserGlobalPermissions) to only those roles (or return empty set when the user's roles are not allowed). Ensure this integrates with PermissionsConfigTO flow and does not bypass populateUserPermissions/populateUserGlobalPermissions results for /site/... items.src/main/api/studio-api.yaml (1)
3992-4019:⚠️ Potential issue | 🔴 CriticalFix the DELETE operation object: replace invalid
paramswithparametersand remove stalesiteIdrequirement from request body.The operation uses
params:(line 3998), which is not a valid OpenAPI field. Tooling will discard it, leaving thesiteIdpath parameter undocumented for generators. Additionally, the request body'srequiredlist (line 4017) still includessiteIddespite the parameter moving to the path, creating inconsistency between the spec and the endpoint contract.Suggested fix
delete: tags: - contentTypes summary: Delete files related to a given content-type description: 'Required permission "write_configuration"' operationId: deleteContentType - params: + parameters: - name: siteId in: path description: site ID required: true schema: type: string requestBody: + required: true content: application/json: schema: type: object properties: contentType: type: string description: The content-type to delete deleteDependencies: type: boolean description: Indicates if all dependencies of the content-type should be deleted (defaults to false) required: - - siteId - contentType🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 3992 - 4019, Replace the invalid OpenAPI field "params" with "parameters" for the deleteContentType operation (operationId: deleteContentType) and define the path parameter there (name: siteId, in: path, required: true, schema: type: string); also remove "siteId" from the requestBody.required array so the request body only requires contentType (and optionally deleteDependencies) to keep the path parameter and body requirements consistent.
🧹 Nitpick comments (25)
src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/form-definition.xml (2)
417-423: Confirm correctness of new top-level content-type settings (previewable/noThumbnail/paths includes)The PR adds/updates these top-level fields at the end of the form definition:
previewable=false(Line 417)noThumbnail=false(Line 418)paths/includes/pattern=^/site/components/contacts/.*(Lines 419-423)This could affect:
- how the form is presented in the Studio UI (preview/thumb behavior)
- where instances are allowed to live / be discovered (path restriction)
- compatibility with the content-type v2 schema (expected tag names/locations/structure)
Please verify that
^/site/components/contacts/.*is the intended storage/discovery path pattern for this specific content type (/component/contact-widget) and that it won’t unintentionally exclude existing content or other expected component placement paths.If there’s a schema/XSD or a schema validator for
form-definition.xml, run it against the updated file (and ideally compare with the prior serialized output). Also consider validating in Studio by attempting to create/edit a contact-widget component from the expected site structure.If
pathsis intended to be centrally managed by the upgrader/blueprint model (rather than per content-type), double-check whether adding it here duplicates or conflicts with the new v2 content-type configuration model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/form-definition.xml` around lines 417 - 423, Verify and, if necessary, correct the new top-level fields added to the contact-widget form-definition: confirm that previewable="false" and noThumbnail="false" are the intended UI behaviors for the /component/contact-widget content type and adjust them if Studio should allow preview/thumbs; validate that the paths/includes/pattern value "^/site/components/contacts/.*" exactly matches all intended storage/discovery locations for contact-widget instances (update the regex to include other expected paths or remove the per-type paths block if path management is centralized by the upgrader/blueprint model); run the form-definition XSD/schema validator against the file and test creating/editing a contact-widget in Studio to ensure no schema or runtime registration conflicts.
17-424: Potential semantic drift from normalizing empty XML elementsThis file has widespread normalization of empty values (e.g.,
<description/>,<value/>,<iceId/>,<quickCreatePath/>,<constraints/>, empty<value/>nodes inside constraints/properties). Depending on how Crafter Studio parses/validatesform-definition.xml, “present but empty” can be treated differently from “missing” (e.g., “no constraint” vs “constraint with empty regex”, or “unset” vs “set to empty string”).Please verify (or add/adjust tests) that the new XML serialization is semantically equivalent to the previous one for:
- constraint
patternfields (many<value/>are empty)- property
defaultValue/help/readonly/maxlength-related empty nodes- datasource definitions and any consumers that interpret empty values
To validate quickly in-repo, I’d suggest a targeted diff-driven check: locate the previous version of this same file (on the PR branch) and confirm there were no changes in element presence (only formatting/self-closing form). If presence changed, it’s important to validate behavior in the content-type UI and any form rendering logic.
If the formatter is purely re-serializing, consider adding a unit/integration test that loads this
form-definition.xmland asserts that the produced form model matches the previous model (or at least key fields/constraints).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/form-definition.xml` around lines 17 - 424, The PR normalized empty XML elements which can change semantics—verify and ensure element presence vs self-closing empty nodes is preserved for constraint pattern fields and property defaults; specifically check constraint pattern/value entries, field ids title_t, email_s, phone_s, file-name/internal-name fields' defaultValue/help/readonly/maxlength properties, quickCreatePath, iceId, and datasources uploadImages/existingImages. Fix by diff-checking this file's previous version on the branch and either restore missing elements or adjust the XML serializer to preserve absent vs empty elements, and add a unit/integration test that loads the previous and current form-definition.xml and asserts the parsed form model (constraints.pattern, property.defaultValue/help/readonly/maxlength, datasource repoPath/useSearch, etc.) are equivalent. If any element presence changed, revert that change or update consumers to handle empty vs missing consistently.src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/form-definition.xml (2)
333-335: Confirm placement expectations for<previewable>and<noThumbnail>You’ve added/normalized these top-level elements after
<datasources>. That’s likely correct, but if the content-type form-definition parser expects them in a particular order (or expects them to be present only when certain other elements exist), ordering/placement could matter.Given this PR also includes content-type config upgrades, double-check that the upgrader/loader expects these elements at this level and position.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/form-definition.xml` around lines 333 - 335, The added top-level elements previewable and noThumbnail are placed after the datasources element which may violate the form-definition parser or upgrader/loader's expected element order; verify the content-type loader/upgrader accepts these elements at the top level and in this position, and if it does not, move previewable and noThumbnail to the parser-expected location (or update the upgrader/loader to accept them here). Inspect the form-definition parser and any upgrade routines that parse form-definition.xml for references to datasources, previewable, or noThumbnail and either adapt the XML order to match those routines or update the parsing/upgrade logic to handle the new placement consistently.
17-281: Formatting normalization is ok, but confirm schema compatibility for self-closing empties.This file appears to have been rewritten to normalize empty elements into self-closing tags (e.g.,
<description/>,<iceId/>,<defaultValue/>,<properties/>,<constraints/>,<quickCreatePath/>). If any downstream loader treats “empty element vs missing element” differently (some XML parsers/schemas can), this could affect behavior.Please confirm that Crafter Studio’s content-type form-definition parser treats these consistently (self-closing vs explicitly-present empty tags).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/form-definition.xml` around lines 17 - 281, The diff normalized empty XML elements to self-closing tags (e.g., <description/>, <iceId/>, <defaultValue/>, <properties/>, <constraints/>, <quickCreatePath/>) which may be treated differently by Crafter Studio’s form-definition parser; validate compatibility by loading this form-definition (content-type /component/feature, title "Feature") into Crafter Studio and running the form-definition parser or creating a test component to ensure behavior is unchanged, and if the parser treats self-closing empties differently, revert only the problematic elements back to explicit empty elements (e.g., <description></description>) for fields like file-name, internal-name, disabled, icon_s, title_t, body_html and the top-level quickCreatePath so the parser recognizes them correctly.src/main/java/org/craftercms/studio/impl/v2/utils/Wrapper.java (1)
25-37: Track assignment state separately from the wrapped value.
hasValue()currently means “non-null”, soset(null)is indistinguishable from “never set”. For a shared generic holder, that contract is surprising and easy to misuse with nullable payloads. Either record an explicitsetflag or makeset(...)reject nulls.Possible fix
public class Wrapper<T> { private T value; + private boolean set; public T get() { return value; } public void set(T value) { this.value = value; + this.set = true; } public boolean hasValue() { - return this.value != null; + return set; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/impl/v2/utils/Wrapper.java` around lines 25 - 37, Wrapper<T>'s hasValue() currently checks for non-null which makes set(null) indistinguishable from "never set"; update the class to track assignment state explicitly: add a private boolean field (e.g., isSet), set isSet = true inside set(T value) (even when value is null), change hasValue() to return isSet, and leave get() to return the stored value; optionally add a clear() or unset() method to reset isSet and value if needed. Ensure you update references to Wrapper.hasValue(), set(T), and get() accordingly.src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/form-definition.xml (1)
432-439: Sanity-check new v2 content-type metadata (preview/noThumbnail/paths)Lines 432-439 introduce:
previewable=falsenoThumbnail=truepaths/includespattern/site/items/companies/.*This looks aligned with the v2 config model, but please double-check the business intent:
- If
noThumbnail=true, then the thumbnail resolution path should not be used; combined with theimageThumbnailvalue (above), this further suggestsimageThumbnailshould not beundefined.- Confirm
/site/items/companies/.*matches where company items are actually stored for this blueprint.Check existing company content storage conventions in this blueprint family (or similar content types) to ensure the include regex is correct and doesn’t accidentally exclude legitimate items.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/form-definition.xml` around lines 432 - 439, Check and reconcile the new v2 metadata: verify that previewable="false" and noThumbnail="true" are intended together and, if so, set or remove the imageThumbnail field referenced earlier (ensure imageThumbnail is not left undefined) in the same form-definition.xml (search for imageThumbnail) so thumbnail resolution won’t be invoked; also confirm the paths/includes pattern `/site/items/companies/.*` matches actual company item storage used by this blueprint family (compare against other company content-types or siblings and update the pattern to the correct storage path if they use a different prefix or folder structure).src/main/java/org/craftercms/studio/model/contentType/DeleteDependency.java (1)
19-21: Potential unused import:JsonPropertyLine [linenumber 20] imports
com.fasterxml.jackson.annotation.JsonPropertybut the shown record uses@JsonAliasonly. IfJsonPropertyis unused in the actual file, this could fail build if the project enforces unused-import checks.Confirm whether
JsonPropertyis used elsewhere in this file (or was removed with the change). If unused, remove the import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/model/contentType/DeleteDependency.java` around lines 19 - 21, The import com.fasterxml.jackson.annotation.JsonProperty appears unused in the DeleteDependency record; inspect the DeleteDependency type and surrounding code to confirm JsonProperty is not referenced anywhere in this file, and if it's not used remove the JsonProperty import to avoid unused-import build failures; if the intention was to annotate a field or constructor parameter, instead add the appropriate `@JsonProperty` annotations to the specific component (e.g., in the DeleteDependency record declaration) rather than keeping an unused import.src/main/webapp/default-site/scripts/pages/login.groovy (2)
34-38: User-facing message: consider sanitizing / using a stable error textLine 37-38 sets
model.errorMessage = lockedException.message. Depending on howLockedExceptionis populated, this could surface internal wording.Confirm
LockedException.messageis safe and stable for UI display across environments (especially if it can include usernames or other internal details).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/default-site/scripts/pages/login.groovy` around lines 34 - 38, Replace direct use of LockedException.message in the UI: do not assign lockedException.message to model.errorMessage; instead set a sanitized, stable user-facing string (e.g., "Your account is locked. Try again later.") and avoid exposing internal details or usernames in the message; continue to use loginAttemptManager.getUserLockTimeLeftSeconds(lockedException.username) for the lock countdown but ensure the username is not rendered in the UI. Update the code paths referencing model.errorMessage, LockedException, lockedException.username, and loginAttemptManager.getUserLockTimeLeftSeconds accordingly so the UI shows a generic, safe message while preserving the lockedTimeSeconds behavior.
24-38: Add defensive handling for potentially-missing beans / propertiesLines 24-27 and 29-38 assume:
applicationContext.get("studioConfiguration")returns a non-null object with the propertySECURITY_PASSWORD_REQUIREMENTS_MINIMUM_COMPLEXITYapplicationContext.get('loginAttemptManager')is non-null whenparams.erroris present and the exception isLockedExceptionIf either bean/property is missing in this script context, you’ll get a hard NPE and the login page may break.
At minimum, consider failing gracefully (e.g., default complexity to a safe value, or skip lockedTimeSeconds if loginAttemptManager is null).
Confirm these beans are always present for the
default-sitelogin page script execution path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/default-site/scripts/pages/login.groovy` around lines 24 - 38, The current code assumes applicationContext.get("studioConfiguration") and applicationContext.get('loginAttemptManager') are non-null and that studioConfigurationSB.getProperty(SECURITY_PASSWORD_REQUIREMENTS_MINIMUM_COMPLEXITY) returns a value; add defensive checks to avoid NPEs: null-check studioConfigurationSB before calling getProperty and fallback to a safe default integer (e.g., 0 or configured default) for passwordRequirementsMinComplexity, and null-check loginAttemptManager before calling getUserLockTimeLeftSeconds so you only set model.lockedTimeSeconds when loginAttemptManager is present; keep the existing logic around params.error and LockedException but guard accesses to lockedException.username with these presence checks so the login page fails gracefully if beans/properties are missing.src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/form-definition.xml (1)
291-298: Sanity-check preview vs thumbnail behaviorLines 291-298 set:
previewable=falsenoThumbnail=falsepaths/includesregex^/site/components/articles-widget/.*This is likely fine, but please confirm the intended UX: disabling preview while still allowing thumbnails can be valid, but if previews are disabled specifically because thumbnails/preview rendering are broken, you may want
noThumbnail=trueas well.Confirm this matches how other component content-types in the same blueprint family configure preview/noThumbnail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/form-definition.xml` around lines 291 - 298, The config sets previewable=false but noThumbnail=false for the Articles Widget (keys: previewable, noThumbnail, and the paths/includes pattern ^/site/components/articles-widget/.*); verify other component content-types in this blueprint family and make them consistent—either keep previews disabled and explicitly set noThumbnail=true if thumbnails should also be disabled, or set previewable=true/noThumbnail=false if thumbnails/previews should be available; update the previewable and/or noThumbnail flags on the Articles Widget to match the established pattern used by other components.src/main/webapp/default-site/scripts/api/ContentServices.groovy (1)
24-73: Consider leaving a deprecatedgetContentTypeshim for one cycle.Removing the helper outright turns older Groovy callers into
MissingMethodExceptionat runtime. A stub that fails with a targeted migration message is much easier to diagnose during upgrades.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/default-site/scripts/api/ContentServices.groovy` around lines 24 - 73, Add a deprecated shim method named getContentType to the ContentServices class that preserves runtime compatibility: implement a static getContentType(...) that either delegates to ServiceFactory.getContentServices(context).getContentType(...) if that implementation exists, or immediately throws a clear, targeted exception (e.g., UnsupportedOperationException/IllegalStateException) with a migration message pointing callers to the new API (mention ServiceFactory.getContentServices(context) and the replacement method such as getContentItemTree or the equivalent new method). Ensure the stub is annotated/marked deprecated and the exception message explicitly instructs how to migrate.src/main/java/org/craftercms/studio/model/contentType/PathIncludeExcludes.java (1)
27-29: Optional hardening: ensureincludes/excludesare never null.
PathIncludeExcludesstoresCollection<PathPattern>fields (Line 27-28). If deserialization or construction can producenullfor either collection, downstream iteration may NPE.Optional improvement: add a compact constructor to default nulls to empty collections (or enforce non-null via
Objects.requireNonNulldepending on desired semantics).Add null-safety in the record to make the model more robust and easier to consume.
If you want, I can draft the exact compact constructor change consistent with your project’s preferred null-handling patterns.
Verify whether your JSON/XML mapper or service layer guarantees non-null collections for these fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/model/contentType/PathIncludeExcludes.java` around lines 27 - 29, PathIncludeExcludes may receive null for includes/excludes during deserialization; add a compact constructor on the PathIncludeExcludes record to harden null-safety by either calling Objects.requireNonNull on includes and excludes (to enforce non-null) or by defaulting null values to Collections.emptyList() (to allow empty collections), and update any callers if you choose the requireNonNull approach; ensure you reference the record name PathIncludeExcludes and its components includes and excludes when making the change and verify whether your JSON/XML mapper already guarantees non-null collections before committing the choice.src/main/java/org/craftercms/studio/model/contentType/CopyDependency.java (1)
25-27: Optional: validate/guardpatternandtargetinCopyDependency.The record introduces
String patternandString target(Line 25-27) without any null/empty enforcement. If any downstream logic assumes non-null/non-empty values, this can become a runtime failure later.Consider a compact constructor with
requireNonNull(and optionally reject blank strings) to enforce invariants close to the model boundary.Confirm whether OpenAPI schema + controller validation already guarantees these fields are present and non-empty.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/model/contentType/CopyDependency.java` around lines 25 - 27, Add a compact constructor to the CopyDependency record that enforces invariants for pattern and target: use Objects.requireNonNull for both fields and optionally reject blank strings (e.g., trim and check isEmpty) to throw IllegalArgumentException for invalid values; update the constructor in CopyDependency to perform these checks so downstream code can rely on non-null/non-empty values. Also verify whether the OpenAPI schema and controller-layer validation already guarantee presence/non-emptiness of pattern and target—if they do, keep the checks defensive but consider aligning error types/messages with controller validation.src/main/resources/crafter/studio/upgrade/pipelines.yaml (1)
766-773: Confirm the merge operation is safe and targets only form-definition.xml files you expect.The added
contentTypeConfigMergeUpgraderstep (Line 769-772) looks correct for scoping changes toconfig/studio/content-types/**/form-definition.xml, and the referenced template (content-type-merge-v5.0.0.2.xslt) should be safe because it merges the intended elements from a siblingconfig.xmlinto eachform-definition.xmland then deletesconfig.xmlright after the merge (so re-running shouldn’t re-merge those elements). citeturn0search0That said, please double-check:
- The regex really matches the on-disk path format used by all supported installations (no extra directory segments).
- The upgrader won’t fail/partially apply when
config.xmlis missing for a given content type (if such states can exist).Consider adding/confirming an upgrade test case for “already upgraded” sites to ensure no regressions in repeated upgrade runs.
If you can, verify the template’s actual merge/delete logic in the referenced XSLT and the implementation expectations of
contentTypeConfigMergeUpgrader(e.g., what it does when the siblingconfig.xmlis absent).
(As per learnings: content-type-merge-v5.0.0.2.xslt deletes config.xml after merge; idempotency is non-concern in that design.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/crafter/studio/upgrade/pipelines.yaml` around lines 766 - 773, Confirm and harden the contentTypeConfigMergeUpgrader step: verify the regex "config/studio/content-types/.+/form-definition\.xml" exactly matches your on-disk paths (no extra segments) and tighten it if needed; update or guard the contentTypeConfigMergeUpgrader implementation to safely skip or no-op when the sibling config.xml is absent (so it never throws or partially applies changes); add an automated upgrade test covering an "already upgraded" site (form-definition.xml present but no config.xml) to ensure idempotency; and inspect the referenced template content-type-merge-v5.0.0.2.xslt to confirm it performs the merge then deletes config.xml as expected.src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/form-definition.xml (1)
32-37: Watch for semantic drift in booleans + validate the new path include regex.
- The
no-template-requiredproperty is declared as<type>boolean</type>(Line 36) but its<value/>is empty (Line 35). Please confirm the form engine treats an empty value as the intended default (e.g.,false) rather than erroring or treating it as “unset”.- The new v2
pathsconfiguration for this content type uses an include regex^/site/website/(?!articles/)(.*)(Line 548-549). Please double-check that this matches exactly the intended URL space forpage/category-landingand doesn’t unintentionally include/exclude paths you rely on.If there’s any ambiguity in boolean handling, prefer explicitly setting boolean defaults (e.g.,
false) in these blueprints rather than leaving<value/>empty.Run/verify blueprint validation or an automated content-type configuration load test to ensure these fields parse as expected with the v2 model/controller changes.
Also applies to: 544-550
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/form-definition.xml` around lines 32 - 37, The boolean property "no-template-required" currently has an empty <value/> which may be treated as unset; set an explicit default (e.g., false) in the property definition to avoid semantic drift and ensure the form engine treats it as a boolean (update the <property> for no-template-required to include an explicit false value). Also re-check and, if needed, tighten the include regex ^/site/website/(?!articles/)(.*) used in the v2 paths for the page/category-landing content type to confirm it only matches the intended URL space (adjust the pattern to explicitly anchor or constrain segments if necessary), then run the blueprint validation/content-type load test to verify parsing under the v2 model/controller.src/main/resources/crafter/studio/upgrade/5.0.x/content-type/content-type-merge-v5.0.0.2.xslt (1)
22-52: Minor robustness: consider bindingdocument($configFileName)once + verify path/URI resolution.Right now each
<xsl:copy-of ...>callsdocument($configFileName)again. It should be fine, but binding once improves clarity and avoids repeated lookups. Also, ensure the upgrader passes a value that the XSLT processor can resolve withdocument()(URI vs filesystem path vs classpath).Example refactor:
Proposed refactor
<xsl:template match="form"> <xsl:copy> <xsl:apply-templates select="node() | @*"/> - <xsl:copy-of select="document($configFileName)/content-type/previewable"/> - <xsl:copy-of select="document($configFileName)/content-type/noThumbnail"/> - <xsl:copy-of select="document($configFileName)/content-type/paths"/> - <xsl:copy-of select="document($configFileName)/content-type/delete-dependencies"/> - <xsl:copy-of select="document($configFileName)/content-type/copy-dependencies"/> - <xsl:copy-of select="document($configFileName)/content-type/allowed-roles"/> + <xsl:variable name="cfg" select="document($configFileName)"/> + <xsl:copy-of select="$cfg/content-type/previewable"/> + <xsl:copy-of select="$cfg/content-type/noThumbnail"/> + <xsl:copy-of select="$cfg/content-type/paths"/> + <xsl:copy-of select="$cfg/content-type/delete-dependencies"/> + <xsl:copy-of select="$cfg/content-type/copy-dependencies"/> + <xsl:copy-of select="$cfg/content-type/allowed-roles"/> </xsl:copy> </xsl:template>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/crafter/studio/upgrade/5.0.x/content-type/content-type-merge-v5.0.0.2.xslt` around lines 22 - 52, The repeated calls to document($configFileName) inside the template matching form are inefficient and brittle; bind document($configFileName) once to a variable (e.g., xsl:variable name="cfg" select="document($configFileName)"/> placed in the template matching "form" or at top-level) and replace each xsl:copy-of select="document($configFileName)/content-type/..." with xsl:copy-of select="$cfg/content-type/..." to avoid repeated lookups, and add a simple existence check (e.g., test="$cfg" or test="$cfg/content-type") before copying to handle unresolved URIs; also confirm the upgrader supplies a document()-resolvable URI (filesystem vs classpath) when invoking this XSLT.src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/form-definition.xml (2)
525-532: Verify semantics of<value/>in boolean constraints/properties.This file includes many boolean
constraint/propertyentries where<value/>is used (empty element). If the form-definition parser interprets empty differently thanfalse/trueexplicitly, this could change validation behavior (e.g., “required” constraints, “readonly” flags).At minimum, please spot-check any entries that look like they might have lost
true/falseduring normalization:
- constraints that are labeled
required- properties labeled
readonly,disableFlattening, etc.- the datasource
dataTypevalue (quote escaping/JSON parsing)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/form-definition.xml` around lines 525 - 532, Several boolean constraint/property entries use empty <value/> elements which may be interpreted differently than explicit true/false; inspect and replace empty <value/> nodes for boolean flags such as constraint name="required", property name="readonly", property name="disableFlattening", and any datasource entries (e.g., dataType) so they contain explicit "true" or "false" text as appropriate; spot-check each form-definition block that contains <constraint> or <property> elements and ensure JSON/quote-escaped datasource values are valid strings rather than empty nodes to preserve intended validation/behavior.
527-531: Path include regex should match intended component paths.
<pattern>^/site/components/.*</pattern>should align with where this component form is expected to apply (likely/site/components/**). If component instances can live elsewhere (alternate roots, additional folder levels), users might not see the form where expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/form-definition.xml` around lines 527 - 531, The current <pattern> value '^/site/components/.*' in the paths/includes block is too strict and may miss component instances in nested folders or alternate roots; update the pattern in form-definition.xml to a regex that matches the intended component tree such as '^/site/components(/.*)?$' (or add additional <pattern> entries for alternate roots like '/project/site/components') so the social-media-widget form applies to all nested component paths; adjust the <pattern> element under <paths><includes> accordingly in the file.src/main/webapp/default-site/scripts/pages/preview.groovy (1)
22-24: Minor: session-derived values can be null — are templates handling that?
ticketsandusernamecome fromrequest.getSession().getValue(...). If those are optional, ensure the view layer tolerates null/empty. Not necessarily a bug, but worth a quick check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/default-site/scripts/pages/preview.groovy` around lines 22 - 24, The session-derived variables ticket and username fetched via request.getSession().getValue(...) in preview.groovy can be null; update the code that sets or passes these values (variables ticket and username) to either provide safe defaults (e.g., empty string) or add null checks before rendering so the view templates tolerate missing values—locate where ticket and username are used/passed to the view in preview.groovy and ensure you replace direct uses with null-safe logic (e.g., conditional rendering or defaulting) so templates won't break when request.getSession().getValue(...) returns null.src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/form-definition.xml (1)
1-232: Verify the new<paths><includes>pattern matches real content paths.The addition of:
<paths><includes><pattern>^/site/website/.*</pattern></includes></paths>is likely intentional for the v2 content-type model, but it’s worth confirming this level-descriptor form is only meant to apply under
/site/website/**in this blueprint. If the actual content lives under a different root (e.g., other site ids or additional folder layouts), the form could fail to appear where expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/form-definition.xml` around lines 1 - 232, The form for content-type '/component/level-descriptor' currently restricts applicability via the paths include pattern '^/site/website/.*' in the <paths><includes> block; confirm where the actual level-descriptor content lives and update the pattern(s) accordingly (for example add additional <pattern> entries or broaden to match other site roots such as '^/site/[^/]+/website/.*' or '^/site/.*') so the form appears for the intended content; search the repo for existing level-descriptor instances and adjust the <paths><includes><pattern> for this form-definition.xml to cover those real paths.src/main/java/org/craftercms/studio/model/contentType/PathPattern.java (1)
24-34: Consider addingequals,hashCode, andtoString.
PathPatternis a value object and is intended to live inside collections (per the class doc andPathIncludeExcludes). Withoutequals/hashCode, collection operations likecontains, dedup, and test assertions fall back to identity semantics, which is rarely the desired behavior for a config DTO.♻️ Proposed addition
public class PathPattern { private final String pattern; public PathPattern(String pattern) { this.pattern = pattern; } public String getPattern() { return pattern; } + + `@Override` + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof PathPattern other)) return false; + return java.util.Objects.equals(pattern, other.pattern); + } + + `@Override` + public int hashCode() { + return java.util.Objects.hashCode(pattern); + } + + `@Override` + public String toString() { + return "PathPattern{pattern='" + pattern + "'}"; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/model/contentType/PathPattern.java` around lines 24 - 34, PathPattern is a value object but lacks equals/hashCode/toString which breaks value-based collection semantics; implement equals(Object) comparing the pattern field, implement hashCode() derived from pattern (Objects.hash or pattern.hashCode with null-safety), and add a readable toString() that includes the pattern; update methods in the PathPattern class (constructor PathPattern(String) and getPattern()) to keep behavior unchanged and ensure equals/hashCode handle null pattern correctly so collections like contains/dedup and test assertions work as intended.src/main/webapp/default-site/scripts/libs/EnvironmentOverrides.groovy (2)
77-77: Reuse the existinguserServiceSBreference.
userServiceSBwas already resolved at line 52; callingappContext.get("userService")again is redundant and adds an extra bean lookup on the hot path of every request that produces full site values.♻️ Proposed change
- def sites = appContext.get("userService").getCurrentUserSites() + def sites = userServiceSB.getCurrentUserSites()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/default-site/scripts/libs/EnvironmentOverrides.groovy` at line 77, The code redundantly calls appContext.get("userService").getCurrentUserSites() instead of reusing the already-resolved userServiceSB; replace that bean lookup by calling userServiceSB.getCurrentUserSites() when assigning sites (the variable defined as def sites), so the method invocation uses the existing userServiceSB reference to avoid the extra bean lookup on the hot path.
140-141: Remove the commented-out line.Line 140 leaves dead code from the legacy
SiteServicesflow. Since this PR removes that abstraction, the comment should go too.♻️ Proposed cleanup
static getMinimalValuesForSite(appContext, request) { def result = [:] -// def context = SiteServices.createContext(appContext, request) def studioConfigurationSB = appContext.get("studioConfiguration") result.useBaseDomain = studioConfigurationSB.getProperty(STUDIO_COOKIE_USE_BASE_DOMAIN) return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/default-site/scripts/libs/EnvironmentOverrides.groovy` around lines 140 - 141, Remove the dead commented code referencing SiteServices.createContext: delete the commented line "// def context = SiteServices.createContext(appContext, request)" from EnvironmentOverrides.groovy so only the active variable retrieval (def studioConfigurationSB = appContext.get("studioConfiguration")) remains; this cleans up the legacy SiteServices artifact and keeps the file focused on the current flow.src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgrader.java (1)
72-86: Avoid recomputing the sibling config path.
Path.of(formDefinitionPath).resolveSibling(configFileName)is built twice (once inexecuteTemplateto track for deletion, once ingetTemplateParametersto pass to the XSLT). Compute it once and reuse, e.g. via a small helper. Otherwise the two sites can drift in subtle ways (e.g. if one becomes relative and the other absolute).♻️ Proposed refactor
`@Override` protected void executeTemplate(StudioUpgradeContext context, String formDefinitionPath, OutputStream os) throws UpgradeException { logger.info("Executing config merge template for form definition: {}", formDefinitionPath); super.executeTemplate(context, formDefinitionPath, os); - Path configFilePath = Path.of(formDefinitionPath).resolveSibling(configFileName); - trackDeletedFiles(configFilePath.toString()); + trackDeletedFiles(siblingConfigPath(formDefinitionPath).toString()); } `@Override` protected Map<String, Object> getTemplateParameters(StudioUpgradeContext context, String formDefinitionPath) { Map<String, Object> params = new HashMap<>(super.getTemplateParameters(context, formDefinitionPath)); - params.put(CONFIG_FILE_PARAM, Path.of(formDefinitionPath).resolveSibling(configFileName)); + params.put(CONFIG_FILE_PARAM, siblingConfigPath(formDefinitionPath)); return params; } + + private Path siblingConfigPath(String formDefinitionPath) { + return Path.of(formDefinitionPath).resolveSibling(configFileName); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgrader.java` around lines 72 - 86, Compute the sibling config path once via a small private helper and reuse it in both executeTemplate and getTemplateParameters: add a private method like resolveConfigPath(String formDefinitionPath) that returns Path.of(formDefinitionPath).resolveSibling(configFileName), replace the two inline Path.of(...).resolveSibling(...) occurrences in executeTemplate (where you call trackDeletedFiles) and in getTemplateParameters (where you put CONFIG_FILE_PARAM) to call resolveConfigPath(formDefinitionPath) instead so both sites use the exact same computed Path.src/main/java/org/craftercms/studio/api/v2/service/content/ContentTypeService.java (1)
116-122: Javadoc/throws inconsistency and possibly unused import.
getAllContentTypesdocuments@throws SiteNotFoundExceptionbut only declaresthrows ServiceLayerException. SinceSiteNotFoundExceptionis a subtype that's technically valid, but for clarity either declareSiteNotFoundExceptionexplicitly or document@throws ServiceLayerExceptioninstead.getContentTypeandgetAllowedContentTypeshave no@throwsdocumentation at all — please add for consistency with the rest of the interface.org.dom4j.DocumentException(line 26) appears unused in this file; consider removing it (and similarly inContentTypeServiceImpl.javaif also unused there).Also applies to: 26-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/api/v2/service/content/ContentTypeService.java` around lines 116 - 122, Update the Javadoc and throws declarations for clarity and remove the unused import: in ContentTypeService, ensure the Javadoc for getAllContentTypes matches its signature by either adding `@throws` ServiceLayerException instead of `@throws` SiteNotFoundException or by declaring throws SiteNotFoundException on the method; add appropriate `@throws` ServiceLayerException (and/or SiteNotFoundException if used) to the Javadoc for getContentType and getAllowedContentTypes to be consistent with the other interface methods; and remove the unused import org.dom4j.DocumentException from this file (and from ContentTypeServiceImpl.java if it is unused there) to clean up imports.
jmendeza
force-pushed
the
feature/6061-content-types-cleanup
branch
from
fd42c1b to
d51af77
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/api/studio-api.yaml (1)
3992-4019:⚠️ Potential issue | 🔴 CriticalFix the delete operation contract.
Line 3998 uses
params, which is not a valid OpenAPI 3.0 Operation Object field—must useparameters. Additionally, line 4018 incorrectly requiressiteIdin the JSON body, butContentTypeController.deleteContentType()receivessiteIdfrom the path variable (@PathVariable String siteId) and the request body only carriescontentTypeanddeleteDependencies(per theDeleteContentTypeRequestmodel). This divergence from the actual endpoint breaks OpenAPI compliance and can break generated clients.🛠️ Proposed fix
delete: tags: - contentTypes summary: Delete files related to a given content-type description: 'Required permission "write_configuration"' operationId: deleteContentType - params: + parameters: - name: siteId in: path description: site ID required: true schema: type: string requestBody: + required: true content: application/json: schema: type: object properties: contentType: type: string description: The content-type to delete deleteDependencies: type: boolean description: Indicates if all dependencies of the content-type should be deleted (defaults to false) required: - - siteId - contentType🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 3992 - 4019, The OpenAPI operation for deleteContentType is invalid: replace the incorrect "params" key with "parameters" and define the path parameter for siteId there (required, in: path, schema type: string); then fix the requestBody schema to match DeleteContentTypeRequest by removing siteId from the required properties and only including contentType (string, required) and deleteDependencies (boolean, optional). Update the operationId deleteContentType entry to reference the corrected parameters and body shape so it matches ContentTypeController.deleteContentType and the DeleteContentTypeRequest model.
♻️ Duplicate comments (2)
src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java (1)
593-602:⚠️ Potential issue | 🟠 MajorSynthetic content-type IDs still leak into v2
getContentType(..)call.The exclusion list only filters out
CONTENT_TYPE_FOLDER,"asset", andCONTENT_TYPE_UNKNOWN. However,getContentTypeClass(site, path)(called fromloadContentItemline 576 for non-XML items) andpopulateContentDrivenPropertiescan setitem.contentTypeto other synthetic values likeCONTENT_TYPE_SCRIPT,CONTENT_TYPE_CONFIGURATION,CONTENT_TYPE_RENDERING_TEMPLATE,CONTENT_TYPE_FILE,CONTENT_TYPE_TAXONOMY,CONTENT_TYPE_LEVEL_DESCRIPTOR,CONTENT_TYPE_PAGE,CONTENT_TYPE_COMPONENT,CONTENT_TYPE_DOCUMENT. These are not real content-type folders.Per
ContentTypeServiceInternalImpl.getContentType(siteId, contentTypeId), the v2 service throws (e.g.,ConfigurationException) when the content type doesn't exist, instead of returning null. As a result, any item whose resolved type falls in this synthetic set will throwServiceLayerExceptionhere, propagate up togetContentItem(..)(lines 513-515), and causegetContentItemto returnnullfor scripts, templates, configuration files, taxonomy, etc. — a regression vs. the v1 behavior.Additionally, the
if (contentType != null)guard on line 597 is dead code given the v2 contract (the call either returns a validContentTypeor throws).Suggested approach: gate the call to
contentTypeService.getContentType(..)on real content-type folders only (e.g., reusegetContentTypeClass(..)to detect synthetic classes such asscript,configuration,renderingTemplate,file,taxonomy,page,component,document,levelDescriptor,assetand skip the v2 lookup), or restrict the v2 lookup to IDs that look like actual content-type paths (e.g., starting with/page/,/component/,/taxonomy/under the content-types base).Based on learnings:
ContentTypeServiceInternalImpl.getContentType(String siteId, String contentTypeId)throws an exception (e.g.,ConfigurationException) when the content type does not exist or when there is a parsing problem — it does NOT return null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java` around lines 593 - 602, The current call to contentTypeService.getContentType(site, contentTypeId) in loadContentItem leaks synthetic content-type IDs (e.g., CONTENT_TYPE_SCRIPT, CONTENT_TYPE_CONFIGURATION, CONTENT_TYPE_RENDERING_TEMPLATE, CONTENT_TYPE_FILE, CONTENT_TYPE_TAXONOMY, CONTENT_TYPE_LEVEL_DESCRIPTOR, CONTENT_TYPE_PAGE, CONTENT_TYPE_COMPONENT, CONTENT_TYPE_DOCUMENT) and can throw from ContentTypeServiceInternalImpl instead of returning null; gate the v2 lookup so it only runs for real content-type folders: reuse getContentTypeClass(site, path) or check that contentTypeId matches real content-type path patterns (e.g., starts with the content-types base like "/page/" or "/component/" etc.) before calling contentTypeService.getContentType(...), skip the call for synthetic classes listed above (which are set by loadContentItem and populateContentDrivenProperties), and remove the now-dead if (contentType != null) branch since getContentType either returns a valid ContentType or throws; ensure getContentItem callers are unaffected by these synthetic types.src/main/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImpl.java (1)
222-232:⚠️ Potential issue | 🟡 MinorEmpty-update edge case.
When both
nameanddescriptionare null/blank,siteDao.updateSite(...)will either fail (empty SET clause) or return 0, surfacing as a misleadingSiteNotFoundExceptionhere. See the related comment onSiteDAO.xmlupdateSite for the recommended fix; an earlyreturn;at the start of this method when both args are blank would also work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImpl.java` around lines 222 - 232, The updateSite method calls siteDao.updateSite even when both name and description are null/blank, which causes an empty-update error or a misleading SiteNotFoundException; fix this in updateSite by checking both inputs up front (e.g., using the same isNotEmpty/isNotBlank helper used elsewhere) and returning early when neither name nor description is provided so you do not call siteDao.updateSite unnecessarily or throw the wrong exception.
🧹 Nitpick comments (2)
src/main/java/org/craftercms/studio/api/v2/utils/StudioUtils.java (1)
55-56: Optional: drop redundant^anchor.
Matcher#matches()requires the entire input to match, so the leading^is redundant in both patterns (and there's no$for symmetry). Either remove^or anchor both ends for readability — purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/api/v2/utils/StudioUtils.java` around lines 55 - 56, The two regex constants COMPONENT_CONTENT_TYPE_ID_PATTERN and PAGE_CONTENT_TYPE_ID_PATTERN use a redundant leading '^' because their callers use Matcher#matches(); update the Pattern.compile invocations to remove the leading '^' (change "^/component/.*" to "/component/.*" and "^/page/.*" to "/page/.*") so the patterns remain correct and consistent; alternatively you may anchor both ends by adding '^' and '$' if you prefer strict full-string anchors, but do not leave an unmatched leading '^' alone.src/main/java/org/craftercms/studio/impl/v1/web/security/access/StudioAbstractAccessDecisionVoter.java (1)
65-82: Optional: simplify withanyMatch.The intermediate
HashSetis built only to perform a singlecontainscheck. A streamanyMatchis more idiomatic, short-circuits on the first match, and avoids the extra allocation:♻️ Proposed simplification
protected boolean isSiteMember(String siteId, User currentUser) { try { List<Site> userSites = userService.getUserSites(-1, currentUser.getUsername()); - - Set<String> sites = new HashSet<>(); - for (Site site : userSites) { - sites.add(site.getSiteId()); - } - - return sites.contains(siteId); + return userSites.stream().anyMatch(s -> siteId.equals(s.getSiteId())); } catch (UserNotFoundException e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/impl/v1/web/security/access/StudioAbstractAccessDecisionVoter.java` around lines 65 - 82, The isSiteMember method builds a temporary HashSet just to check membership; replace the manual loop and set with a stream anyMatch on the list returned by userService.getUserSites(-1, currentUser.getUsername()) to directly check siteId equality (e.g., userSites.stream().anyMatch(s -> siteId.equals(s.getSiteId()))), preserving the existing catch blocks for UserNotFoundException and ServiceLayerException and keeping the same logger calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/api/studio-api.yaml`:
- Around line 10527-10529: The OpenAPI schema declares the field as "label" but
the server-side model (ContentType.java) serializes it as "title" via
`@JsonProperty`("title"); update the schema entry in studio-api.yaml to expose
"title" instead of "label" so generated clients match the server wire format
(replace the "label" property name with "title" and keep the same
type/description).
In `@src/main/java/org/craftercms/studio/api/v2/dal/SiteDAO.java`:
- Around line 173-180: SitesServiceImpl.updateSite() currently allows blank
strings to be passed to SiteDAO.updateSite(String siteId, String name, String
description), which causes blank values to be persisted; before calling
SiteDAO.updateSite(...) normalize any blank-only name or description to null
(e.g., use StringUtils.isBlank or trim+isEmpty) so the mapper's "ignore null"
behavior is used, or alternatively adjust the SQL/mapper to treat empty strings
as NULL for the NAME and DESC parameters; update the call site in
SitesServiceImpl.updateSite() to convert "" (or whitespace-only) to null for the
NAME and DESC parameters before invoking SiteDAO.updateSite.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/content/ContentTypeServiceImpl.java`:
- Around line 123-135: The permission on configuration endpoints is too broad:
update the `@HasPermission` annotation on getAllContentTypes(...) and
getContentType(...) to require PERMISSION_READ_CONFIGURATION (same guard used by
getAllModelDefinitions()) instead of PERMISSION_CONTENT_READ; ensure the
constant PERMISSION_READ_CONFIGURATION is imported or available and update both
method annotations so configuration/form definitions are only returned to
callers with configuration-read rights.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentTypeServiceInternalImpl.java`:
- Around line 292-295: The method getContentTypeFormController currently checks
for the presence of the form-definition.xml
(concat(getContentTypePath(contentTypeId), contentTypeDefinitionFilename)) but
then unconditionally tries to load the form controller (formControllerFilePath),
which causes ContentNotFoundException when a definition exists but the
controller does not; change the existence check to verify the controller path
(build controllerPath via concat(getContentTypePath(contentTypeId),
formControllerFilePath) and call contentService.contentExists with that
controllerPath) and only then return the ImmutablePair(controllerPath,
contentService.getContentAsResource(...)); if the controller is absent, return
null to preserve the method contract.
In `@src/main/webapp/default-site/scripts/pages/preview.groovy`:
- Line 31: The preview model currently hardcodes model.authenticationType = ''
which drops real authentication info; replace that assignment so it derives the
type from the authenticated user like the sibling files do (use
authenticatedUser.getAuthenticationType() as String or equivalent) so
preview.groovy populates model.authenticationType consistently with entry.groovy
and EnvironmentOverrides.groovy rather than an empty string.
---
Outside diff comments:
In `@src/main/api/studio-api.yaml`:
- Around line 3992-4019: The OpenAPI operation for deleteContentType is invalid:
replace the incorrect "params" key with "parameters" and define the path
parameter for siteId there (required, in: path, schema type: string); then fix
the requestBody schema to match DeleteContentTypeRequest by removing siteId from
the required properties and only including contentType (string, required) and
deleteDependencies (boolean, optional). Update the operationId deleteContentType
entry to reference the corrected parameters and body shape so it matches
ContentTypeController.deleteContentType and the DeleteContentTypeRequest model.
---
Duplicate comments:
In
`@src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java`:
- Around line 593-602: The current call to
contentTypeService.getContentType(site, contentTypeId) in loadContentItem leaks
synthetic content-type IDs (e.g., CONTENT_TYPE_SCRIPT,
CONTENT_TYPE_CONFIGURATION, CONTENT_TYPE_RENDERING_TEMPLATE, CONTENT_TYPE_FILE,
CONTENT_TYPE_TAXONOMY, CONTENT_TYPE_LEVEL_DESCRIPTOR, CONTENT_TYPE_PAGE,
CONTENT_TYPE_COMPONENT, CONTENT_TYPE_DOCUMENT) and can throw from
ContentTypeServiceInternalImpl instead of returning null; gate the v2 lookup so
it only runs for real content-type folders: reuse getContentTypeClass(site,
path) or check that contentTypeId matches real content-type path patterns (e.g.,
starts with the content-types base like "/page/" or "/component/" etc.) before
calling contentTypeService.getContentType(...), skip the call for synthetic
classes listed above (which are set by loadContentItem and
populateContentDrivenProperties), and remove the now-dead if (contentType !=
null) branch since getContentType either returns a valid ContentType or throws;
ensure getContentItem callers are unaffected by these synthetic types.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImpl.java`:
- Around line 222-232: The updateSite method calls siteDao.updateSite even when
both name and description are null/blank, which causes an empty-update error or
a misleading SiteNotFoundException; fix this in updateSite by checking both
inputs up front (e.g., using the same isNotEmpty/isNotBlank helper used
elsewhere) and returning early when neither name nor description is provided so
you do not call siteDao.updateSite unnecessarily or throw the wrong exception.
---
Nitpick comments:
In `@src/main/java/org/craftercms/studio/api/v2/utils/StudioUtils.java`:
- Around line 55-56: The two regex constants COMPONENT_CONTENT_TYPE_ID_PATTERN
and PAGE_CONTENT_TYPE_ID_PATTERN use a redundant leading '^' because their
callers use Matcher#matches(); update the Pattern.compile invocations to remove
the leading '^' (change "^/component/.*" to "/component/.*" and "^/page/.*" to
"/page/.*") so the patterns remain correct and consistent; alternatively you may
anchor both ends by adding '^' and '$' if you prefer strict full-string anchors,
but do not leave an unmatched leading '^' alone.
In
`@src/main/java/org/craftercms/studio/impl/v1/web/security/access/StudioAbstractAccessDecisionVoter.java`:
- Around line 65-82: The isSiteMember method builds a temporary HashSet just to
check membership; replace the manual loop and set with a stream anyMatch on the
list returned by userService.getUserSites(-1, currentUser.getUsername()) to
directly check siteId equality (e.g., userSites.stream().anyMatch(s ->
siteId.equals(s.getSiteId()))), preserving the existing catch blocks for
UserNotFoundException and ServiceLayerException and keeping the same logger
calls.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77c15719-f25d-43c1-b6c3-64ddd011424b
📒 Files selected for processing (58)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/dal/GroupPerSiteResult.javasrc/main/java/org/craftercms/studio/api/v1/dal/GroupResult.javasrc/main/java/org/craftercms/studio/api/v1/dal/Permission.javasrc/main/java/org/craftercms/studio/api/v1/dal/Role.javasrc/main/java/org/craftercms/studio/api/v1/dal/SiteFeed.javasrc/main/java/org/craftercms/studio/api/v1/dal/SiteFeedMapper.javasrc/main/java/org/craftercms/studio/api/v1/dal/UserProfileResult.javasrc/main/java/org/craftercms/studio/api/v1/service/asset/processing/AssetProcessingService.javasrc/main/java/org/craftercms/studio/api/v1/service/configuration/ContentTypesConfig.javasrc/main/java/org/craftercms/studio/api/v1/service/configuration/ServicesConfig.javasrc/main/java/org/craftercms/studio/api/v1/service/content/ContentTypeService.javasrc/main/java/org/craftercms/studio/api/v1/service/security/SecurityService.javasrc/main/java/org/craftercms/studio/api/v1/service/site/SiteService.javasrc/main/java/org/craftercms/studio/api/v1/to/ContentItemTO.javasrc/main/java/org/craftercms/studio/api/v1/to/ContentTypeConfigTO.javasrc/main/java/org/craftercms/studio/api/v1/util/DmContentItemComparator.javasrc/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteReady.javasrc/main/java/org/craftercms/studio/api/v2/dal/SiteDAO.javasrc/main/java/org/craftercms/studio/api/v2/service/content/ContentTypeService.javasrc/main/java/org/craftercms/studio/api/v2/utils/StudioUtils.javasrc/main/java/org/craftercms/studio/impl/v1/service/configuration/ContentTypesConfigImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/configuration/ServicesConfigImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentTypeServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/security/SecurityServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/web/security/access/StudioAbstractAccessDecisionVoter.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentTypeServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentTypeServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/marketplace/internal/MarketplaceServiceInternalImpl.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/utils/spring/ContentResource.javasrc/main/java/org/craftercms/studio/model/contentType/ContentType.javasrc/main/java/org/craftercms/studio/model/rest/ApiResponse.javasrc/main/java/org/craftercms/studio/model/rest/contentType/DeleteContentTypeRequest.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/database/mybatis-config.xmlsrc/main/resources/crafter/studio/security/api.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/DependencyMapper.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/SiteFeedMapper.xmlsrc/main/resources/org/craftercms/studio/api/v2/dal/GroupDAO.xmlsrc/main/resources/org/craftercms/studio/api/v2/dal/SiteDAO.xmlsrc/main/webapp/default-site/scripts/api/SecurityServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/UserServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovysrc/main/webapp/default-site/scripts/api/impl/security/SpringSecurityServices.groovysrc/main/webapp/default-site/scripts/libs/EnvironmentOverrides.groovysrc/main/webapp/default-site/scripts/pages/entry.groovysrc/main/webapp/default-site/scripts/pages/login.groovysrc/main/webapp/default-site/scripts/pages/preview.groovysrc/test/java/org/craftercms/studio/api/v2/utils/StudioUtilsTest.javasrc/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentTypeServiceInternalImplTest.javasrc/test/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImplTest.java
💤 Files with no reviewable changes (23)
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovy
- src/main/java/org/craftercms/studio/api/v1/dal/Permission.java
- src/main/webapp/default-site/scripts/api/impl/security/SpringSecurityServices.groovy
- src/main/resources/crafter/studio/database-context.xml
- src/main/java/org/craftercms/studio/api/v1/dal/GroupResult.java
- src/main/webapp/default-site/scripts/api/UserServices.groovy
- src/main/java/org/craftercms/studio/api/v1/dal/GroupPerSiteResult.java
- src/main/java/org/craftercms/studio/api/v1/dal/UserProfileResult.java
- src/main/java/org/craftercms/studio/api/v1/service/site/SiteService.java
- src/main/resources/org/craftercms/studio/api/v1/dal/DependencyMapper.xml
- src/main/webapp/default-site/scripts/api/SecurityServices.groovy
- src/main/java/org/craftercms/studio/api/v1/dal/SiteFeedMapper.java
- src/main/java/org/craftercms/studio/api/v1/dal/SiteFeed.java
- src/main/java/org/craftercms/studio/api/v1/util/DmContentItemComparator.java
- src/main/java/org/craftercms/studio/api/v1/to/ContentTypeConfigTO.java
- src/main/java/org/craftercms/studio/api/v1/service/configuration/ContentTypesConfig.java
- src/main/java/org/craftercms/studio/api/v1/dal/Role.java
- src/main/java/org/craftercms/studio/api/v1/service/asset/processing/AssetProcessingService.java
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentTypeService.java
- src/main/java/org/craftercms/studio/impl/v1/service/configuration/ContentTypesConfigImpl.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentTypeServiceImpl.java
- src/main/resources/org/craftercms/studio/api/v1/dal/SiteFeedMapper.xml
- src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java
✅ Files skipped from review due to trivial changes (4)
- src/main/resources/org/craftercms/studio/api/v2/dal/GroupDAO.xml
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/ContentResource.java
- src/main/java/org/craftercms/studio/impl/v2/service/marketplace/internal/MarketplaceServiceInternalImpl.java
- src/main/java/org/craftercms/studio/model/contentType/ContentType.java
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/java/org/craftercms/studio/model/rest/ApiResponse.java
- src/main/java/org/craftercms/studio/model/rest/contentType/DeleteContentTypeRequest.java
- src/main/resources/crafter/studio/security/api.xml
- src/main/java/org/craftercms/studio/api/v1/to/ContentItemTO.java
- src/main/webapp/default-site/scripts/api/ServiceFactory.groovy
- src/main/java/org/craftercms/studio/impl/v1/service/configuration/ServicesConfigImpl.java
- src/main/webapp/default-site/scripts/libs/EnvironmentOverrides.groovy
jmendeza
force-pushed
the
feature/6061-content-types-cleanup
branch
from
d51af77 to
d72db56
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/api/studio-api.yaml (1)
3828-3831:⚠️ Potential issue | 🟠 MajorFix OpenAPI payload key to match controller implementation.
The endpoint
/api/2/configuration/content_types/{siteId}/usagesets the payload key to"usage"(viaRESULT_KEY_USAGEin ContentTypeController line 68), but the spec documents"dependencies"at line 3830. Update the OpenAPI spec to use"usage"as the payload key to match the controller serialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 3828 - 3831, The OpenAPI spec for the /api/2/configuration/content_types/{siteId}/usage endpoint uses the payload key "dependencies" but the controller (ContentTypeController, see RESULT_KEY_USAGE constant) serializes the response under "usage"; update the spec so the response payload key is "usage" instead of "dependencies" (i.e., change the property name at the endpoint response schema to "usage" to match the controller).
♻️ Duplicate comments (2)
src/main/webapp/default-site/scripts/pages/preview.groovy (1)
31-31:⚠️ Potential issue | 🟠 MajorRestore
authenticationTypefrom the authenticated user.At Line 31,
model.authenticationType = ''drops user auth metadata and can break preview consumers expecting the real value.🔧 Proposed fix
-model.authenticationType = '' +model.authenticationType = currentUser?.getAuthenticationType() as String🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/default-site/scripts/pages/preview.groovy` at line 31, Replace the hardcoded empty assignment to model.authenticationType in preview.groovy with the actual value from the authenticated principal used by the app (for example the controller/template's authenticatedUser, request.user, session.principal, or equivalent). Locate the line setting model.authenticationType and assign it from the auth principal's authenticationType property (use null-safe access and fall back to an empty string only if the principal is absent) so preview consumers receive the real auth metadata.src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java (1)
594-597:⚠️ Potential issue | 🟠 MajorSkip synthetic/system types before calling
contentTypeService.getContentType(...).At Line 596, synthetic IDs like
script,configuration,rendering-template,file(and taxonomy class values) can still hitgetContentType(...). In this codebase that method throws for non-existent content types, sogetContentItem(...)can fail for those assets.Suggested fix
- if (contentTypeId != null && !contentTypeId.equals(CONTENT_TYPE_FOLDER) && !contentTypeId.equals("asset") && - !contentTypeId.equals(CONTENT_TYPE_UNKNOWN)) { + if (contentTypeId != null && !Set.of( + CONTENT_TYPE_FOLDER, + CONTENT_TYPE_ASSET, + CONTENT_TYPE_UNKNOWN, + CONTENT_TYPE_SCRIPT, + CONTENT_TYPE_CONFIGURATION, + CONTENT_TYPE_RENDERING_TEMPLATE, + CONTENT_TYPE_FILE, + CONTENT_TYPE_TAXONOMY + ).contains(contentTypeId)) { ContentType contentType = contentTypeService.getContentType(site, contentTypeId); if (contentType != null) { item.setForm(contentType.getId());Based on learnings:
ContentTypeServiceInternalImpl.getContentType(String siteId, String contentTypeId)throws when the content type does not exist, rather than returning null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java` around lines 594 - 597, The code calls contentTypeService.getContentType(site, contentTypeId) for IDs that may be synthetic/system types (e.g., "script", "configuration", "rendering-template", "file" and taxonomy class values) which causes exceptions; update the guarding condition around contentTypeId (the check that currently uses CONTENT_TYPE_FOLDER and CONTENT_TYPE_UNKNOWN) to also explicitly skip these synthetic/system IDs before calling contentTypeService.getContentType(...) so getContentItem(...) never invokes getContentType for those values; reference contentTypeId, CONTENT_TYPE_FOLDER, CONTENT_TYPE_UNKNOWN, contentTypeService.getContentType(...) and getContentItem(...) when making the change.
🧹 Nitpick comments (2)
src/main/java/org/craftercms/studio/impl/v1/web/security/access/StudioAbstractAccessDecisionVoter.java (1)
69-74: You can avoid extra allocation in the membership check.At Line 69-74, the
HashSetis only used for onecontainscall; a short-circuit loop is simpler and cheaper.♻️ Proposed simplification
- Set<String> sites = new HashSet<>(); - for (Site site : userSites) { - sites.add(site.getSiteId()); - } - - return sites.contains(siteId); + for (Site site : userSites) { + if (StringUtils.equals(site.getSiteId(), siteId)) { + return true; + } + } + return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/impl/v1/web/security/access/StudioAbstractAccessDecisionVoter.java` around lines 69 - 74, The code in StudioAbstractAccessDecisionVoter builds a temporary HashSet named "sites" from "userSites" just to call contains(siteId); remove that allocation and replace it with a short-circuit membership check by iterating userSites (for-each) and returning true as soon as site.getSiteId().equals(siteId) (or use userSites.stream().anyMatch(s -> s.getSiteId().equals(siteId))) to avoid extra allocation and improve performance.src/main/api/studio-api.yaml (1)
3804-4050: AddcontentTypesto top-level OpenAPI tags for cleaner docs grouping.These operations consistently use
contentTypes, but that tag isn’t declared in the roottags:list.🧹 Suggested patch
tags: - name: groups description: Group management operations @@ - name: configuration description: Configuration operations + - name: contentTypes + description: Content type configuration operations - name: plugin description: Plugin operations🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 3804 - 4050, Add a top-level OpenAPI tag entry named "contentTypes" to the root tags list so all endpoints using tags: - contentTypes are declared; update the root tags array (where other tags are defined) to include an object with name: "contentTypes" and a short description (e.g., "Content types configuration and management") so the getContentTypeUsage, getContentTypePreviewImage, getAllowedContentTypes, getContentTypeConfiguration, deleteContentType, getContentTypeFormController, etc. appear grouped correctly in the generated docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/api/studio-api.yaml`:
- Line 8265: The OpenAPI spec documents the CONTENT_TYPE_IN_USE error code as
54000 but the backend returns 55000, causing a contract mismatch; update the
studio-api.yaml entry for the CONTENT_TYPE_IN_USE error (the line containing
"`CONTENT_TYPE_IN_USE`" / the message "The content type cannot be deleted
because it is still in use by content items") to use code 55000 to match runtime
behavior, or alternatively coordinate with the backend to change the implemented
value to 54000—ensure the `CODE:` field in the spec equals the backend's numeric
constant (55000) so clients and API docs agree.
---
Outside diff comments:
In `@src/main/api/studio-api.yaml`:
- Around line 3828-3831: The OpenAPI spec for the
/api/2/configuration/content_types/{siteId}/usage endpoint uses the payload key
"dependencies" but the controller (ContentTypeController, see RESULT_KEY_USAGE
constant) serializes the response under "usage"; update the spec so the response
payload key is "usage" instead of "dependencies" (i.e., change the property name
at the endpoint response schema to "usage" to match the controller).
---
Duplicate comments:
In
`@src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java`:
- Around line 594-597: The code calls contentTypeService.getContentType(site,
contentTypeId) for IDs that may be synthetic/system types (e.g., "script",
"configuration", "rendering-template", "file" and taxonomy class values) which
causes exceptions; update the guarding condition around contentTypeId (the check
that currently uses CONTENT_TYPE_FOLDER and CONTENT_TYPE_UNKNOWN) to also
explicitly skip these synthetic/system IDs before calling
contentTypeService.getContentType(...) so getContentItem(...) never invokes
getContentType for those values; reference contentTypeId, CONTENT_TYPE_FOLDER,
CONTENT_TYPE_UNKNOWN, contentTypeService.getContentType(...) and
getContentItem(...) when making the change.
In `@src/main/webapp/default-site/scripts/pages/preview.groovy`:
- Line 31: Replace the hardcoded empty assignment to model.authenticationType in
preview.groovy with the actual value from the authenticated principal used by
the app (for example the controller/template's authenticatedUser, request.user,
session.principal, or equivalent). Locate the line setting
model.authenticationType and assign it from the auth principal's
authenticationType property (use null-safe access and fall back to an empty
string only if the principal is absent) so preview consumers receive the real
auth metadata.
---
Nitpick comments:
In `@src/main/api/studio-api.yaml`:
- Around line 3804-4050: Add a top-level OpenAPI tag entry named "contentTypes"
to the root tags list so all endpoints using tags: - contentTypes are declared;
update the root tags array (where other tags are defined) to include an object
with name: "contentTypes" and a short description (e.g., "Content types
configuration and management") so the getContentTypeUsage,
getContentTypePreviewImage, getAllowedContentTypes, getContentTypeConfiguration,
deleteContentType, getContentTypeFormController, etc. appear grouped correctly
in the generated docs.
In
`@src/main/java/org/craftercms/studio/impl/v1/web/security/access/StudioAbstractAccessDecisionVoter.java`:
- Around line 69-74: The code in StudioAbstractAccessDecisionVoter builds a
temporary HashSet named "sites" from "userSites" just to call contains(siteId);
remove that allocation and replace it with a short-circuit membership check by
iterating userSites (for-each) and returning true as soon as
site.getSiteId().equals(siteId) (or use userSites.stream().anyMatch(s ->
s.getSiteId().equals(siteId))) to avoid extra allocation and improve
performance.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 651f5a55-66dc-4def-9daa-094865fe1a30
📒 Files selected for processing (57)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/dal/GroupPerSiteResult.javasrc/main/java/org/craftercms/studio/api/v1/dal/GroupResult.javasrc/main/java/org/craftercms/studio/api/v1/dal/Permission.javasrc/main/java/org/craftercms/studio/api/v1/dal/Role.javasrc/main/java/org/craftercms/studio/api/v1/dal/SiteFeed.javasrc/main/java/org/craftercms/studio/api/v1/dal/SiteFeedMapper.javasrc/main/java/org/craftercms/studio/api/v1/dal/UserProfileResult.javasrc/main/java/org/craftercms/studio/api/v1/service/asset/processing/AssetProcessingService.javasrc/main/java/org/craftercms/studio/api/v1/service/configuration/ContentTypesConfig.javasrc/main/java/org/craftercms/studio/api/v1/service/configuration/ServicesConfig.javasrc/main/java/org/craftercms/studio/api/v1/service/content/ContentTypeService.javasrc/main/java/org/craftercms/studio/api/v1/service/security/SecurityService.javasrc/main/java/org/craftercms/studio/api/v1/service/site/SiteService.javasrc/main/java/org/craftercms/studio/api/v1/to/ContentItemTO.javasrc/main/java/org/craftercms/studio/api/v1/to/ContentTypeConfigTO.javasrc/main/java/org/craftercms/studio/api/v1/util/DmContentItemComparator.javasrc/main/java/org/craftercms/studio/api/v2/annotation/RequireSiteReady.javasrc/main/java/org/craftercms/studio/api/v2/dal/SiteDAO.javasrc/main/java/org/craftercms/studio/api/v2/service/content/ContentTypeService.javasrc/main/java/org/craftercms/studio/api/v2/utils/StudioUtils.javasrc/main/java/org/craftercms/studio/impl/v1/service/configuration/ContentTypesConfigImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/configuration/ServicesConfigImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentTypeServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/security/SecurityServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/web/security/access/StudioAbstractAccessDecisionVoter.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentTypeServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentTypeServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/marketplace/internal/MarketplaceServiceInternalImpl.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/utils/spring/ContentResource.javasrc/main/java/org/craftercms/studio/model/contentType/ContentType.javasrc/main/java/org/craftercms/studio/model/rest/contentType/DeleteContentTypeRequest.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/database/mybatis-config.xmlsrc/main/resources/crafter/studio/security/api.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/DependencyMapper.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/SiteFeedMapper.xmlsrc/main/resources/org/craftercms/studio/api/v2/dal/GroupDAO.xmlsrc/main/resources/org/craftercms/studio/api/v2/dal/SiteDAO.xmlsrc/main/webapp/default-site/scripts/api/SecurityServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/UserServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovysrc/main/webapp/default-site/scripts/api/impl/security/SpringSecurityServices.groovysrc/main/webapp/default-site/scripts/libs/EnvironmentOverrides.groovysrc/main/webapp/default-site/scripts/pages/entry.groovysrc/main/webapp/default-site/scripts/pages/login.groovysrc/main/webapp/default-site/scripts/pages/preview.groovysrc/test/java/org/craftercms/studio/api/v2/utils/StudioUtilsTest.javasrc/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentTypeServiceInternalImplTest.javasrc/test/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImplTest.java
💤 Files with no reviewable changes (23)
- src/main/java/org/craftercms/studio/api/v1/dal/GroupResult.java
- src/main/java/org/craftercms/studio/api/v1/dal/Permission.java
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovy
- src/main/resources/crafter/studio/database-context.xml
- src/main/webapp/default-site/scripts/api/UserServices.groovy
- src/main/java/org/craftercms/studio/api/v1/service/asset/processing/AssetProcessingService.java
- src/main/java/org/craftercms/studio/api/v1/dal/GroupPerSiteResult.java
- src/main/java/org/craftercms/studio/api/v1/to/ContentTypeConfigTO.java
- src/main/java/org/craftercms/studio/api/v1/dal/Role.java
- src/main/java/org/craftercms/studio/api/v1/service/configuration/ContentTypesConfig.java
- src/main/java/org/craftercms/studio/api/v1/dal/SiteFeed.java
- src/main/java/org/craftercms/studio/api/v1/dal/UserProfileResult.java
- src/main/resources/org/craftercms/studio/api/v1/dal/DependencyMapper.xml
- src/main/java/org/craftercms/studio/api/v1/service/site/SiteService.java
- src/main/java/org/craftercms/studio/api/v1/util/DmContentItemComparator.java
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentTypeService.java
- src/main/webapp/default-site/scripts/api/impl/security/SpringSecurityServices.groovy
- src/main/java/org/craftercms/studio/api/v1/dal/SiteFeedMapper.java
- src/main/webapp/default-site/scripts/api/SecurityServices.groovy
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentTypeServiceImpl.java
- src/main/resources/org/craftercms/studio/api/v1/dal/SiteFeedMapper.xml
- src/main/java/org/craftercms/studio/impl/v1/service/site/SiteServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v1/service/configuration/ContentTypesConfigImpl.java
✅ Files skipped from review due to trivial changes (6)
- src/main/java/org/craftercms/studio/model/rest/contentType/DeleteContentTypeRequest.java
- src/main/resources/crafter/studio/database/mybatis-config.xml
- src/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v2/utils/spring/ContentResource.java
- src/main/resources/org/craftercms/studio/api/v2/dal/GroupDAO.xml
- src/main/java/org/craftercms/studio/model/contentType/ContentType.java
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/java/org/craftercms/studio/api/v1/service/configuration/ServicesConfig.java
- src/main/resources/crafter/studio/security/api.xml
- src/main/webapp/default-site/scripts/pages/login.groovy
- src/main/java/org/craftercms/studio/api/v2/dal/SiteDAO.java
- src/main/webapp/default-site/scripts/pages/entry.groovy
- src/main/webapp/default-site/scripts/libs/EnvironmentOverrides.groovy
- src/main/java/org/craftercms/studio/impl/v2/service/site/internal/SitesServiceInternalImpl.java
jmendeza
force-pushed
the
feature/6061-content-types-cleanup
branch
from
25793da to
d222944
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/api/studio-api.yaml (1)
3826-3847:⚠️ Potential issue | 🟠 MajorExpose this payload as
usage, notdependencies.
ContentTypeController.getContentTypeUsage()serializes theResultOneentity underusage. Keepingdependencieshere makes the documented wire format diverge from the actual response shape.🛠️ Proposed fix
- dependencies: + usage: type: object properties: templates: type: array🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/api/studio-api.yaml` around lines 3826 - 3847, The OpenAPI schema currently names the payload property "dependencies" but the controller (ContentTypeController.getContentTypeUsage) actually returns the ResultOne payload under "usage"; change the schema object key from "dependencies" to "usage", update its nested properties (templates, scripts, content) and their descriptions to reflect they are usage details, and ensure any $ref or examples referencing "dependencies" are updated to "usage" so the documented wire format matches the actual response shape.
♻️ Duplicate comments (1)
src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java (1)
514-523:⚠️ Potential issue | 🟠 MajorOnly resolve real content-type IDs through
contentTypeService.
loadContentItem()feedsloadContentTypeProperties()withitem.contentTypefromgetContentTypeClass(site, path)for non-XML/config items, but this branch only filtersfolder,asset, andunknown. Other system-class values still hitcontentTypeService.getContentType(...), sogetContentItem(...)can end up swallowing that exception and returningnullfor those items.Suggested fix
- if (contentTypeId != null && !contentTypeId.equals(CONTENT_TYPE_FOLDER) && !contentTypeId.equals("asset") && - !contentTypeId.equals(CONTENT_TYPE_UNKNOWN)) { + if ((item.isPage() || item.isComponent() || item.isDocument()) && + StringUtils.isNotBlank(contentTypeId)) { ContentType contentType = contentTypeService.getContentType(site, contentTypeId); - if (contentType != null) { - item.setForm(contentType.getId()); - item.setPreviewable(contentType.isPreviewable()); - item.isPreviewable = item.previewable; - } + item.setForm(contentType.getId()); + item.setPreviewable(contentType.isPreviewable()); + item.isPreviewable = item.previewable; } else { String mimeType = StudioUtils.getMimeType(item.getName());Based on learnings,
ContentTypeServiceInternalImpl.getContentType(String siteId, String contentTypeId)throws when the content type does not exist, so system-class values must be filtered before this call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java` around lines 514 - 523, The code calls contentTypeService.getContentType(...) for many non-folder/asset/unknown IDs and that can throw for system-class values; update loadContentTypeProperties to avoid calling ContentTypeService for system-class contentTypes by adding a guard (e.g., implement and call a helper isSystemContentType(String contentTypeId)) and extend the existing condition before invoking contentTypeService.getContentType(site, contentTypeId) so system-class IDs are filtered out (keep existing checks for CONTENT_TYPE_FOLDER, "asset", CONTENT_TYPE_UNKNOWN and skip any IDs that isSystemContentType(...) returns true); this prevents exceptions from ContentTypeService.getContentType from being raised for internal/system classes.
🧹 Nitpick comments (2)
src/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java (1)
84-90: Handle whitespace-onlyname/descriptionas missing input.
defaultIfEmptydoes not catch" ", so whitespace-only values can bypass the validation and be stored. Consider trimming before null/empty checks.💡 Proposed adjustment
- String normalizedName = defaultIfEmpty(name, null); - String normalizedDescription = defaultIfEmpty(description, null); + String normalizedName = (name == null || name.trim().isEmpty()) ? null : name.trim(); + String normalizedDescription = (description == null || description.trim().isEmpty()) ? null : description.trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java` around lines 84 - 90, Trim the incoming name and description before checking for emptiness so whitespace-only strings are treated as missing: replace the current normalization that assigns normalizedName and normalizedDescription with logic that trims the raw name/description (or uses StringUtils.isBlank) and then maps blank/empty to null; update the validation that throws InvalidParametersException to use these trimmed/nullified values and pass them into sitesServiceInternal.updateSite(siteId, normalizedName, normalizedDescription).src/main/java/org/craftercms/studio/api/v2/service/content/ContentTypeService.java (1)
116-122: Align Javadoc exception contract with the method signature.Line 120 documents
SiteNotFoundException, butgetAllContentTypesdeclaresthrows ServiceLayerException. Consider making the Javadoc consistent with the declared contract to avoid ambiguity for API consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/api/v2/service/content/ContentTypeService.java` around lines 116 - 122, The Javadoc for ContentTypeService.getAllContentTypes is inconsistent: it documents SiteNotFoundException but the method signature only declares throws ServiceLayerException; update the Javadoc on getAllContentTypes to reflect the declared contract by replacing or augmenting the `@throws` tag to document ServiceLayerException (and only include SiteNotFoundException if you also change the method signature to throw it), referencing the method name ContentTypeService.getAllContentTypes and the ServiceLayerException class so API consumers see the correct exception contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/api/studio-api.yaml`:
- Line 3855: Update the OpenAPI paths that use underscore subpaths to the
controller's hyphenated subpaths: replace /preview_image with /preview-image,
/allowed_types with /allowed-types, and /form_controller with /form-controller
in the spec entries (e.g., the path string currently shown as
"/api/2/configuration/content_types/{siteId}/preview_image"); ensure the
operation objects and any $ref/servers/links that reference the underscored
paths are updated to the hyphenated versions so clients are pointed at the
actual controller routes.
In
`@src/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.java`:
- Around line 673-678: The update call is using repoOperation.getPath() to
compute the content type/systemType for a moved item; change the arguments that
determine system type to use the move destination path instead so the DB stores
the new systemType. Specifically, in the updateItemRow invocation replace the
repoOperation.getPath() passed into getContentTypeClass (and any other
system-type/mime/size computations related to the moved item) with the
operation's destination path (e.g., repoOperation.getDestinationPath()) so
getContentTypeClass(...) and related calls reflect the destination rather than
the source path.
In
`@src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/AbstractContentUpgradeOperation.java`:
- Around line 87-88: The line setting includedPaths uses an unresolved TODO in
the runtime path resolution; replace the inline TODO by finalizing the path
logic or referencing a tracked issue and extract the fallback computation into a
small helper (e.g., a method like computeRelativeConfigPath or
resolveFallbackConfigPath) called from the current site upgrade flow so the call
becomes includedPaths =
singletonList(repo.resolve(computeRelativeConfigPath(context.getCurrentConfigPath(),
File.separator))); ensure the helper encapsulates the CS.removeStart usage and
clearly documents expected behavior or links to an issue ID if you opt not to
complete the logic now.
---
Outside diff comments:
In `@src/main/api/studio-api.yaml`:
- Around line 3826-3847: The OpenAPI schema currently names the payload property
"dependencies" but the controller (ContentTypeController.getContentTypeUsage)
actually returns the ResultOne payload under "usage"; change the schema object
key from "dependencies" to "usage", update its nested properties (templates,
scripts, content) and their descriptions to reflect they are usage details, and
ensure any $ref or examples referencing "dependencies" are updated to "usage" so
the documented wire format matches the actual response shape.
---
Duplicate comments:
In
`@src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java`:
- Around line 514-523: The code calls contentTypeService.getContentType(...) for
many non-folder/asset/unknown IDs and that can throw for system-class values;
update loadContentTypeProperties to avoid calling ContentTypeService for
system-class contentTypes by adding a guard (e.g., implement and call a helper
isSystemContentType(String contentTypeId)) and extend the existing condition
before invoking contentTypeService.getContentType(site, contentTypeId) so
system-class IDs are filtered out (keep existing checks for CONTENT_TYPE_FOLDER,
"asset", CONTENT_TYPE_UNKNOWN and skip any IDs that isSystemContentType(...)
returns true); this prevents exceptions from ContentTypeService.getContentType
from being raised for internal/system classes.
---
Nitpick comments:
In
`@src/main/java/org/craftercms/studio/api/v2/service/content/ContentTypeService.java`:
- Around line 116-122: The Javadoc for ContentTypeService.getAllContentTypes is
inconsistent: it documents SiteNotFoundException but the method signature only
declares throws ServiceLayerException; update the Javadoc on getAllContentTypes
to reflect the declared contract by replacing or augmenting the `@throws` tag to
document ServiceLayerException (and only include SiteNotFoundException if you
also change the method signature to throw it), referencing the method name
ContentTypeService.getAllContentTypes and the ServiceLayerException class so API
consumers see the correct exception contract.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.java`:
- Around line 84-90: Trim the incoming name and description before checking for
emptiness so whitespace-only strings are treated as missing: replace the current
normalization that assigns normalizedName and normalizedDescription with logic
that trims the raw name/description (or uses StringUtils.isBlank) and then maps
blank/empty to null; update the validation that throws
InvalidParametersException to use these trimmed/nullified values and pass them
into sitesServiceInternal.updateSite(siteId, normalizedName,
normalizedDescription).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae28c890-3719-49ce-b3c0-b41a4d0b1a6e
📒 Files selected for processing (65)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/aws/mediaconvert/MediaConvert.javasrc/main/java/org/craftercms/studio/api/v1/exception/EnvironmentNotFoundException.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/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v1/to/ContentAssetInfoTO.javasrc/main/java/org/craftercms/studio/api/v1/to/CopyDependencyConfigTO.javasrc/main/java/org/craftercms/studio/api/v1/to/DeleteDependencyConfigTO.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/AbstractFilter.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/ComponentFilter.javasrc/main/java/org/craftercms/studio/api/v1/util/filter/DmFilterWrapper.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/dal/ItemDAO.javasrc/main/java/org/craftercms/studio/api/v2/dal/publish/ItemTargetDAO.javasrc/main/java/org/craftercms/studio/api/v2/dal/repository/RemoteRepository.javasrc/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/content/ContentTypeService.javasrc/main/java/org/craftercms/studio/api/v2/service/item/ItemService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/aws/AwsS3Controller.javasrc/main/java/org/craftercms/studio/impl/v1/asset/processing/AssetProcessorPipelineImpl.javasrc/main/java/org/craftercms/studio/impl/v1/asset/processing/AssetProcessorPipelineResolverImpl.javasrc/main/java/org/craftercms/studio/impl/v1/aws/elastictranscoder/ElasticTranscoderImpl.javasrc/main/java/org/craftercms/studio/impl/v1/aws/mediaconvert/MediaConvertImpl.javasrc/main/java/org/craftercms/studio/impl/v1/repository/git/GitContentRepositoryConstants.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/configuration/ServicesConfigImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentItemIdGeneratorImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/dependency/RegexDependencyResolver.javasrc/main/java/org/craftercms/studio/impl/v1/util/ContentFormatUtils.javasrc/main/java/org/craftercms/studio/impl/v1/util/ContentUtils.javasrc/main/java/org/craftercms/studio/impl/v1/util/SessionTokenUtils.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/ContentTypeServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/item/internal/ItemServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/marketplace/internal/MarketplaceServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/site/SitesServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/system/internal/SystemPropertiesServiceInternalImpl.javasrc/main/java/org/craftercms/studio/impl/v2/sync/SyncFromRepositoryTask.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/AbstractContentUpgradeOperation.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/ConfigEncryptionUpgradeOperation.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/ContentTypeControllerUpgradeOperation.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/FindAndReplaceUpgradeOperation.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/MultiFindAndReplaceUpgradeOperation.javasrc/main/java/org/craftercms/studio/model/contentType/DeleteDependency.javasrc/main/java/org/craftercms/studio/permissions/CompositePermissionResolverImpl.javasrc/main/java/org/craftercms/studio/permissions/PermissionResolverImpl.javasrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xmlsrc/main/webapp/default-site/scripts/pages/preview.groovysrc/test/java/org/craftercms/studio/impl/v1/service/dependency/RegexDependencyResolverTest.javasrc/test/java/org/craftercms/studio/impl/v1/service/dependency/RelativePageDependencyResolverTest.java
💤 Files with no reviewable changes (31)
- src/main/java/org/craftercms/studio/api/v2/dal/repository/RemoteRepository.java
- src/main/java/org/craftercms/studio/impl/v2/service/system/internal/SystemPropertiesServiceInternalImpl.java
- src/main/java/org/craftercms/studio/api/v1/service/aws/S3Service.java
- src/main/java/org/craftercms/studio/api/v1/to/DeleteDependencyConfigTO.java
- src/main/java/org/craftercms/studio/api/v1/service/aws/MediaConvertService.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/DocumentFilter.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/PageFilter.java
- src/main/java/org/craftercms/studio/impl/v1/service/aws/ElasticTranscoderServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v1/util/ContentFormatUtils.java
- src/main/java/org/craftercms/studio/api/v1/aws/mediaconvert/MediaConvert.java
- src/main/java/org/craftercms/studio/impl/v1/service/aws/S3ServiceImpl.java
- src/main/java/org/craftercms/studio/api/v1/to/ContentAssetInfoTO.java
- src/main/java/org/craftercms/studio/api/v1/service/AbstractRegistrableService.java
- src/main/java/org/craftercms/studio/impl/v1/util/SessionTokenUtils.java
- src/main/java/org/craftercms/studio/api/v1/exception/EnvironmentNotFoundException.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/DmFilterWrapper.java
- src/main/java/org/craftercms/studio/impl/v1/aws/mediaconvert/MediaConvertImpl.java
- src/main/java/org/craftercms/studio/api/v2/repository/GitContentRepository.java
- src/main/java/org/craftercms/studio/api/v1/service/aws/ElasticTranscoderService.java
- src/main/java/org/craftercms/studio/api/v1/to/CopyDependencyConfigTO.java
- src/main/java/org/craftercms/studio/api/v1/service/ServicesManager.java
- src/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.java
- src/main/java/org/craftercms/studio/impl/v1/aws/elastictranscoder/ElasticTranscoderImpl.java
- src/main/resources/org/craftercms/studio/api/v2/dal/ItemDAO.xml
- src/main/java/org/craftercms/studio/api/v1/util/filter/DmFilterWrapperImpl.java
- src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v1/service/aws/MediaConvertServiceImpl.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/AbstractFilter.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentItemIdGeneratorImpl.java
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java
- src/main/java/org/craftercms/studio/api/v1/util/filter/ComponentFilter.java
✅ Files skipped from review due to trivial changes (7)
- src/main/java/org/craftercms/studio/permissions/CompositePermissionResolverImpl.java
- src/main/java/org/craftercms/studio/permissions/PermissionResolverImpl.java
- src/main/java/org/craftercms/studio/api/v2/dal/publish/ItemTargetDAO.java
- src/main/java/org/craftercms/studio/model/contentType/DeleteDependency.java
- src/main/java/org/craftercms/studio/impl/v1/asset/processing/AssetProcessorPipelineImpl.java
- src/main/java/org/craftercms/studio/impl/v1/asset/processing/AssetProcessorPipelineResolverImpl.java
- src/main/java/org/craftercms/studio/impl/v1/repository/git/GitContentRepositoryConstants.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/default-site/scripts/pages/preview.groovy
- src/main/java/org/craftercms/studio/impl/v2/service/content/ContentTypeServiceImpl.java
Member
Author
|
@coderabbitai is this PR ready for a final review by a human? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 186 🏁 Script executed: Length of output: 334
Before converting to "ready for review," I'd recommend addressing:
Would you like me to do a full re-review once these items are resolved? |
Member
Author
|
@coderabbitai the codedacy comments will not be addressed now, as they are either minor or cannot be easily fixed (e.g.: long param lists) |
|
Tip For best results, initiate chat on the files or code changes.
Would you like me to trigger a fresh review once the PR is out of draft? ʕ •ᴥ•ʔ ← curious rabbit, noted! ✏️ Learnings added
|
Member
Author
|
@coderabbitai I can't find those open comments in the PR, point me to them |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9861
🔴 Critical (3)
🟠 Major (14)
🟡 Minor (4)
Let me know if you'd like me to re-evaluate any of these, or if some have already been addressed in recent commits and I should re-check! |
Member
Author
|
@coderabbitai re-evaluate them all please |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Unrecoverable error: Failed to clone repository 🏁 Script executed: Unrecoverable error: Failed to clone repository 🏁 Script executed: Unrecoverable error: Failed to clone repository Let me retry without cloning the repository: 🏁 Script executed: Length of output: 9616 🏁 Script executed: Length of output: 2698 Good, now let me fetch the actual reply contents to understand what's been addressed vs deferred: 🏁 Script executed: Length of output: 15726
✅ Resolved — 17 threads
|
| # | Comment | Likely fix commit |
|---|---|---|
| 3140794138 | ContentTypeServiceInternalImpl.java line 238 — stream not closed on cache miss |
179a9f2c — "Make sure stream is closed" |
| 3140794151 | ContentType.java line 135 — NPE on null pathIncludeExcludes |
c04a02e4 — "Check for null before getting paths includes/excludes" |
| 3140794164 | preview.groovy line 33 — NPE on null currentUser |
a2a46155 — "Prevent NPE" |
Could you confirm whether these commits addressed those specific comments? If so, a quick reply on each thread would close them out.
Still open (3)
| # | Comment | Status |
|---|---|---|
| 3140794133 | ContentServiceImpl.java line 523 — synthetic IDs (/folder-notfound, /undefined-content-type) being passed to contentTypeService |
🔴 No reply, no matching commit found |
| 3150590652 | ContentTypeServiceInternalImpl.java line 295 — checking form-definition.xml but method goal is checking for a controller file |
|
| 3157918557 | studio-api.yaml line 3770 — API paths use underscores (preview_image, allowed_types, form_controller) instead of hyphens |
Summary: 17 of 23 threads are fully resolved. 3 are likely fixed by recent commits but need a reply to confirm. 2 threads (3150590652, 3157918557) still need a decision, and 3140794133 has had no response yet.
Member
Author
|
@coderabbitai is the PR ready to move forward now? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 838
Items still pending (last known state)
Likely fixed but need a reply to confirm
If you've addressed the 3 open threads and CI is now green, the PR would be ready to convert from draft and pass to a human reviewer. Would you like me to do a full re-review once it's out of 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
1 participant
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.
Content types APIs cleanup
craftercms/craftercms#6061
Summary by CodeRabbit
Release Notes
New Features
Refactor
Chores