Content types APIs v2#3913
Merged
sumerjabri merged 17 commits intocraftercms:developfrom May 6, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoved legacy v1 Groovy content-type endpoints and wrappers; added v2 ContentType REST controller, service API methods and models; introduced a content-type config merge upgrader (XSLT + pipeline), refactored upgrade helpers, updated many repo-bootstrap content-type configs/forms, and added tests and wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as "ContentTypeController\n(API v2)"
participant Service as "ContentTypeService"
participant Internal as "ContentTypeServiceInternal"
participant Repo as "Repository/Storage"
Client->>Controller: HTTP GET /api/2/configuration/content-type/{siteId} or /allowed_types
Controller->>Service: getContentType(siteId, contentTypeId)\nor getAllowedContentTypes(siteId, path)
Service->>Internal: delegate call
Internal->>Repo: read form-definition.xml (and site config if merged)
Repo-->>Internal: return file/config data
Internal-->>Service: ContentType object / allowed types
Service-->>Controller: ResultOne / ResultList
Controller-->>Client: HTTP 200 JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 4026-4036: The OpenAPI spec for operationId getAllowedContentTypes
mentions "at a given path" but only declares siteId; align the contract by
either adding a path parameter (e.g., query param named path) and implementing a
matching v2 handler that calls the service (or a new service method) analogous
to v1's getAllowedContentTypesForPath(site, path), or remove the "at a given
path" text from the summary/description to reflect site-scoped behavior and
ensure the operationId maps to the existing v2 service method (or create a v2
method that accepts both site and path if you choose the path-parameter
approach). Ensure the parameter name matches the service signature and update
any related request/handler code to accept and validate the new path parameter.
🪄 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: 11bc82e1-b22e-4960-90f5-99272b24b884
📒 Files selected for processing (6)
src/main/api/studio-api.yamlsrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/ContentTypeServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentTypeServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-content-type.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-content-types.get.groovy
💤 Files with no reviewable changes (5)
- src/main/webapp/default-site/scripts/rest/api/1/content/get-content-types.get.groovy
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-content-type.get.groovy
- src/main/webapp/default-site/scripts/api/ContentTypeServices.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentTypeServices.groovy
jmendeza
force-pushed
the
feature/6061-content-types
branch
3 times, most recently
from
907f4b2 to
34bedf9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 4037-4063: The operation getContentTypeConfiguration currently
declares query parameter contentTypeId as required and returns a single
contentType object, but the summary promises "all content types" when
contentTypeId is omitted; change the operation to reflect the "get all" behavior
by making the query parameter contentTypeId required: false (optional) and
update the 200 response payload to return contentTypes as an array of
ContentType (schema: type: array, items: $ref:
'#/components/schemas/ContentType') instead of a single contentType object so
parameter and response shape match the documented "get all" behavior.
In
`@src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.java`:
- Around line 53-56: The testUpgrade method is a no-op and should either be
implemented or removed; add an arrange/act/assert flow that constructs the
ContentTypeConfigMergeUpgrader (or obtains the existing "upgrader" fixture),
prepare input content-type config(s) and an UpgradeContext, call
upgrader.upgrade(...) (the act), and assert the expected transformed config or
side-effects (e.g., returned result, modified files, or logged messages) using
assertions; alternatively, if the behavior isn’t ready, delete this placeholder
test to avoid false positives. Ensure you reference the test method name
testUpgrade and the upgrader.upgrade(...) call when making the change so CI
reflects real verification.
🪄 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: 7ea37039-30e8-4382-8076-903fe8bdfae1
📒 Files selected for processing (11)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v2/utils/StudioUtils.javasrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/ContentTypeServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentTypeServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-content-type.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-content-types.get.groovysrc/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.javasrc/test/java/org/craftercms/studio/impl/v2/upgrade/operations/site/ContentTypeControllerUpgradeOperationTest.javasrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0/custom/expected.groovysrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0/custom/input.groovy
💤 Files with no reviewable changes (5)
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-content-type.get.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-content-types.get.groovy
- src/main/webapp/default-site/scripts/api/ContentTypeServices.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentTypeServices.groovy
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/site/ContentTypeControllerUpgradeOperationTest.java
jmendeza
force-pushed
the
feature/6061-content-types
branch
from
8350193 to
113158f
Compare
|
Only repository collaborators, contributors, or members can run CodeRabbit commands. |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/BatchXsltFileUpgradeOperation.java (1)
82-102:⚠️ Potential issue | 🟠 MajorPer-file failures are swallowed — the upgrade reports success even when every file fails.
Inside
paths.forEach, anyExceptionfromexecuteTemplate/replaceFileis caught and only logged (line 95-97). If the XSLT is broken or I/O fails for every match,doExecutestill completes normally and the upgrade framework marks the step as successful, leaving the repository silently un-migrated. Consider accumulating failures and rethrowing anUpgradeExceptionat the end (or at least after the first failure, gated by config), so the upgrade manager can surface/retry.🤖 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/site/BatchXsltFileUpgradeOperation.java` around lines 82 - 102, The per-file exceptions inside BatchXsltFileUpgradeOperation are currently logged and swallowed in the paths.forEach lambda (calls to executeTemplate and replaceFile), causing doExecute to succeed even when all files fail; change the implementation to collect failures (e.g., a thread-safe List<Exception> or boolean flag) while iterating getPaths(repository) and after the stream completes throw a new UpgradeException (including details or the first/aggregated exceptions) if any failures occurred so the upgrade framework sees the error instead of reporting success.
♻️ Duplicate comments (1)
src/main/api/studio-api.yaml (1)
4037-4063:⚠️ Potential issue | 🟠 MajorResolve the content type configuration contract mismatch.
Line 4037 says omitting
contentTypeIdreturns all content types, but Lines 4047-4049 require it and Lines 4062-4063 return a singlecontentType. Either make the parameter optional and return an array, or narrow the summary to single-content-type behavior.🛠️ Minimal doc fix if this endpoint only returns one content type
- summary: Get content type configuration file for a given content type. It gets all content types if no contentTypeId is provided + summary: Get content type configuration for a given content type🤖 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 4037 - 4063, The OpenAPI contract for operationId getContentTypeConfiguration is inconsistent: parameter contentTypeId is marked required but summary says omitting it returns all content types and the response schema returns a single contentType; decide intended behavior and fix the spec accordingly—either make the query parameter contentTypeId optional and change the response schema to return an array (e.g., contentTypes: array of ContentType) or keep contentTypeId required and update the summary/description to state it returns a single contentType; update the parameter block for contentTypeId and the response schema (the contentType property) to match the chosen behavior so summary, parameters, and response are consistent.
🧹 Nitpick comments (3)
src/main/java/org/craftercms/studio/model/contentType/ContentType.java (1)
42-44: Consider@JsonPropertyhandling forUNKNOWN.If unknown content types can appear in serialized payloads, add
@JsonEnumDefaultValueonUNKNOWNso deserialization of unexpected inputs falls back gracefully rather than failing.🤖 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/ContentType.java` around lines 42 - 44, The ContentType.Type enum currently defines PAGE, COMPONENT, UNKNOWN; to ensure unknown serialized values deserialize safely, annotate the UNKNOWN enum constant with `@JsonEnumDefaultValue` (importing com.fasterxml.jackson.annotation.JsonEnumDefaultValue) so Jackson will map unexpected inputs to Type.UNKNOWN during deserialization; update the Type enum declaration (ContentType.Type) to add that annotation to the UNKNOWN constant and run tests to verify behavior.src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/BatchXsltFileUpgradeOperation.java (1)
111-115:replaceFilesilently drops the transformation when output is empty.If
Files.size(temp) == 0, the original file is left as-is with no log/warning. An empty XSLT result is almost always a bug (e.g., stylesheet didn't match the root); logging atwarnhere would make such misconfigurations diagnosable without changing behavior.Proposed fix
protected void replaceFile(Path path, Path temp) throws IOException { if (Files.size(temp) > 0) { Files.move(temp, path, StandardCopyOption.REPLACE_EXISTING); + } else { + logger.warn("XSLT produced empty output for '{}'; leaving original file unchanged", path); } }🤖 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/site/BatchXsltFileUpgradeOperation.java` around lines 111 - 115, The replaceFile method in BatchXsltFileUpgradeOperation currently drops the transformed temp file silently when Files.size(temp) == 0; update replaceFile(Path path, Path temp) to emit a warning (logger.warn or the class's logging facility) when the temp file is empty and include the target path and that the XSLT produced empty output, but keep the existing behavior of not replacing the original file; this makes empty transformation results discoverable without changing semantics.src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.java (1)
95-119: Assert the config file deletion side effect.The production upgrader deletes the sibling
config.xmlafter merging, but this test only verifies the transformed XML. Add an assertion so regressions in the cleanup behavior are caught too.Test coverage improvement
import static org.mockito.Mockito.*; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; @@ upgrader.doExecute(context); + assertFalse(Files.exists(configFilePath), "The merged config.xml should be deleted"); // Assert🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.java` around lines 95 - 119, The test currently runs upgrader.doExecute(context) and compares expectedXml vs actualXml but misses asserting the side effect of removing the sibling config.xml; after upgrader.doExecute(context) (or after the IO comparison block) add an assertion that the original sibling config file (the config.xml file next to the input/content type file—use the same File instance that was set up for expectedFile/resultFile or derive it from resultFile/expectedFile) no longer exists (e.g., assertFalse(configFile.exists()) or equivalent), so the test verifies the cleanup behavior of the upgrader.
🤖 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/java/org/craftercms/studio/controller/rest/v2/ContentTypeController.java`:
- Around line 70-82: The controller currently dereferences the ImmutablePair
returned by contentTypeService methods and causes a 500 when the service returns
null for missing optional resources; update getContentTypeFormController and
getContentTypePreviewImage (and the similar endpoints around lines 112-119) to
check the returned ImmutablePair for null (or a null Resource) and return
ResponseEntity.notFound().build() when absent, otherwise call
getResourceResponse(resource.getKey(), resource.getValue()); ensure
getResourceResponse is only invoked with non-null pair and resource to provide a
proper 404 for missing content-type resources.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/content/ContentTypeServiceImpl.java`:
- Around line 122-132: The two new methods in
ContentTypeServiceImpl—getContentType(`@SiteId` String siteId, String
contentTypeId) and getAllowedContentTypes(`@SiteId` String siteId, `@ContentPath`
String path)—are missing the `@RequireSiteReady` guard; add the `@RequireSiteReady`
annotation above both method declarations (keeping existing `@HasPermission` and
throws SiteNotFoundException on getContentType) so they follow the same
readiness check applied to other methods like getContentTypeUsage,
getContentTypePreviewImage, getContentTypeFormController and
getAllModelDefinitions and still delegate to contentTypeServiceInternal.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentTypeServiceInternalImpl.java`:
- Around line 162-172: The two stub methods in ContentTypeServiceInternalImpl
(getContentType and getAllowedContentTypes) should not return null/empty
silently; change their implementations to throw UnsupportedOperationException
(or another unchecked exception indicating "Not Implemented") so callers via
ContentTypeServiceImpl will surface a 501-like failure instead of returning
null/emptyList; update getContentType(String siteId, String contentTypeId) and
getAllowedContentTypes(String siteId, String path) to throw this exception with
a clear message mentioning the class and method names.
In
`@src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgrader.java`:
- Around line 59-69: The deletion of tracked config files (deletedFiles) in
doExecute currently runs irrespective of whether
executeTemplate/BatchXsltFileUpgradeOperation.replaceFile succeeded, which can
remove original config.xml on a failed replacement; change the flow so deletions
only occur after successful replaceFile calls: either (A) modify
BatchXsltFileUpgradeOperation.executeTemplate/replaceFile to throw or propagate
an exception on replacement failure and let doExecute delete files only after
executeTemplate completes without error, or (B) have the batch operation return
per-file success results (or populate a new list like successfullyReplacedFiles)
and move the Files.deleteIfExists(repositoryRoot.resolve(...)) loop to run only
over successfullyReplacedFiles; reference doExecute, executeTemplate,
BatchXsltFileUpgradeOperation, replaceFile, deletedFiles, repositoryRoot and
Files.deleteIfExists when making the change.
In `@src/main/java/org/craftercms/studio/model/contentType/ContentType.java`:
- Around line 27-45: ContentType currently has only protected fields and no
accessors, so Jackson will serialize it as an empty object; update the
ContentType class to expose its properties (e.g., add standard public
getters/setters for fields like id, label, type, allowedRoles,
deleteDependencies, copyDependencies, previewable, imageThumbnail, noThumbnail,
pathIncludes, pathExcludes, quickCreate, quickCreatePath), or alternatively
annotate the class with a Lombok annotation such as `@Data` (or make fields
public) or add a Jackson visibility annotation like
`@JsonAutoDetect`(fieldVisibility = JsonAutoDetect.Visibility.ANY) so GET
/api/2/configuration/content_types/{siteId} returns the actual fields; ensure
the Type enum remains unchanged and that the approach matches how
DeleteDependency/CopyDependency are exposed.
In
`@src/main/resources/crafter/studio/upgrade/5.0.x/content-type/content-type-merge-v5.0.0.2.xslt`:
- Around line 18-21: Update the header comment that lists which elements are
copied into the generated XSLT so it reflects the actual behavior: add
"noThumbnail" to the existing enumeration of previewable, paths,
delete-dependencies, copy-dependencies, and allowed-roles (the header comment
near the top of content-type-merge-v5.0.0.2.xslt) so the documentation stays in
sync with the template that copies the noThumbnail element.
- Around line 42-52: The current <xsl:template match="form"> blindly appends
document($configFileName)/content-type/* via the six <xsl:copy-of> calls
(previewable, noThumbnail, paths, delete-dependencies, copy-dependencies,
allowed-roles), which causes duplicates when those elements already exist; to
fix, suppress existing elements before appending by either adding empty
templates like <xsl:template match="form/previewable"/> (and similarly for
noThumbnail, paths, delete-dependencies, copy-dependencies, allowed-roles) so
the <xsl:apply-templates> step will not copy them, or change the append logic in
the <xsl:template match="form"> to conditionally copy from
document($configFileName) only when the target element is absent (e.g., use
tests like not(previewable) before performing the corresponding <xsl:copy-of>),
keeping references to the existing template name "form" and the
document($configFileName)/content-type/* copy-of calls.
In
`@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/header/form-definition.xml`:
- Around line 170-172: The imageManager property currently references undeclared
IDs; update the <name>imageManager</name> entry so it only uses the declared
image datasource IDs (existing_images and upload_image) or rename the declared
datasources to match (existingimages and uploadimage). Specifically, edit the
imageManager value to remove or replace the undeclared IDs (existingimages,
uploadimage) so it matches the actual datasource names (existing_images,
upload_image) in this XML form-definition.
In
`@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/article/form-definition.xml`:
- Around line 880-884: Remove the stray "-> MIGRATE" text from inside the
<paths> element so the start tag reads simply <paths>, and fix indentation of
the nested <includes> block (replace the tab with the project's standard spaces)
so the <includes> and <pattern> children are properly aligned; locate the
offending text near the <paths> element that contains the <includes> and the
<pattern> ^/site/website/articles/.* and update those lines accordingly.
In
`@src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/form-definition.xml`:
- Around line 221-225: The <pattern> value inside the <paths>/<includes> block
currently reads "/site/taxonomy/.*" which lacks the leading ^ anchor and is
inconsistent with the other patterns; update that pattern to
'^/site/taxonomy/.*' (the <pattern> element under the <paths> -> <includes>
section) so it uses the same anchored regex semantics as the other patterns.
In
`@src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/page/item-explorer/form-definition.xml`:
- Around line 119-124: The constraint with <name>required</name> currently uses
the wrong type—change the constraint's <type> from "int" to "boolean" so the
stored <value>true</value> is typed correctly; update the <constraint> element
that contains <name>required</name> under the <constraints> block (the required
validator entry) to use boolean type.
---
Outside diff comments:
In
`@src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/BatchXsltFileUpgradeOperation.java`:
- Around line 82-102: The per-file exceptions inside
BatchXsltFileUpgradeOperation are currently logged and swallowed in the
paths.forEach lambda (calls to executeTemplate and replaceFile), causing
doExecute to succeed even when all files fail; change the implementation to
collect failures (e.g., a thread-safe List<Exception> or boolean flag) while
iterating getPaths(repository) and after the stream completes throw a new
UpgradeException (including details or the first/aggregated exceptions) if any
failures occurred so the upgrade framework sees the error instead of reporting
success.
---
Duplicate comments:
In `@src/main/api/studio-api.yaml`:
- Around line 4037-4063: The OpenAPI contract for operationId
getContentTypeConfiguration is inconsistent: parameter contentTypeId is marked
required but summary says omitting it returns all content types and the response
schema returns a single contentType; decide intended behavior and fix the spec
accordingly—either make the query parameter contentTypeId optional and change
the response schema to return an array (e.g., contentTypes: array of
ContentType) or keep contentTypeId required and update the summary/description
to state it returns a single contentType; update the parameter block for
contentTypeId and the response schema (the contentType property) to match the
chosen behavior so summary, parameters, and response are consistent.
---
Nitpick comments:
In
`@src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/BatchXsltFileUpgradeOperation.java`:
- Around line 111-115: The replaceFile method in BatchXsltFileUpgradeOperation
currently drops the transformed temp file silently when Files.size(temp) == 0;
update replaceFile(Path path, Path temp) to emit a warning (logger.warn or the
class's logging facility) when the temp file is empty and include the target
path and that the XSLT produced empty output, but keep the existing behavior of
not replacing the original file; this makes empty transformation results
discoverable without changing semantics.
In `@src/main/java/org/craftercms/studio/model/contentType/ContentType.java`:
- Around line 42-44: The ContentType.Type enum currently defines PAGE,
COMPONENT, UNKNOWN; to ensure unknown serialized values deserialize safely,
annotate the UNKNOWN enum constant with `@JsonEnumDefaultValue` (importing
com.fasterxml.jackson.annotation.JsonEnumDefaultValue) so Jackson will map
unexpected inputs to Type.UNKNOWN during deserialization; update the Type enum
declaration (ContentType.Type) to add that annotation to the UNKNOWN constant
and run tests to verify behavior.
In
`@src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.java`:
- Around line 95-119: The test currently runs upgrader.doExecute(context) and
compares expectedXml vs actualXml but misses asserting the side effect of
removing the sibling config.xml; after upgrader.doExecute(context) (or after the
IO comparison block) add an assertion that the original sibling config file (the
config.xml file next to the input/content type file—use the same File instance
that was set up for expectedFile/resultFile or derive it from
resultFile/expectedFile) no longer exists (e.g.,
assertFalse(configFile.exists()) or equivalent), so the test verifies the
cleanup behavior of the upgrader.
🪄 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: 385fc781-6688-41c3-baba-3cd598423891
📒 Files selected for processing (81)
src/main/api/studio-api.yamlsrc/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/controller/rest/v2/ConfigurationController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentTypeController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ResultConstants.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/upgrade/operations/contentType/ContentTypeConfigMergeUpgrader.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/AbstractXsltFileUpgradeOperation.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/BatchXsltFileUpgradeOperation.javasrc/main/java/org/craftercms/studio/model/contentType/ContentType.javasrc/main/java/org/craftercms/studio/model/contentType/CopyDependency.javasrc/main/java/org/craftercms/studio/model/contentType/DeleteDependency.javasrc/main/java/org/craftercms/studio/model/rest/contentType/DeleteContentTypeRequest.javasrc/main/resources/crafter/studio/extension/rendering-overlay-context.xmlsrc/main/resources/crafter/studio/studio-upgrade-context.xmlsrc/main/resources/crafter/studio/upgrade/5.0.x/content-type/content-type-merge-v5.0.0.2.xsltsrc/main/resources/crafter/studio/upgrade/pipelines.yamlsrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/header/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/header/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/left-rail/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/left-rail/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/article/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/article/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/home/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/home/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/taxonomy/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/taxonomy/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/studio_version.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/page/catalog/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/page/catalog/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/studio_version.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/page/entry/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/page/entry/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/studio_version.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/post/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/post/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/page/item-explorer/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/page/item-explorer/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/taxonomy/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/taxonomy/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/studio_version.xmlsrc/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.javasrc/test/java/org/craftercms/studio/impl/v2/upgrade/operations/site/ContentTypeControllerUpgradeOperationTest.javasrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/config.xmlsrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/expected.xmlsrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/form-definition.xmlsrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0/custom/expected.groovysrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0/custom/input.groovy
💤 Files with no reviewable changes (25)
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/header/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/level-descriptor/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/page/item-explorer/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/left-rail/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/page/catalog/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/taxonomy/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/page/entry/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/taxonomy/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/article/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/component/level-descriptor/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/home/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/post/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/level-descriptor/config.xml
✅ Files skipped from review due to trivial changes (15)
- src/main/java/org/craftercms/studio/api/v2/utils/StudioUtils.java
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/studio_version.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/studio_version.xml
- src/main/java/org/craftercms/studio/model/contentType/CopyDependency.java
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/studio_version.xml
- src/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/studio_version.xml
- src/main/java/org/craftercms/studio/model/contentType/DeleteDependency.java
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/post/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/form-definition.xml
- src/main/java/org/craftercms/studio/model/rest/contentType/DeleteContentTypeRequest.java
- src/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/config.xml
- src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/site/ContentTypeControllerUpgradeOperationTest.java
jmendeza
force-pushed
the
feature/6061-content-types
branch
from
cc710fe to
1fa0080
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/main/api/studio-api.yaml (1)
4037-4062:⚠️ Potential issue | 🟠 MajorResolve the optional
contentTypeIdcontract before clients depend on it.This operation says omitting
contentTypeIdreturns all content types, but the backend method shown inContentTypeController.getContentTyperequirescontentTypeId, and the documented response is a singlecontentType. Either implement/document the get-all variant, or mark the query parameter required and remove the get-all text.🐛 Proposed spec fix for current backend behavior
- summary: Get content type configuration file for a given content type. It gets all content types if no contentTypeId is provided + summary: Get content type configuration file for a given content type ... - name: contentTypeId in: query + required: true schema: type: stringBased on learnings, v2 OpenAPI may keep forward-looking variants, but the interim behavior should be documented, such as
501 Not Implemented, when backend support is not complete.Run this read-only check to verify the backend parameter requirement and response key:
#!/bin/bash # Verify whether the backend currently supports missing contentTypeId. rg -n --type=java -C6 '@GetMapping\(SITE_ID\)|getContentType\s*\(' src/main/java/org/craftercms/studio/controller/rest/v2/ContentTypeController.java rg -n -C8 'operationId: getContentTypeConfiguration|name: contentTypeId|contentType:' src/main/api/studio-api.yaml🤖 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 4037 - 4062, The OpenAPI operation getContentTypeConfiguration currently claims omitting the query param contentTypeId returns all content types but the backend method ContentTypeController.getContentType requires contentTypeId and returns a single contentType; fix by aligning spec with implementation: either (A) mark the query parameter contentTypeId as required and remove the "get all" text and ensure the response schema returns a single ContentType object under contentType, or (B) implement the get-all behavior in ContentTypeController.getContentType (or add a new controller method) to accept a missing contentTypeId and return an array of ContentType (update the operationId/getContentTypeConfiguration response to contentType: { type: array, items: $ref: '#/components/schemas/ContentType' }); if you prefer a forward-looking placeholder, document missing backend support by returning 501 Not Implemented for the get-all variant in the spec.src/main/java/org/craftercms/studio/controller/rest/v2/ContentTypeController.java (1)
70-82:⚠️ Potential issue | 🟠 MajorReturn 404 for missing optional content-type resources.
ContentTypeService.getContentTypeFormController(...)is documented to returnnullwhen absent, but Lines 73-74 dereference the pair before checking it. Missing form controllers currently become a 500 instead of a clean 404; the shared helper should guard both resource endpoints.Null-safe resource response
`@GetMapping`(FORM_CONTROLLER) public ResponseEntity<Resource> getContentTypeFormController(`@ValidSiteId` `@RequestParam` String siteId, `@ValidConfigurationPath` `@RequestParam` String contentTypeId) throws ServiceLayerException { ImmutablePair<String, Resource> resource = contentTypeService.getContentTypeFormController(siteId, contentTypeId); - return getResourceResponse(resource.getKey(), resource.getValue()); + return getResourceResponse(resource); } @@ public ResponseEntity<Resource> getContentTypePreviewImage(`@ValidSiteId` `@RequestParam` String siteId, `@ValidConfigurationPath` `@RequestParam` String contentTypeId) throws ServiceLayerException { ImmutablePair<String, Resource> resource = contentTypeService.getContentTypePreviewImage(siteId, contentTypeId); - return getResourceResponse(resource.getKey(), resource.getValue()); + return getResourceResponse(resource); } @@ - private ResponseEntity<Resource> getResourceResponse(String name, Resource resource) { - String mimeType = StudioUtils.getMimeType(name); + private ResponseEntity<Resource> getResourceResponse(ImmutablePair<String, Resource> resource) { + if (resource == null || resource.getValue() == null) { + return ResponseEntity.notFound().build(); + } + + String mimeType = StudioUtils.getMimeType(resource.getKey()); + var response = ResponseEntity.ok(); + if (mimeType != null) { + response.header(HttpHeaders.CONTENT_TYPE, mimeType); + } - return ResponseEntity - .ok() - .header(HttpHeaders.CONTENT_TYPE, mimeType) - .body(resource); + return response.body(resource.getValue()); }Also applies to: 112-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/controller/rest/v2/ContentTypeController.java` around lines 70 - 82, The controller currently dereferences the ImmutablePair returned by contentTypeService.getContentTypeFormController(...) and getContentTypePreviewImage(...) without null checks, causing 500s for missing optional resources; update getContentTypeFormController and getContentTypePreviewImage handlers to first check the returned ImmutablePair for null and also ensure pair.getValue() (the Resource) is not null, and if either is null return a 404 ResponseEntity (e.g. ResponseEntity.notFound().build()) instead of calling getResourceResponse; alternatively, update the shared helper getResourceResponse to be null-safe (accept null key/resource and return 404) and use that from both controller methods so both endpoints (and the duplicate block at the later lines) consistently return 404 for missing content-type resources.
🤖 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 3986: The OpenAPI spec routes are misnamed and don't match the
controller: update the paths in src/main/api/studio-api.yaml so they match the
controller's mapping "/api/2/configuration/content-type/{siteId}" and its
sub-path "/api/2/configuration/content-type/{siteId}/allowed_types" (replace the
plural "content-types" and the underscore "content_types" with the singular
hyphenated "content-type"); ensure the operation objects for the methods
currently defined under
"/api/2/configuration/content-types/{siteId}/allowed_types" and
"/api/2/configuration/content_types/{siteId}" are moved/renamed to the
controller-correct paths so generated clients call the backend routes the
controller actually serves.
In
`@src/main/java/org/craftercms/studio/api/v2/service/content/ContentTypeService.java`:
- Around line 125-132: The method signature for getAllowedContentTypes is
missing the declared SiteNotFoundException; update the declaration of
getAllowedContentTypes to include "throws SiteNotFoundException" to match
getContentType and reflect that `@RequireSiteReady`'s
sitesService.checkSiteState() can throw this exception so callers can handle it
explicitly.
In
`@src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/form-definition.xml`:
- Around line 402-406: Replace the current
<paths><excludes><pattern>^/site/.*</pattern></excludes></paths> block so it no
longer blocks all /site/ paths; instead use an <includes> block with a pattern
that matches the intended site subtree (e.g. change to
<paths><includes><pattern>^/site/website/.*</pattern></includes></paths>) so the
search-results page content-type follows the blueprint convention and allows
creation under /site/website/...
In
`@src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/page/catalog/form-definition.xml`:
- Around line 120-125: The `required` constraint in the identified <constraint>
block currently uses <type>int</type>; change it to <type>boolean</type> to
match the other `required` constraints in this file (look for the <constraint>
elements where <name>required</name> and adjust the <type> node accordingly) so
all `required` entries consistently use boolean.
---
Duplicate comments:
In `@src/main/api/studio-api.yaml`:
- Around line 4037-4062: The OpenAPI operation getContentTypeConfiguration
currently claims omitting the query param contentTypeId returns all content
types but the backend method ContentTypeController.getContentType requires
contentTypeId and returns a single contentType; fix by aligning spec with
implementation: either (A) mark the query parameter contentTypeId as required
and remove the "get all" text and ensure the response schema returns a single
ContentType object under contentType, or (B) implement the get-all behavior in
ContentTypeController.getContentType (or add a new controller method) to accept
a missing contentTypeId and return an array of ContentType (update the
operationId/getContentTypeConfiguration response to contentType: { type: array,
items: $ref: '#/components/schemas/ContentType' }); if you prefer a
forward-looking placeholder, document missing backend support by returning 501
Not Implemented for the get-all variant in the spec.
In
`@src/main/java/org/craftercms/studio/controller/rest/v2/ContentTypeController.java`:
- Around line 70-82: The controller currently dereferences the ImmutablePair
returned by contentTypeService.getContentTypeFormController(...) and
getContentTypePreviewImage(...) without null checks, causing 500s for missing
optional resources; update getContentTypeFormController and
getContentTypePreviewImage handlers to first check the returned ImmutablePair
for null and also ensure pair.getValue() (the Resource) is not null, and if
either is null return a 404 ResponseEntity (e.g.
ResponseEntity.notFound().build()) instead of calling getResourceResponse;
alternatively, update the shared helper getResourceResponse to be null-safe
(accept null key/resource and return 404) and use that from both controller
methods so both endpoints (and the duplicate block at the later lines)
consistently return 404 for missing content-type resources.
🪄 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: 7e72b1b9-c784-44ad-8ae1-f5742d36db8b
📒 Files selected for processing (86)
src/main/api/studio-api.yamlsrc/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/controller/rest/v2/ConfigurationController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentTypeController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ResultConstants.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/upgrade/operations/contentType/ContentTypeConfigMergeUpgrader.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/AbstractXsltFileUpgradeOperation.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/BatchXsltFileUpgradeOperation.javasrc/main/java/org/craftercms/studio/model/contentType/ContentType.javasrc/main/java/org/craftercms/studio/model/contentType/CopyDependency.javasrc/main/java/org/craftercms/studio/model/contentType/DeleteDependency.javasrc/main/java/org/craftercms/studio/model/rest/contentType/DeleteContentTypeRequest.javasrc/main/resources/crafter/studio/extension/rendering-overlay-context.xmlsrc/main/resources/crafter/studio/studio-upgrade-context.xmlsrc/main/resources/crafter/studio/upgrade/5.0.x/content-type/content-type-merge-v5.0.0.2.xsltsrc/main/resources/crafter/studio/upgrade/pipelines.yamlsrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/ContentTypeServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentTypeServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-content-type.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-content-types.get.groovysrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/header/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/header/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/left-rail/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/left-rail/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/article/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/article/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/home/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/home/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/taxonomy/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/taxonomy/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/studio_version.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/page/catalog/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/page/catalog/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/studio_version.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/page/entry/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/page/entry/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/studio_version.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/post/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/post/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/page/item-explorer/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/page/item-explorer/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/taxonomy/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/taxonomy/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/studio_version.xmlsrc/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.javasrc/test/java/org/craftercms/studio/impl/v2/upgrade/operations/site/ContentTypeControllerUpgradeOperationTest.javasrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/config.xmlsrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/expected.xmlsrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/form-definition.xmlsrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0/custom/expected.groovysrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0/custom/input.groovy
💤 Files with no reviewable changes (30)
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/page/catalog/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/home/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/taxonomy/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/post/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/level-descriptor/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/article/config.xml
- src/main/webapp/default-site/scripts/rest/api/1/content/get-content-type.get.groovy
- src/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/component/level-descriptor/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/page/entry/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/config.xml
- src/main/webapp/default-site/scripts/rest/api/1/content/get-content-types.get.groovy
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/left-rail/config.xml
- src/main/webapp/default-site/scripts/api/ContentTypeServices.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentTypeServices.groovy
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/page/item-explorer/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/taxonomy/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/level-descriptor/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/header/config.xml
✅ Files skipped from review due to trivial changes (19)
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/studio_version.xml
- src/main/java/org/craftercms/studio/api/v2/utils/StudioUtils.java
- src/main/java/org/craftercms/studio/model/contentType/DeleteDependency.java
- src/main/java/org/craftercms/studio/model/contentType/CopyDependency.java
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/studio_version.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/studio_version.xml
- src/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/studio_version.xml
- src/main/java/org/craftercms/studio/model/contentType/ContentType.java
- src/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/post/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/level-descriptor/form-definition.xml
- src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java
- src/main/java/org/craftercms/studio/model/rest/contentType/DeleteContentTypeRequest.java
- src/main/resources/crafter/studio/upgrade/pipelines.yaml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/home/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/page/item-explorer/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/left-rail/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/form-definition.xml
🚧 Files skipped from review as they are similar to previous changes (23)
- src/main/java/org/craftercms/studio/controller/rest/v2/ResultConstants.java
- src/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/component/level-descriptor/form-definition.xml
- src/main/resources/crafter/studio/studio-upgrade-context.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/taxonomy/form-definition.xml
- src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/AbstractXsltFileUpgradeOperation.java
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/level-descriptor/form-definition.xml
- src/main/java/org/craftercms/studio/impl/v2/service/content/ContentTypeServiceImpl.java
- src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/site/ContentTypeControllerUpgradeOperationTest.java
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/form-definition.xml
- src/main/resources/crafter/studio/upgrade/5.0.x/content-type/content-type-merge-v5.0.0.2.xslt
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/form-definition.xml
- src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.java
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentTypeServiceInternalImpl.java
- src/main/webapp/default-site/scripts/api/ServiceFactory.groovy
- src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgrader.java
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/article/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/taxonomy/form-definition.xml
- src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/BatchXsltFileUpgradeOperation.java
jmendeza
force-pushed
the
feature/6061-content-types
branch
from
1fa0080 to
8c2daf5
Compare
jmendeza
force-pushed
the
feature/6061-content-types
branch
from
8c2daf5 to
23bfed3
Compare
jmendeza
force-pushed
the
feature/6061-content-types
branch
from
23bfed3 to
efeed13
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/main/java/org/craftercms/studio/api/v2/service/content/ContentTypeService.java (1)
125-132:⚠️ Potential issue | 🟡 MinorDeclare
SiteNotFoundExceptionongetAllowedContentTypes.The implementation is guarded by
@RequireSiteReady, so this interface method should expose the same site-validation failure contract asgetContentType.Suggested fix
- Collection<String> getAllowedContentTypes(String siteId, String path); + Collection<String> getAllowedContentTypes(String siteId, String path) throws SiteNotFoundException;🤖 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 125 - 132, The getAllowedContentTypes method should declare the same site-validation exception as getContentType; update the ContentTypeService.getAllowedContentTypes signature to throw SiteNotFoundException (matching the `@RequireSiteReady` guarded contract) and adjust its javadoc to document the thrown SiteNotFoundException so callers know the site-not-ready failure mode.src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgrader.java (1)
58-79:⚠️ Potential issue | 🟠 MajorDelete
config.xmlonly after the form file replacement succeeds.
trackDeletedFiles()still runs insideexecuteTemplate(), beforeBatchXsltFileUpgradeOperationperformsreplaceFile(...). Since the parent batch loop logs and swallows per-file replacement failures,super.doExecute(context)can return normally and Line 65 can deleteconfig.xmleven though the transformedform-definition.xmlwas never installed.Move tracking/deletion onto a post-replacement success path, or make the parent batch operation propagate replacement failures before this cleanup runs.
🤖 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 58 - 79, The current code calls trackDeletedFiles(...) inside executeTemplate(), which runs before the parent replacement logic and can mark config.xml for deletion even if replaceFile(...) fails; move the call so it only runs after a successful replacement. Remove trackDeletedFiles(...) from executeTemplate(...) and instead invoke it from the post-replacement success path (i.e., after BatchXsltFileUpgradeOperation.replaceFile(...) completes successfully) or by overriding the hook the parent uses to signal a successful replace; ensure doExecute(...) still performs the actual deletions only for entries tracked after successful replacements. Reference: ContentTypeConfigMergeUpgrader.executeTemplate, ContentTypeConfigMergeUpgrader.doExecute, trackDeletedFiles, and replaceFile.src/main/api/studio-api.yaml (1)
4033-4064:⚠️ Potential issue | 🟠 MajorAlign this operation with the implemented controller contract.
ContentTypeController.getContentTypeis mapped to/api/2/configuration/content-types/{siteId}, requirescontentTypeId, and returns a singlecontentTypepayload (ResultOne<ContentType>), but this spec documents/content_types/{siteId}, an optional query parameter, andcontentTypes: ContentType[]. Generated clients will call/deserialise the wrong contract.If the list-all variant is intentional but not implemented yet, document the interim behavior; otherwise match the current backend. Based on learnings, v2 OpenAPI specs may keep forward-looking variants only when the planned/interim behavior is documented.
🐛 Proposed spec fix to match the current controller
- /api/2/configuration/content_types/{siteId}: + /api/2/configuration/content-types/{siteId}: get: tags: - contentTypes - summary: Get content type configuration file for a given content type. It gets all content types if no contentTypeId is provided + summary: Get content type configuration file for a given content type description: 'Required permission "content_read"' operationId: getContentTypeConfiguration parameters: - name: siteId in: path description: site ID required: true schema: type: string - name: contentTypeId in: query + required: true schema: type: string responses: '200': description: OK content: application/json: schema: type: object properties: response: $ref: '#/components/schemas/ApiResponse' - contentTypes: - type: array - items: - $ref: '#/components/schemas/ContentType' + contentType: + $ref: '#/components/schemas/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 4033 - 4064, The OpenAPI operation does not match the controller: ContentTypeController.getContentType is implemented at /api/2/configuration/content-types/{siteId}, requires a contentTypeId and returns a single ResultOne<ContentType>, but the spec currently defines /api/2/configuration/content_types/{siteId} with an optional query param and an array response. Fix the spec by renaming the path to /api/2/configuration/content-types/{siteId}, remove the optional query parameter contentTypeId, add a required path or required parameter for contentTypeId as the controller expects, and change the 200 response schema to return a single contentType payload (ResultOne/ contentType) instead of contentTypes: ContentType[]; keep operationId as getContentType to match the controller.
🧹 Nitpick comments (2)
src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.java (1)
95-118: Assert theconfig.xmlcleanup side effect.The upgrader’s contract includes deleting merged
config.xmlfiles, but the test only verifies the transformed form XML. Add an assertion afterdoExecuteso regressions in the cleanup path are caught. Based on learnings,config.xmlis deleted byContentTypeConfigMergeUpgraderimmediately after the merge.Proposed assertion
upgrader.doExecute(context); + + assertFalse("The merged config.xml should be deleted", Files.exists(configFilePath)); // Assert🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.java` around lines 95 - 118, Add an assertion in ContentTypeConfigMergeUpgraderTest immediately after calling upgrader.doExecute(context) to verify the cleanup side effect: check that the merged config.xml file the upgrader should delete no longer exists (e.g., assertFalse(configFile.exists()) or equivalent), so failures in ContentTypeConfigMergeUpgrader's deletion step are caught; locate the test around the upgrader.doExecute(context) call and add the existence check for the same File reference used to represent the merged config prior to the merge.src/main/java/org/craftercms/studio/controller/rest/v2/ContentTypeController.java (1)
62-62: Avoidthrows Exceptionon the controller signature.
ContentTypeService.getContentTypeUsagealmost certainly declares narrower checked exceptions (e.g.ServiceLayerException/SiteNotFoundException, like the other endpoints in this controller). Propagating a bareExceptionhere defeats the project's exception-handling/advice layer and makes the API contract fuzzy — please narrow it to what the service actually throws.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/controller/rest/v2/ContentTypeController.java` at line 62, The controller method in ContentTypeController currently declares "throws Exception"; instead change its throws clause to match the specific checked exceptions declared by ContentTypeService.getContentTypeUsage (e.g. ServiceLayerException, SiteNotFoundException or whatever concrete exceptions that service method declares). Locate the controller method referencing getContentTypeUsage and replace the broad "throws Exception" with the exact exception types returned by ContentTypeService.getContentTypeUsage so the exception-handling/advice layer can operate correctly.
🤖 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/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/form-definition.xml`:
- Around line 272-304: The checkbox-group field with id "categories_o" is
pointing to the wrong datasource name ("categories"); update the datasource
property value to "categories_o" so it matches the declared datasource id for
that field (look for the <field> with id "categories_o" and change its
<property><name>datasource</name><value>…</value></property> accordingly); also
make the same correction for the other instance of the "categories_o" field
found later in the file.
In
`@src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.java`:
- Around line 77-83: The test currently uses a fixed shared path via
StudioUtils.getStudioTemporaryFilesRoot().resolve("config.xml") which can
collide across runs; change the setup to create a per-test temp directory (e.g.,
via Files.createTempDirectory) and then resolve both config and form files
inside that directory instead of using
StudioUtils.getStudioTemporaryFilesRoot(); update usages of configFilePath and
formFilePath to point to files created within that temp dir, call deleteOnExit
(or deleteRecursively in teardown) for the temp directory, and keep copying
originalFormFile and originalConfigFile into those per-test paths.
- Line 19: Remove the unused import
org.apache.commons.collections4.IterableUtils and replace the TestNG assertion
usage with the JUnit assertion: locate the test in
ContentTypeConfigMergeUpgraderTest where org.testng.Assert.assertEquals is
called and change that assertion to use JUnit's assertFalse with the same
boolean expression/condition; ensure you import the correct JUnit static method
if not already present and run the test compile to confirm the TestNG dependency
is no longer needed.
---
Duplicate comments:
In `@src/main/api/studio-api.yaml`:
- Around line 4033-4064: The OpenAPI operation does not match the controller:
ContentTypeController.getContentType is implemented at
/api/2/configuration/content-types/{siteId}, requires a contentTypeId and
returns a single ResultOne<ContentType>, but the spec currently defines
/api/2/configuration/content_types/{siteId} with an optional query param and an
array response. Fix the spec by renaming the path to
/api/2/configuration/content-types/{siteId}, remove the optional query parameter
contentTypeId, add a required path or required parameter for contentTypeId as
the controller expects, and change the 200 response schema to return a single
contentType payload (ResultOne/ contentType) instead of contentTypes:
ContentType[]; keep operationId as getContentType to match the controller.
In
`@src/main/java/org/craftercms/studio/api/v2/service/content/ContentTypeService.java`:
- Around line 125-132: The getAllowedContentTypes method should declare the same
site-validation exception as getContentType; update the
ContentTypeService.getAllowedContentTypes signature to throw
SiteNotFoundException (matching the `@RequireSiteReady` guarded contract) and
adjust its javadoc to document the thrown SiteNotFoundException so callers know
the site-not-ready failure mode.
In
`@src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgrader.java`:
- Around line 58-79: The current code calls trackDeletedFiles(...) inside
executeTemplate(), which runs before the parent replacement logic and can mark
config.xml for deletion even if replaceFile(...) fails; move the call so it only
runs after a successful replacement. Remove trackDeletedFiles(...) from
executeTemplate(...) and instead invoke it from the post-replacement success
path (i.e., after BatchXsltFileUpgradeOperation.replaceFile(...) completes
successfully) or by overriding the hook the parent uses to signal a successful
replace; ensure doExecute(...) still performs the actual deletions only for
entries tracked after successful replacements. Reference:
ContentTypeConfigMergeUpgrader.executeTemplate,
ContentTypeConfigMergeUpgrader.doExecute, trackDeletedFiles, and replaceFile.
---
Nitpick comments:
In
`@src/main/java/org/craftercms/studio/controller/rest/v2/ContentTypeController.java`:
- Line 62: The controller method in ContentTypeController currently declares
"throws Exception"; instead change its throws clause to match the specific
checked exceptions declared by ContentTypeService.getContentTypeUsage (e.g.
ServiceLayerException, SiteNotFoundException or whatever concrete exceptions
that service method declares). Locate the controller method referencing
getContentTypeUsage and replace the broad "throws Exception" with the exact
exception types returned by ContentTypeService.getContentTypeUsage so the
exception-handling/advice layer can operate correctly.
In
`@src/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.java`:
- Around line 95-118: Add an assertion in ContentTypeConfigMergeUpgraderTest
immediately after calling upgrader.doExecute(context) to verify the cleanup side
effect: check that the merged config.xml file the upgrader should delete no
longer exists (e.g., assertFalse(configFile.exists()) or equivalent), so
failures in ContentTypeConfigMergeUpgrader's deletion step are caught; locate
the test around the upgrader.doExecute(context) call and add the existence check
for the same File reference used to represent the merged config prior to the
merge.
🪄 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: edb3fe2f-22f0-4d5a-9bcf-41c988cc01af
📒 Files selected for processing (77)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v2/service/content/ContentTypeService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ConfigurationController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentTypeController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ResultConstants.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/upgrade/operations/contentType/ContentTypeConfigMergeUpgrader.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/AbstractXsltFileUpgradeOperation.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/BatchXsltFileUpgradeOperation.javasrc/main/java/org/craftercms/studio/model/contentType/ContentType.javasrc/main/java/org/craftercms/studio/model/contentType/CopyDependency.javasrc/main/java/org/craftercms/studio/model/contentType/DeleteDependency.javasrc/main/java/org/craftercms/studio/model/rest/contentType/DeleteContentTypeRequest.javasrc/main/resources/crafter/studio/extension/rendering-overlay-context.xmlsrc/main/resources/crafter/studio/studio-upgrade-context.xmlsrc/main/resources/crafter/studio/upgrade/5.0.x/content-type/content-type-merge-v5.0.0.2.xsltsrc/main/resources/crafter/studio/upgrade/pipelines.yamlsrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/header/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/header/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/left-rail/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/left-rail/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/article/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/article/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/home/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/home/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/taxonomy/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/taxonomy/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/studio_version.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/page/catalog/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/page/catalog/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/studio_version.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/page/entry/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/page/entry/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/studio_version.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/level-descriptor/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/level-descriptor/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/post/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/post/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/page/item-explorer/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/page/item-explorer/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/taxonomy/config.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/taxonomy/form-definition.xmlsrc/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/studio_version.xmlsrc/test/java/org/craftercms/studio/impl/v2/upgrade/operations/contentType/ContentTypeConfigMergeUpgraderTest.javasrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/config.xmlsrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/expected.xmlsrc/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/form-definition.xml
💤 Files with no reviewable changes (25)
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/page/catalog/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/product/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/home/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/component/level-descriptor/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/taxonomy/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/category-landing/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/post/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/page/item-explorer/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/level-descriptor/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/left-rail/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/header/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/level-descriptor/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/feature/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/contact-widget/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/page/entry/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/article/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/config.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/taxonomy/config.xml
✅ Files skipped from review due to trivial changes (16)
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/studio_version.xml
- src/main/java/org/craftercms/studio/model/contentType/CopyDependency.java
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/author/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/studio_version.xml
- src/main/java/org/craftercms/studio/model/contentType/DeleteDependency.java
- src/main/resources/crafter/studio/studio-upgrade-context.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/studio_version.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/studio_version.xml
- src/main/java/org/craftercms/studio/controller/rest/v2/ResultConstants.java
- src/main/java/org/craftercms/studio/impl/v2/service/content/ContentTypeServiceImpl.java
- src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/page/search-results/form-definition.xml
- src/main/java/org/craftercms/studio/model/contentType/ContentType.java
- src/test/resources/crafter/studio/upgrade/content-type/5.0/5.0.0.2/config.xml
- src/main/java/org/craftercms/studio/model/rest/contentType/DeleteContentTypeRequest.java
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/social-media-widget/form-definition.xml
🚧 Files skipped from review as they are similar to previous changes (13)
- src/main/webapp/default-site/scripts/api/ServiceFactory.groovy
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/level-descriptor/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/taxonomy/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/4000_empty/config/studio/content-types/page/entry/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/level-descriptor/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/taxonomy/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/1000_website_editorial/config/studio/content-types/component/articles-widget/form-definition.xml
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/page/catalog/form-definition.xml
- src/main/java/org/craftercms/studio/impl/v2/upgrade/operations/site/BatchXsltFileUpgradeOperation.java
- src/main/webapp/repo-bootstrap/global/blueprints/5000_headless_blog/config/studio/content-types/component/level-descriptor/form-definition.xml
- src/main/resources/crafter/studio/upgrade/pipelines.yaml
- src/main/resources/crafter/studio/upgrade/5.0.x/content-type/content-type-merge-v5.0.0.2.xslt
- src/main/webapp/repo-bootstrap/global/blueprints/2000_headless_store/config/studio/content-types/component/company/form-definition.xml
Member
|
@rart please review the OAS file spec for the new API. |
sumerjabri
approved these changes
Apr 26, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Content types APIs v2
craftercms/craftercms#6061
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests