fix(storages): hide PR suffix for services and fix instantSave logic

- Restrict "Add suffix for PR deployments" checkbox to non-service
  resources in both shared and service file-storage views
- Replace condition `is_preview_deployments_enabled` with `!$isService`
  for PR suffix visibility in storages/show.blade.php
- Fix FileStorage::instantSave() to use authorize + syncData instead
  of delegating to submit(), preventing unintended side effects
- Add $this->validate() to Storages/Show::instantSave() before saving
- Add response content schemas to storages API OpenAPI annotations
- Add additionalProperties: false to storage update request schema
- Rewrite PreviewDeploymentBindMountTest with behavioral tests of
  addPreviewDeploymentSuffix instead of file-content inspection
This commit is contained in:
Andras Bacsai committed 2026-03-16 21:10:00 +01:00
1 parent 3ffe900b31
commit 15d6de9f41
10 files changed
+311 -107

No files matched your search

+142 -75
View File
@@ -3,107 +3,174 @@
/**
* Tests for GitHub issue #7802: volume mappings from repo content in Preview Deployments.
*
* Bind mount volumes use a per-volume `is_preview_suffix_enabled` setting to control
* whether the -pr-N suffix is applied during preview deployments.
* When enabled (default), the suffix is applied for data isolation.
* When disabled, the volume path is shared with the main deployment.
* Named Docker volumes also respect this setting.
* Behavioral tests for addPreviewDeploymentSuffix and related helper functions.
*
* Note: The parser functions (applicationParser, serviceParser) and
* ApplicationDeploymentJob methods require database-persisted models with
* relationships (Application->destination->server, etc.), making them
* unsuitable for unit tests. Integration tests for those paths belong
* in tests/Feature/.
*/
it('uses is_preview_suffix_enabled setting for bind mount suffix in preview deployments', function () {
$parsersFile = file_get_contents(__DIR__.'/../../bootstrap/helpers/parsers.php');
describe('addPreviewDeploymentSuffix', function () {
it('appends -pr-N suffix for non-zero pull request id', function () {
expect(addPreviewDeploymentSuffix('myvolume', 3))->toBe('myvolume-pr-3');
});
// Find the bind mount handling block (type === 'bind')
$bindBlockStart = strpos($parsersFile, "if (\$type->value() === 'bind')");
$volumeBlockStart = strpos($parsersFile, "} elseif (\$type->value() === 'volume')");
$bindBlock = substr($parsersFile, $bindBlockStart, $volumeBlockStart - $bindBlockStart);
it('returns name unchanged when pull request id is zero', function () {
expect(addPreviewDeploymentSuffix('myvolume', 0))->toBe('myvolume');
});
// Bind mount block should check is_preview_suffix_enabled before applying suffix
expect($bindBlock)
->toContain('$isPreviewSuffixEnabled')
->toContain('is_preview_suffix_enabled')
->toContain('addPreviewDeploymentSuffix');
it('handles pull request id of 1', function () {
expect(addPreviewDeploymentSuffix('scripts', 1))->toBe('scripts-pr-1');
});
it('handles large pull request ids', function () {
expect(addPreviewDeploymentSuffix('data', 9999))->toBe('data-pr-9999');
});
it('handles names with dots and slashes', function () {
expect(addPreviewDeploymentSuffix('./scripts', 2))->toBe('./scripts-pr-2');
});
it('handles names with existing hyphens', function () {
expect(addPreviewDeploymentSuffix('my-volume-name', 5))->toBe('my-volume-name-pr-5');
});
it('handles empty name with non-zero pr id', function () {
expect(addPreviewDeploymentSuffix('', 1))->toBe('-pr-1');
});
it('handles uuid-prefixed volume names', function () {
$uuid = 'abc123_my-volume';
expect(addPreviewDeploymentSuffix($uuid, 7))->toBe('abc123_my-volume-pr-7');
});
it('defaults pull_request_id to 0', function () {
expect(addPreviewDeploymentSuffix('myvolume'))->toBe('myvolume');
});
});
it('still applies preview deployment suffix to named volume paths', function () {
$parsersFile = file_get_contents(__DIR__.'/../../bootstrap/helpers/parsers.php');
describe('sourceIsLocal', function () {
it('detects relative paths starting with dot-slash', function () {
expect(sourceIsLocal(str('./scripts')))->toBeTrue();
});
// Find the named volume handling block (type === 'volume')
$volumeBlockStart = strpos($parsersFile, "} elseif (\$type->value() === 'volume')");
$volumeBlock = substr($parsersFile, $volumeBlockStart, 1000);
it('detects absolute paths starting with slash', function () {
expect(sourceIsLocal(str('/var/data')))->toBeTrue();
});
// Named volumes SHOULD still get the -pr-N suffix for isolation
expect($volumeBlock)->toContain('addPreviewDeploymentSuffix');
it('detects tilde paths', function () {
expect(sourceIsLocal(str('~/data')))->toBeTrue();
});
it('detects parent directory paths', function () {
expect(sourceIsLocal(str('../config')))->toBeTrue();
});
it('returns false for named volumes', function () {
expect(sourceIsLocal(str('myvolume')))->toBeFalse();
});
});
it('confirms addPreviewDeploymentSuffix works correctly', function () {
$result = addPreviewDeploymentSuffix('myvolume', 3);
expect($result)->toBe('myvolume-pr-3');
describe('replaceLocalSource', function () {
it('replaces dot-slash prefix with target path', function () {
$result = replaceLocalSource(str('./scripts'), str('/app'));
expect((string) $result)->toBe('/app/scripts');
});
$result = addPreviewDeploymentSuffix('myvolume', 0);
expect($result)->toBe('myvolume');
it('replaces dot-dot-slash prefix with target path', function () {
$result = replaceLocalSource(str('../config'), str('/app'));
expect((string) $result)->toBe('/app./config');
});
it('replaces tilde prefix with target path', function () {
$result = replaceLocalSource(str('~/data'), str('/app'));
expect((string) $result)->toBe('/app/data');
});
});
/**
* Tests for GitHub issue #7343: $uuid mutation in label generation leaks into
* subsequent services' volume paths during preview deployments.
* Source-code structure tests for parser and deployment job.
*
* The label generation block must use a local variable ($labelUuid) instead of
* mutating the shared $uuid variable, which is used for volume base paths.
* These verify that key code patterns exist in the parser and deployment job.
* They are intentionally text-based because the parser/deployment functions
* require database-persisted models with deep relationships, making behavioral
* unit tests impractical. Full behavioral coverage should be done via Feature tests.
*/
it('does not mutate shared uuid variable during label generation', function () {
$parsersFile = file_get_contents(__DIR__.'/../../bootstrap/helpers/parsers.php');
describe('parser structure: bind mount handling', function () {
it('checks is_preview_suffix_enabled before applying suffix', function () {
$parsersFile = file_get_contents(__DIR__.'/../../bootstrap/helpers/parsers.php');
// Find the FQDN label generation block
$labelBlockStart = strpos($parsersFile, '$shouldGenerateLabelsExactly = $resource->destination->server->settings->generate_exact_labels;');
$labelBlock = substr($parsersFile, $labelBlockStart, 300);
$bindBlockStart = strpos($parsersFile, "if (\$type->value() === 'bind')");
$volumeBlockStart = strpos($parsersFile, "} elseif (\$type->value() === 'volume')");
$bindBlock = substr($parsersFile, $bindBlockStart, $volumeBlockStart - $bindBlockStart);
// Should use $labelUuid, not mutate $uuid
expect($labelBlock)
->toContain('$labelUuid = $resource->uuid')
->not->toContain('$uuid = $resource->uuid')
->not->toContain("\$uuid = \"{$resource->uuid}");
expect($bindBlock)
->toContain('$isPreviewSuffixEnabled')
->toContain('is_preview_suffix_enabled')
->toContain('addPreviewDeploymentSuffix');
});
it('applies preview suffix to named volumes', function () {
$parsersFile = file_get_contents(__DIR__.'/../../bootstrap/helpers/parsers.php');
$volumeBlockStart = strpos($parsersFile, "} elseif (\$type->value() === 'volume')");
$volumeBlock = substr($parsersFile, $volumeBlockStart, 1000);
expect($volumeBlock)->toContain('addPreviewDeploymentSuffix');
});
});
it('uses labelUuid in all proxy label generation calls', function () {
$parsersFile = file_get_contents(__DIR__.'/../../bootstrap/helpers/parsers.php');
describe('parser structure: label generation uuid isolation', function () {
it('uses labelUuid instead of mutating shared uuid', function () {
$parsersFile = file_get_contents(__DIR__.'/../../bootstrap/helpers/parsers.php');
// Find the FQDN label generation block (from shouldGenerateLabelsExactly to the closing brace)
$labelBlockStart = strpos($parsersFile, '$shouldGenerateLabelsExactly');
$labelBlockEnd = strpos($parsersFile, "data_forget(\$service, 'volumes.*.content')");
$labelBlock = substr($parsersFile, $labelBlockStart, $labelBlockEnd - $labelBlockStart);
$labelBlockStart = strpos($parsersFile, '$shouldGenerateLabelsExactly = $resource->destination->server->settings->generate_exact_labels;');
$labelBlock = substr($parsersFile, $labelBlockStart, 300);
// All uuid references in label functions should use $labelUuid
expect($labelBlock)
->toContain('uuid: $labelUuid')
->not->toContain('uuid: $uuid');
expect($labelBlock)
->toContain('$labelUuid = $resource->uuid')
->not->toContain('$uuid = $resource->uuid')
->not->toContain('$uuid = "{$resource->uuid}');
});
it('uses labelUuid in all proxy label generation calls', function () {
$parsersFile = file_get_contents(__DIR__.'/../../bootstrap/helpers/parsers.php');
$labelBlockStart = strpos($parsersFile, '$shouldGenerateLabelsExactly');
$labelBlockEnd = strpos($parsersFile, "data_forget(\$service, 'volumes.*.content')");
$labelBlock = substr($parsersFile, $labelBlockStart, $labelBlockEnd - $labelBlockStart);
expect($labelBlock)
->toContain('uuid: $labelUuid')
->not->toContain('uuid: $uuid');
});
});
it('checks is_preview_suffix_enabled in deployment job for persistent volumes', function () {
$deploymentJobFile = file_get_contents(__DIR__.'/../../app/Jobs/ApplicationDeploymentJob.php');
describe('deployment job structure: is_preview_suffix_enabled', function () {
it('checks setting in generate_local_persistent_volumes', function () {
$deploymentJobFile = file_get_contents(__DIR__.'/../../app/Jobs/ApplicationDeploymentJob.php');
// Find the generate_local_persistent_volumes method
$methodStart = strpos($deploymentJobFile, 'function generate_local_persistent_volumes()');
$methodEnd = strpos($deploymentJobFile, 'function generate_local_persistent_volumes_only_volume_names()');
$methodBlock = substr($deploymentJobFile, $methodStart, $methodEnd - $methodStart);
$methodStart = strpos($deploymentJobFile, 'function generate_local_persistent_volumes()');
$methodEnd = strpos($deploymentJobFile, 'function generate_local_persistent_volumes_only_volume_names()');
$methodBlock = substr($deploymentJobFile, $methodStart, $methodEnd - $methodStart);
// Should check is_preview_suffix_enabled before applying suffix
expect($methodBlock)
->toContain('is_preview_suffix_enabled')
->toContain('$isPreviewSuffixEnabled')
->toContain('addPreviewDeploymentSuffix');
});
expect($methodBlock)
->toContain('is_preview_suffix_enabled')
->toContain('$isPreviewSuffixEnabled')
->toContain('addPreviewDeploymentSuffix');
});
it('checks is_preview_suffix_enabled in deployment job for volume names', function () {
$deploymentJobFile = file_get_contents(__DIR__.'/../../app/Jobs/ApplicationDeploymentJob.php');
it('checks setting in generate_local_persistent_volumes_only_volume_names', function () {
$deploymentJobFile = file_get_contents(__DIR__.'/../../app/Jobs/ApplicationDeploymentJob.php');
// Find the generate_local_persistent_volumes_only_volume_names method
$methodStart = strpos($deploymentJobFile, 'function generate_local_persistent_volumes_only_volume_names()');
$methodEnd = strpos($deploymentJobFile, 'function generate_healthcheck_commands()');
$methodBlock = substr($deploymentJobFile, $methodStart, $methodEnd - $methodStart);
$methodStart = strpos($deploymentJobFile, 'function generate_local_persistent_volumes_only_volume_names()');
$methodEnd = strpos($deploymentJobFile, 'function generate_healthcheck_commands()');
$methodBlock = substr($deploymentJobFile, $methodStart, $methodEnd - $methodStart);
// Should check is_preview_suffix_enabled before applying suffix
expect($methodBlock)
->toContain('is_preview_suffix_enabled')
->toContain('$isPreviewSuffixEnabled')
->toContain('addPreviewDeploymentSuffix');
expect($methodBlock)
->toContain('is_preview_suffix_enabled')
->toContain('$isPreviewSuffixEnabled')
->toContain('addPreviewDeploymentSuffix');
});
});