Enforce error diagnostics by aborting execution

[PARAIPAN9]

Motivation

• Fixes Enforce error diagnostics by aborting execution #180

Modifications

• Create ErrorThrowingDiagnosticCollector wrapper collector.

Result

• The ErrorThrowingDiagnosticCollector throws an error when a diagnostic with severity .error is emited.

Test Plan

• Make sure all tests pass and add additional test.

[PARAIPAN9]

I have a question because I'm a bit confused about what to do after the creation of the wrapper. Should we use the wrapper in place of the extensions from DiagnosticCollector?

[czechboy0]

I think DiagnosticCollector's emit method should become throwing, and then create a new concrete collector implementation that looks something like this:

struct ErrorThrowingDiagnosticCollector: DiagnosticCollector {
    var upstream: any DiagnosticCollector

    func emit(_ diagnostic: Diagnostic) throws {
        upstream.emit(diagnostic)
        if diagnostic.severity == .error { throw diagnostic }
    }
}

And then use this collector by default, wrapping today's collector.

[PARAIPAN9]

I think DiagnosticCollector's emit method should become throwing, and then create a new concrete collector implementation that looks something like this:

struct ErrorThrowingDiagnosticCollector: DiagnosticCollector {
    var upstream: any DiagnosticCollector

    func emit(_ diagnostic: Diagnostic) throws {
        upstream.emit(diagnostic)
        if diagnostic.severity == .error { throw diagnostic }
    }
}

And then use this collector by default, wrapping today's collector.

It makes sense, perfect!!

[czechboy0]

@swift-server-bot test this please

[czechboy0]

Ok the code looks good, would you be able to add one test that constructs the pipeline with a malformed input and ends up throwing the error? Just to verify that it's really working?

[PARAIPAN9]

Sure.

[czechboy0]

@swift-server-bot test this please

[czechboy0]

Got some build failures on Linux:

/code/Tests/OpenAPIGeneratorTests/Test_GenerateOptions.swift:38:34: error: cannot call value of non-function type 'String'
        let arguments = [docPath.path(), "--config", configPath.path]
                                 ^   ~~
                                     
/code/Tests/OpenAPIGeneratorTests/Test_GenerateOptions.swift:42:94: error: 'nil' requires a contextual type
            try await generator.runGenerator(outputDirectory: outputDirectory, pluginSource: nil, isDryRun: false)

You can fix the first one by using docPath.path (the property) instead. For the second, just pass the "build plugin" plugin source case.

[czechboy0]

@swift-server-bot test this please

[czechboy0]

** Running /code/scripts/check-license-headers.sh...
** ERROR: Unsupported file extension for file (exclude or update this script): Tests/OpenAPIGeneratorTests/Resources/Docs/malformed-openapi.yaml

You'll need to add the file to the license check ignore list.

[czechboy0]

Hmm this actually breaks 5.9.0, because it creates a dependency of a test target on an executable target. https://ci.swiftserver.group/job/swift-openapi-generator-swift590-prb/246/console

To fix this, you'll need to start depending on _OpenAPIGeneratorCore instead of swift-openapi-generator, and move _YamlFileDiagnosticsCollector to _OpenAPIGeneratorCore, and create a method called e.g. preparedDiagnosticsCollector(...) -> DiagnosticCollector & Sendable that lives in _OpenAPIGeneratorCore. Then just test that method, and call the method from the swift-openapi-generator code.

I don't think we'll be able to make the new dependency work otherwise.

[czechboy0]

@swift-server-bot test this please

[czechboy0]

@swift-server-bot test this please

[czechboy0]

@swift-server-bot test this please

[czechboy0]

@PARAIPAN9 The CI is happy, but I believe we still need to move the remaining diagnostic collection code into the new prepare... function, so that it's called from the CLI and also from tests. Right now we're still re-creating the collectors using different code in the CLI vs the tests.

[czechboy0]

See comment above.

[czechboy0]

Thank you @PARAIPAN9, looks great now!