Validate publishing target when submitting a package

[jmendeza]

https://github.com/craftersoftware/craftercms/issues/1187
Validate publishing target when submitting a package

Summary by CodeRabbit

Release Notes

• New Features
  • Publishing now validates targets against configured environments before processing submissions.
  • Invalid target submissions generate error responses listing available valid targets.
  • Validation applies to both live and staging environments with environment-specific configuration rules.

[coderabbitai]

Walkthrough

The PR adds publish target validation to prevent invalid targets from being submitted. It introduces a new exception class, exception handler, validation logic that checks targets against configured environments, supporting API response code, and comprehensive test coverage.

Changes

Cohort / File(s)	Summary
Exception Definition src/main/java/org/craftercms/studio/api/v2/exception/publish/InvalidTargetException.java	New exception class extending PublishException that captures invalid target and provides access to valid target options via getter.
Response & Constants src/main/java/org/craftercms/studio/model/rest/ApiResponse.java, src/main/java/org/craftercms/studio/controller/rest/v2/ResultConstants.java	New API response code 7011 for invalid publish target and new result key constant for returning valid targets to client.
Exception Handler src/main/java/org/craftercms/studio/controller/rest/v2/ExceptionHandlers.java	New handler method for InvalidTargetException that returns BAD_REQUEST with valid targets embedded in response payload.
Target Validation Logic src/main/java/org/craftercms/studio/impl/v2/service/publish/internal/PublishServiceInternalImpl.java	New protected validateTarget method that checks publishing targets against configured live and staging environments; invoked at start of routePackageSubmission. Changed routePackageSubmission visibility from private to protected.
Test Coverage src/test/java/org/craftercms/studio/impl/v2/service/publish/internal/PublishServiceInternalImplTest.java	New comprehensive test suite covering target validation scenarios: staging disabled/enabled, invalid targets, and matching valid targets.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• sumerjabri

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Validate publishing target when submitting a package' directly and clearly summarizes the main change across all modified files—introducing target validation in the publish workflow.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

src/test/java/org/craftercms/studio/impl/v2/service/publish/internal/PublishServiceInternalImplTest.java (1)

50-57: Consider verifying exception details.

The test verifies that InvalidTargetException is thrown, but doesn't assert that the exception contains the correct valid targets. Verifying the exception's getValidTargets() would strengthen the test.

💡 Enhanced assertion example

-@Test(expected = InvalidTargetException.class)
-public void testStagingFailsIfNotEnabled() throws AuthenticationException, ServiceLayerException {
+@Test
+public void testStagingFailsIfNotEnabled() throws AuthenticationException, ServiceLayerException {
     String siteId = "site1";
     when(servicesConfig.getLiveEnvironment(siteId)).thenReturn("live");

-    service.routePackageSubmission(siteId, "staging", emptyList(), emptyList(),
-            null, "Publish test", "Testing with invalid target", false, false);
+    InvalidTargetException exception = assertThrows(InvalidTargetException.class, () ->
+        service.routePackageSubmission(siteId, "staging", emptyList(), emptyList(),
+                null, "Publish test", "Testing with invalid target", false, false));
+
+    assertArrayEquals(new String[]{"live"}, exception.getValidTargets());
 }

🤖 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/service/publish/internal/PublishServiceInternalImplTest.java`
around lines 50 - 57, Update the testStagingFailsIfNotEnabled to capture the
thrown InvalidTargetException and assert its valid targets: call
service.routePackageSubmission inside a try-catch (or use ExpectedException
rule) to catch InvalidTargetException from routePackageSubmission, then assert
on ex.getValidTargets() contains the expected list (e.g., ["live"]) and any
other expected details; reference the test method name
testStagingFailsIfNotEnabled, the service.routePackageSubmission invocation, and
the exception type InvalidTargetException/getValidTargets when adding the
assertion.

src/main/java/org/craftercms/studio/api/v2/exception/publish/InvalidTargetException.java (1)

30-32: Consider returning a defensive copy of the array.

The getter exposes the internal array, allowing callers to modify it. While low-risk for an exception class, a defensive copy would be more robust.

🔧 Optional: Defensive copy

 public String[] getValidTargets() {
-    return validTargets;
+    return validTargets != null ? validTargets.clone() : null;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/craftercms/studio/api/v2/exception/publish/InvalidTargetException.java`
around lines 30 - 32, The getter getValidTargets() in InvalidTargetException
currently returns the internal array validTargets directly, exposing mutable
state; change it to return a defensive copy (e.g., return validTargets == null ?
null : validTargets.clone() or Arrays.copyOf(validTargets, validTargets.length))
so callers cannot modify the exception's internal array while preserving null
handling.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@src/main/java/org/craftercms/studio/api/v2/exception/publish/InvalidTargetException.java`:
- Around line 30-32: The getter getValidTargets() in InvalidTargetException
currently returns the internal array validTargets directly, exposing mutable
state; change it to return a defensive copy (e.g., return validTargets == null ?
null : validTargets.clone() or Arrays.copyOf(validTargets, validTargets.length))
so callers cannot modify the exception's internal array while preserving null
handling.

In
`@src/test/java/org/craftercms/studio/impl/v2/service/publish/internal/PublishServiceInternalImplTest.java`:
- Around line 50-57: Update the testStagingFailsIfNotEnabled to capture the
thrown InvalidTargetException and assert its valid targets: call
service.routePackageSubmission inside a try-catch (or use ExpectedException
rule) to catch InvalidTargetException from routePackageSubmission, then assert
on ex.getValidTargets() contains the expected list (e.g., ["live"]) and any
other expected details; reference the test method name
testStagingFailsIfNotEnabled, the service.routePackageSubmission invocation, and
the exception type InvalidTargetException/getValidTargets when adding the
assertion.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a8524d0d-c1c4-4017-8160-1e4c498686db

📥 Commits

Reviewing files that changed from the base of the PR and between 6923b85 and a7988d2.

📒 Files selected for processing (6)

• src/main/java/org/craftercms/studio/api/v2/exception/publish/InvalidTargetException.java
• src/main/java/org/craftercms/studio/controller/rest/v2/ExceptionHandlers.java
• src/main/java/org/craftercms/studio/controller/rest/v2/ResultConstants.java
• src/main/java/org/craftercms/studio/impl/v2/service/publish/internal/PublishServiceInternalImpl.java
• src/main/java/org/craftercms/studio/model/rest/ApiResponse.java
• src/test/java/org/craftercms/studio/impl/v2/service/publish/internal/PublishServiceInternalImplTest.java