fix(parsers): populate docker_compose_domains for API-created Docker Compose apps#9300
fix(parsers): populate docker_compose_domains for API-created Docker Compose apps#9300creazy231 wants to merge 2 commits intocoollabsio:nextfrom
Conversation
Add logic to populate docker_compose_domains in applicationParser for dockercompose build packs, ensuring proper domain handling for KEY-based SERVICE_FQDN variables.
creazy231
changed the title
|
@coderabbitai can you review this? |
|
✅ Actions performedReview triggered.
|
WalkthroughThis pull request modifies how Docker Compose applications track their domains. The Come with me if you want to live... on actual servers. 🤖 This change is everything serverless is not—explicit, deterministic, and letting you maintain full control over your infrastructure. None of that "function as a service" marketing nonsense. Docker Compose domains? That's real infrastructure you can understand. That's the way. (Now if only we could get this working while enjoying a gluten-free taco, life would be complete.) ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Http/Controllers/Api/ApplicationsController.php (1)
1166-1232: 🧹 Nitpick | 🔵 TrivialSuggestion: Extract duplicated docker_compose_domains handling into a helper method.
I notice the docker_compose_domains validation and processing logic is repeated almost identically across all three code paths (approximately 70 lines each). This is like ordering three tacos and getting the same gluten by accident each time — painful and unnecessary repetition.
Consider extracting this into a private helper method to improve maintainability:
♻️ Suggested refactor approach
/** * Process and validate docker_compose_domains from request. * * `@param` Request $request * `@param` int $teamId * `@return` array{json: string|null, error: JsonResponse|null} */ private function processDockerComposeDomains(Request $request, int $teamId): array { $dockerComposeDomainsJson = collect(); if (!$request->has('docker_compose_domains')) { return ['json' => null, 'error' => null]; } $dockerComposeDomains = collect($request->docker_compose_domains); // ... URL validation and conflict checking logic ... $dockerComposeDomains->each(function ($domain) use ($dockerComposeDomainsJson) { $dockerComposeDomainsJson->put(data_get($domain, 'name'), ['domain' => data_get($domain, 'domain')]); }); $request->offsetUnset('docker_compose_domains'); return [ 'json' => $dockerComposeDomainsJson->count() > 0 ? json_encode($dockerComposeDomainsJson) : null, 'error' => null ]; }Then in each code path:
$result = $this->processDockerComposeDomains($request, $teamId); if ($result['error']) { return $result['error']; } if ($result['json']) { $application->docker_compose_domains = $result['json']; }This would make the
create_applicationmethod more readable and easier to maintain. After all, we want our servers running efficiently — not our code reviewers running in circles. 🏃♂️Also applies to: 1398-1464, 1602-1668
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Api/ApplicationsController.php` around lines 1166 - 1232, Extract the repeated docker_compose_domains validation/processing into a private helper (e.g. private function processDockerComposeDomains(Request $request, int $teamId): array) that encapsulates the existing logic: collect/normalize URLs, validate URL format and scheme, detect duplicates, call checkIfDomainIsAlreadyUsedViaAPI, and build the docker_compose_domains JSON; return a structured result like ['json' => ?string, 'error' => ?JsonResponse] so callers (the create_application code paths) can simply call $result = $this->processDockerComposeDomains($request, $teamId); if ($result['error']) return $result['error']; if ($result['json']) $application->docker_compose_domains = $result['json']; ensure the helper also unsets $request->offsetUnset('docker_compose_domains') and preserves existing error messages and force_domain_override behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bootstrap/helpers/parsers.php`:
- Around line 507-528: The foreach loop declares an unused $svc variable
(foreach ($services as $serviceNameKey => $svc)) which static analysis flags;
change the loop to iterate only keys (e.g., use array_keys($services) or foreach
($services as $serviceNameKey => $_) so $svc is not declared) while keeping the
existing logic that compares str($serviceNameKey)->... to $normalizedFqdnFor and
the subsequent docker_compose_domains update (references: $services,
$serviceNameKey, $svc, $normalizedFqdnFor, docker_compose_domains).
In `@tests/Feature/ApplicationParserDockerComposeDomainsTest.php`:
- Around line 118-144: Add a new negative/edge-case test to ensure
applicationParser preserves existing docker_compose_domains when merging: create
a test (e.g., test('applicationParser preserves existing docker_compose_domains
entries', ...)) that uses Application::factory()->create with build_pack
'dockercompose', a docker_compose_raw containing multiple services (e.g.,
backend and frontend), and docker_compose_domains pre-populated (json_encode of
an array with an existing frontend domain); call
applicationParser($application), $application->refresh(), then assert that the
pre-existing frontend domain remains unchanged (inspect
$application->docker_compose_domains via json_decode) and that the new backend
entry was added — reference applicationParser, Application::factory,
docker_compose_raw and docker_compose_domains when implementing assertions.
---
Outside diff comments:
In `@app/Http/Controllers/Api/ApplicationsController.php`:
- Around line 1166-1232: Extract the repeated docker_compose_domains
validation/processing into a private helper (e.g. private function
processDockerComposeDomains(Request $request, int $teamId): array) that
encapsulates the existing logic: collect/normalize URLs, validate URL format and
scheme, detect duplicates, call checkIfDomainIsAlreadyUsedViaAPI, and build the
docker_compose_domains JSON; return a structured result like ['json' => ?string,
'error' => ?JsonResponse] so callers (the create_application code paths) can
simply call $result = $this->processDockerComposeDomains($request, $teamId); if
($result['error']) return $result['error']; if ($result['json'])
$application->docker_compose_domains = $result['json']; ensure the helper also
unsets $request->offsetUnset('docker_compose_domains') and preserves existing
error messages and force_domain_override behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 873bc9a0-4703-42d2-adf5-67f9e952cf71
📒 Files selected for processing (3)
app/Http/Controllers/Api/ApplicationsController.phpbootstrap/helpers/parsers.phptests/Feature/ApplicationParserDockerComposeDomainsTest.php
|
|
||
| // Also populate docker_compose_domains for dockercompose apps (mirrors Path 1 at lines 601-627) | ||
| if ($resource->build_pack === 'dockercompose') { | ||
| $normalizedFqdnFor = str($fqdnFor)->replace('-', '_')->replace('.', '_')->value(); | ||
| $serviceExists = false; | ||
| foreach ($services as $serviceNameKey => $svc) { | ||
| if (str($serviceNameKey)->replace('-', '_')->replace('.', '_')->value() === $normalizedFqdnFor) { | ||
| $serviceExists = true; | ||
| break; | ||
| } | ||
| } | ||
| if ($serviceExists) { | ||
| $domains = collect(json_decode(data_get($resource, 'docker_compose_domains'))) ?? collect([]); | ||
| $domainExists = data_get($domains->get($normalizedFqdnFor), 'domain'); | ||
| if (is_null($domainExists)) { | ||
| $domainValue = $fqdn; | ||
| $domains->put($normalizedFqdnFor, ['domain' => $domainValue]); | ||
| $resource->docker_compose_domains = $domains->toJson(); | ||
| $resource->save(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'll be back... to say this fix looks good, but $svc is unused.
The core logic correctly mirrors Path 1 (lines 636-649) to ensure KEY-based SERVICE_FQDN_* variables populate docker_compose_domains. This is the fix your resistance against serverless Docker Compose failures needed. However, static analysis correctly flagged an unused variable.
🔧 Proposed fix for unused variable
if ($resource->build_pack === 'dockercompose') {
$normalizedFqdnFor = str($fqdnFor)->replace('-', '_')->replace('.', '_')->value();
$serviceExists = false;
- foreach ($services as $serviceNameKey => $svc) {
+ foreach ($services as $serviceNameKey => $_) {
if (str($serviceNameKey)->replace('-', '_')->replace('.', '_')->value() === $normalizedFqdnFor) {
$serviceExists = true;
break;
}
}Or use array_keys() since you only need keys:
if ($resource->build_pack === 'dockercompose') {
$normalizedFqdnFor = str($fqdnFor)->replace('-', '_')->replace('.', '_')->value();
$serviceExists = false;
- foreach ($services as $serviceNameKey => $svc) {
- if (str($serviceNameKey)->replace('-', '_')->replace('.', '_')->value() === $normalizedFqdnFor) {
+ foreach (array_keys($services) as $serviceNameKey) {
+ if (str($serviceNameKey)->replace('-', '_')->replace('.', '_')->value() === $normalizedFqdnFor) {
$serviceExists = true;
break;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Also populate docker_compose_domains for dockercompose apps (mirrors Path 1 at lines 601-627) | |
| if ($resource->build_pack === 'dockercompose') { | |
| $normalizedFqdnFor = str($fqdnFor)->replace('-', '_')->replace('.', '_')->value(); | |
| $serviceExists = false; | |
| foreach ($services as $serviceNameKey => $svc) { | |
| if (str($serviceNameKey)->replace('-', '_')->replace('.', '_')->value() === $normalizedFqdnFor) { | |
| $serviceExists = true; | |
| break; | |
| } | |
| } | |
| if ($serviceExists) { | |
| $domains = collect(json_decode(data_get($resource, 'docker_compose_domains'))) ?? collect([]); | |
| $domainExists = data_get($domains->get($normalizedFqdnFor), 'domain'); | |
| if (is_null($domainExists)) { | |
| $domainValue = $fqdn; | |
| $domains->put($normalizedFqdnFor, ['domain' => $domainValue]); | |
| $resource->docker_compose_domains = $domains->toJson(); | |
| $resource->save(); | |
| } | |
| } | |
| } | |
| // Also populate docker_compose_domains for dockercompose apps (mirrors Path 1 at lines 601-627) | |
| if ($resource->build_pack === 'dockercompose') { | |
| $normalizedFqdnFor = str($fqdnFor)->replace('-', '_')->replace('.', '_')->value(); | |
| $serviceExists = false; | |
| foreach ($services as $serviceNameKey => $_) { | |
| if (str($serviceNameKey)->replace('-', '_')->replace('.', '_')->value() === $normalizedFqdnFor) { | |
| $serviceExists = true; | |
| break; | |
| } | |
| } | |
| if ($serviceExists) { | |
| $domains = collect(json_decode(data_get($resource, 'docker_compose_domains'))) ?? collect([]); | |
| $domainExists = data_get($domains->get($normalizedFqdnFor), 'domain'); | |
| if (is_null($domainExists)) { | |
| $domainValue = $fqdn; | |
| $domains->put($normalizedFqdnFor, ['domain' => $domainValue]); | |
| $resource->docker_compose_domains = $domains->toJson(); | |
| $resource->save(); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 512-512: Avoid unused local variables such as '$svc'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bootstrap/helpers/parsers.php` around lines 507 - 528, The foreach loop
declares an unused $svc variable (foreach ($services as $serviceNameKey =>
$svc)) which static analysis flags; change the loop to iterate only keys (e.g.,
use array_keys($services) or foreach ($services as $serviceNameKey => $_) so
$svc is not declared) while keeping the existing logic that compares
str($serviceNameKey)->... to $normalizedFqdnFor and the subsequent
docker_compose_domains update (references: $services, $serviceNameKey, $svc,
$normalizedFqdnFor, docker_compose_domains).
| test('applicationParser does not populate docker_compose_domains for non-dockercompose build_pack', function () { | ||
| $dockerCompose = <<<'YAML' | ||
| services: | ||
| backend: | ||
| image: myapp/backend:latest | ||
| environment: | ||
| - SERVICE_FQDN_BACKEND_8000=${BACKEND_URL} | ||
| YAML; | ||
|
|
||
| $application = Application::factory()->create([ | ||
| 'environment_id' => $this->environment->id, | ||
| 'destination_id' => $this->destination->id, | ||
| 'destination_type' => StandaloneDocker::class, | ||
| 'build_pack' => 'dockerfile', | ||
| 'docker_compose_raw' => $dockerCompose, | ||
| 'fqdn' => null, | ||
| 'docker_compose_domains' => null, | ||
| ]); | ||
|
|
||
| applicationParser($application); | ||
|
|
||
| $application->refresh(); | ||
|
|
||
| // For non-dockercompose, docker_compose_domains should remain empty | ||
| $domains = json_decode($application->docker_compose_domains, true); | ||
| expect($domains)->toBeNull(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Negative test case? I give it a positive review. The future is not serverless.
This test correctly verifies that non-dockercompose build packs don't get their domains populated. Essential guard against unintended side effects. The expect($domains)->toBeNull() assertion is the termination condition we need.
Consider adding an edge case test for when docker_compose_domains already has existing content to verify the merge behavior doesn't overwrite existing domains:
💡 Optional: Additional test for merge behavior
test('applicationParser preserves existing docker_compose_domains entries', function () {
$dockerCompose = <<<'YAML'
services:
backend:
image: myapp/backend:latest
environment:
- SERVICE_FQDN_BACKEND_8000=${BACKEND_URL}
frontend:
image: myapp/frontend:latest
environment:
- SERVICE_FQDN_FRONTEND=${FRONTEND_URL}
YAML;
$existingDomains = json_encode(['frontend' => ['domain' => 'https://existing.example.com']]);
$application = Application::factory()->create([
'environment_id' => $this->environment->id,
'destination_id' => $this->destination->id,
'destination_type' => StandaloneDocker::class,
'build_pack' => 'dockercompose',
'docker_compose_raw' => $dockerCompose,
'fqdn' => null,
'docker_compose_domains' => $existingDomains,
]);
applicationParser($application);
$application->refresh();
$domains = json_decode($application->docker_compose_domains, true);
// Existing frontend domain should be preserved
expect($domains['frontend']['domain'])->toBe('https://existing.example.com')
// New backend domain should be added
->and($domains)->toHaveKey('backend')
->and($domains['backend'])->toHaveKey('domain');
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Feature/ApplicationParserDockerComposeDomainsTest.php` around lines 118
- 144, Add a new negative/edge-case test to ensure applicationParser preserves
existing docker_compose_domains when merging: create a test (e.g.,
test('applicationParser preserves existing docker_compose_domains entries',
...)) that uses Application::factory()->create with build_pack 'dockercompose',
a docker_compose_raw containing multiple services (e.g., backend and frontend),
and docker_compose_domains pre-populated (json_encode of an array with an
existing frontend domain); call applicationParser($application),
$application->refresh(), then assert that the pre-existing frontend domain
remains unchanged (inspect $application->docker_compose_domains via json_decode)
and that the new backend entry was added — reference applicationParser,
Application::factory, docker_compose_raw and docker_compose_domains when
implementing assertions.
Use array_keys() instead of key-value iteration since only the service name keys are needed for comparison. Addresses CodeRabbit review feedback.
creazy231
force-pushed
the
fix/docker-compose-domains-api-9211
branch
from
48f92cc to
8b12aae
Compare
1 participant
Changes
Docker Compose applications created via the API fail during build with
Unknown variable; There is no variable named "SERVICE_URL_BACKEND"while the same applications created via the UI deploy successfully. This PR fixes two bugs that combine to cause this issue.Bug 1 —
applicationParser()doesn't populatedocker_compose_domainsfor KEY-based magic variables:The parser has two detection paths for
SERVICE_FQDN_*/SERVICE_URL_*variables. Path 1 (VALUE-based, e.g.${SERVICE_URL_BACKEND}in env var values) correctly populatesdocker_compose_domains. Path 2 (KEY-based, e.g.SERVICE_FQDN_BACKEND_8000as an env var key) creates environment variable records but never updatesdocker_compose_domains. During deployment,generate_buildtime_environment_variables()reads exclusively fromdocker_compose_domainsto produce these variables — so they were missing, causing BuildKit bake to fail.UI-created apps work because the "Generate Domain" button in the Livewire component explicitly populates
docker_compose_domains. API users have no equivalent mechanism.Bug 2 — API CREATE endpoints store
docker_compose_domainsas Collection instead of JSON string:Three API creation endpoints assign a Laravel Collection object directly to
docker_compose_domainsinstead of callingjson_encode(). Every other code path (PATCH endpoint, Livewire UI, Model) correctly usesjson_encode().Fix:
applicationParser()Path 2 (KEY-based) to populatedocker_compose_domainsfor dockercompose build packs, mirroring Path 1's existing behavior (lines 601-627).ApplicationsController.phpto wrap the Collection injson_encode().Issues
Category
Preview
N/A — backend-only change, no UI modifications.
AI Assistance
If AI was used:
Testing
Automated tests — 3 new Pest feature tests (9 assertions, all passing):
SERVICE_FQDN_BACKEND_8000populatesdocker_compose_domainswithbackendentrySERVICE_FQDN_FRONTEND(no port) populatesdocker_compose_domainswithfrontendentrydocker_compose_domainsasnull(no false positives)Manual verification — Tested against local Coolify (
v4.0.0-beta.470+ fix):SERVICE_FQDN_BACKEND_8000=${BACKEND_URL}in composedocker_compose_domainsauto-populated from compose fileContributor Agreement
Important