Make touching swagger.json file optional (or at least don't do it during bootstrapping)

[jamisonbryant]

Describe the bug
This plugin uses a Configuration class which has extended logic around setting certain configuration options the plugin needs. One such option is SwaggerBake.json which is a path, relative to application root, where the OpenAPI schema file will be generated. The custom logic essentially checks if the path is valid and if the file exists and is writeable, which is fine, but it also attempts to create the file if it's not present, which is problematic.

To Reproduce
Consider the following command being run on a filesystem with very restrictive permissions:

# assume SwaggerBake.json is set to "webroot/swagger.json" which is the default
# also assume that ROOT/tmp/tests is the *only* directory we can write to on this filesystem
bin/cake swagger bake --output=/tmp/tests/swagger12345.json

During the bootstrap process (when the Configuration object is created) the plugin will check for existence and writability of the path currently in config, before it even gets to the part of the command where the output path is overwritten. This is problematic in read-only filesystems because while the /tmp/tests path may be writable, I don't necessarily want to allow my /webroot directory to be writeable.

Expected behavior
It would be wonderful if during the bootstrap process the plugin would not attempt to create the file. It seems like that is extra responsibility the Configuration class should not be responsible for. I would say that that file should instead be created "just in time", e.g. either just before or just after the swagger bake finishes gathering all the routes for conversion to OpenAPI format.

However, given that this could technically be considered a functionality regression, I suggest possibly adding a new configuration setting autoCreateJsonFile or something like that which puts the control in the hands of the developer. Or, perhaps you can check the arguments to the command and call Configure::write('SwaggerBake.json', $outputArgValue) before instantiating the Configuration object.

Attachments
N/A

Version and Platform (please complete the following information):

• OS/Platform: Debian Linux ("Buster") PHP 8.2 CLI Docker image (on a GitLab runner)
• CakePHP: 5.0.0
• SwaggerBake Version: 3.0

Additional context
N/A

[jamisonbryant]

Looking just a bit closer, the issue that is affecting me is this:

When the SwaggerFactory is instantiated, the Configuration object is also instantiated (causes the default config to be loaded, and the file to be "touched"). Only then is the --output arg read and the output path overridden.

[cnizzardini]

What is the error you are getting here @jamisonbryant ?

[jamisonbryant]

Here is the error:

InvalidArgumentException: Invalid json: /webroot/swagger.json. Config value for json must exist on the file system. An attempt was made to create /builds/my-org/my-proj/webroot/swagger.json, but permission was denied or the file path is bad. Either fix the file system permissions, create the file and/or both. Generally this value should be placed in your projects webroot directory.

Here is our test, it is quite simple:

public function testOpenApiDocsRespondOk(): void
{
  $tmpFile = tempnam(TMP . 'tests', 'swagger');
  $outputPath = str_replace(ROOT, '', $tmpFile);
  
  Configure::write('SwaggerBake.json', $outputPath);
  
  $this->get('/');
  
  $this->assertResponseOk();
  $this->assertResponseContains('/swagger_bake/swagger-ui-bundle.js');
  }

(the reason this test is even necessary is because we messed up a middleware at one point and it broke our Swagger docs but we didn't catch it til much later, so we wanted to prevent that from happening again)

[cnizzardini]

Yes I think touching the file is maybe a bad idea, though this is the first time an issue has been raised around it. It seems simply removing !touch($path) would resolve this issue and I don't think there are any serious backward compatibility concerns in doing this.

However, knowing all you know, wouldn't a reasonable workaround on your part have been just to simply create the file before all this? Was that never a possibility for you, the error message says as much:

        if ((!file_exists($path) && !is_writable($path)) || !touch($path)) {
            throw new InvalidArgumentException(
                sprintf(
                    "Invalid json: `%s`. Config value for `json` must exist on the file system. An attempt was 
                    made to create %s, but permission was denied or the file path is bad. Either fix the file system 
                    permissions, create the file and/or both. $message",
                    $json,
                    $path
                )
            );
        }

This library certainly expects to be able to write to the file system.

[cnizzardini]

I suspect I built it this way as I was focused on assisting engineers during the installation phase, but reasonable errors for file system permissions are given in Swagger::writeFile().

[cnizzardini]

Wait, is this whole issue report to get around a unit test issue...?

[jamisonbryant]

To "get around it"? No. We got around it previously by doing as you suggested earlier, which is to ensure the file is created prior to the error being encountered. We observed this issue in our CI pipeline while running our test case, though. More generally speaking, we noticed this error happens on readonly file systems, like CI pipelines and some Docker images.