From e59e0049bf6107923e5d3c5b71b467da8f102c23 Mon Sep 17 00:00:00 2001 From: Stefana Dranca Date: Wed, 3 Jan 2024 17:36:46 +0000 Subject: [PATCH 1/3] [CodeGenLib] Add validation step for input in IDLToStructuredSwiftTranslator Motivation: The IDLToStructuredSwiftTranslator should discover errors in the CodeGenerationRequest object, before the specialized translators start their work. This way, if the input is not correct no translator will be started. Modifications: Moved the validation functions into a IDLToStructuredSwiftTranslator extension and the associated tests in a separate test file. Result: The input validation is not done by each individual specialzed translator, but by the main translator, before transformations take place. --- .../IDLToStructuredSwiftTranslator.swift | 82 +++++++++ .../Translator/TypealiasTranslator.swift | 72 -------- ...uredSwiftTranslatorSnippetBasedTests.swift | 169 ++++++++++++++++++ ...TypealiasTranslatorSnippetBasedTests.swift | 125 ------------- 4 files changed, 251 insertions(+), 197 deletions(-) create mode 100644 Tests/GRPCCodeGenTests/Internal/Translator/IDLToStructuredSwiftTranslatorSnippetBasedTests.swift diff --git a/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift b/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift index c90bbffbf..b57e6b575 100644 --- a/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift +++ b/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift @@ -22,6 +22,7 @@ struct IDLToStructuredSwiftTranslator: Translator { client: Bool, server: Bool ) throws -> StructuredSwiftRepresentation { + try self.validateInput(codeGenerationRequest) let typealiasTranslator = TypealiasTranslator(client: client, server: server) let topComment = Comment.doc(codeGenerationRequest.leadingTrivia) let imports: [ImportDescription] = [ @@ -49,6 +50,87 @@ struct IDLToStructuredSwiftTranslator: Translator { } } +extension IDLToStructuredSwiftTranslator { + private func validateInput(_ codeGenerationRequest: CodeGenerationRequest) throws { + let servicesByNamespace = Dictionary( + grouping: codeGenerationRequest.services, + by: { $0.namespace } + ) + try checkServiceNamesAreUnique(for: servicesByNamespace) + for service in codeGenerationRequest.services { + try checkMethodNamesAreUnique(in: service) + } + } + + // Verify service names are unique within each namespace and that services with no namespace + // don't have the same names as any of the namespaces. + private func checkServiceNamesAreUnique( + for servicesByNamespace: [String: [CodeGenerationRequest.ServiceDescriptor]] + ) throws { + // Check that if there are services in an empty namespace, none have names which match other namespaces + let noNamespaceServices = servicesByNamespace["", default: []] + let namespaces = servicesByNamespace.keys + for service in noNamespaceServices { + if namespaces.contains(service.name) { + throw CodeGenError( + code: .nonUniqueServiceName, + message: """ + Services with no namespace must not have the same names as the namespaces. \ + \(service.name) is used as a name for a service with no namespace and a namespace. + """ + ) + } + } + + // Check that service names are unique within each namespace. + for (namespace, services) in servicesByNamespace { + var serviceNames: Set = [] + for service in services { + if serviceNames.contains(service.name) { + let errorMessage: String + if namespace.isEmpty { + errorMessage = """ + Services in an empty namespace must have unique names. \ + \(service.name) is used as a name for multiple services without namespaces. + """ + } else { + errorMessage = """ + Services within the same namespace must have unique names. \ + \(service.name) is used as a name for multiple services in the \(service.namespace) namespace. + """ + } + throw CodeGenError( + code: .nonUniqueServiceName, + message: errorMessage + ) + } + serviceNames.insert(service.name) + } + } + } + + // Verify method names are unique for the service. + private func checkMethodNamesAreUnique( + in service: CodeGenerationRequest.ServiceDescriptor + ) throws { + let methodNames = service.methods.map { $0.name } + var seenNames = Set() + + for methodName in methodNames { + if seenNames.contains(methodName) { + throw CodeGenError( + code: .nonUniqueMethodName, + message: """ + Methods of a service must have unique names. \ + \(methodName) is used as a name for multiple methods of the \(service.name) service. + """ + ) + } + seenNames.insert(methodName) + } + } +} + extension CodeGenerationRequest.ServiceDescriptor { var namespacedTypealiasPrefix: String { if self.namespace.isEmpty { diff --git a/Sources/GRPCCodeGen/Internal/Translator/TypealiasTranslator.swift b/Sources/GRPCCodeGen/Internal/Translator/TypealiasTranslator.swift index a502675df..56c43942e 100644 --- a/Sources/GRPCCodeGen/Internal/Translator/TypealiasTranslator.swift +++ b/Sources/GRPCCodeGen/Internal/Translator/TypealiasTranslator.swift @@ -66,10 +66,6 @@ struct TypealiasTranslator: SpecializedTranslator { let services = codeGenerationRequest.services let servicesByNamespace = Dictionary(grouping: services, by: { $0.namespace }) - // Verify service names are unique within each namespace and that services with no namespace - // don't have the same names as any of the namespaces. - try self.checkServiceNamesAreUnique(for: servicesByNamespace) - // Sorting the keys and the services in each list of the dictionary is necessary // so that the generated enums are deterministically ordered. for (namespace, services) in servicesByNamespace.sorted(by: { $0.key < $1.key }) { @@ -85,51 +81,6 @@ struct TypealiasTranslator: SpecializedTranslator { } extension TypealiasTranslator { - private func checkServiceNamesAreUnique( - for servicesByNamespace: [String: [CodeGenerationRequest.ServiceDescriptor]] - ) throws { - // Check that if there are services in an empty namespace, none have names which match other namespaces - let noNamespaceServices = servicesByNamespace["", default: []] - let namespaces = servicesByNamespace.keys - for service in noNamespaceServices { - if namespaces.contains(service.name) { - throw CodeGenError( - code: .nonUniqueServiceName, - message: """ - Services with no namespace must not have the same names as the namespaces. \ - \(service.name) is used as a name for a service with no namespace and a namespace. - """ - ) - } - } - - // Check that service names are unique within each namespace. - for (namespace, services) in servicesByNamespace { - var serviceNames: Set = [] - for service in services { - if serviceNames.contains(service.name) { - let errorMessage: String - if namespace.isEmpty { - errorMessage = """ - Services in an empty namespace must have unique names. \ - \(service.name) is used as a name for multiple services without namespaces. - """ - } else { - errorMessage = """ - Services within the same namespace must have unique names. \ - \(service.name) is used as a name for multiple services in the \(service.namespace) namespace. - """ - } - throw CodeGenError( - code: .nonUniqueServiceName, - message: errorMessage - ) - } - serviceNames.insert(service.name) - } - } - } - private func makeNamespaceEnum( for namespace: String, containing services: [CodeGenerationRequest.ServiceDescriptor] @@ -163,9 +114,6 @@ extension TypealiasTranslator { var methodsEnum = EnumDescription(name: "Methods") let methods = service.methods - // Verify method names are unique for the service. - try self.checkMethodNamesAreUnique(in: service) - // Create the method specific enums. for method in methods { let methodEnum = self.makeMethodEnum(from: method, in: service) @@ -196,26 +144,6 @@ extension TypealiasTranslator { return .enum(serviceEnum) } - private func checkMethodNamesAreUnique( - in service: CodeGenerationRequest.ServiceDescriptor - ) throws { - let methodNames = service.methods.map { $0.name } - var seenNames = Set() - - for methodName in methodNames { - if seenNames.contains(methodName) { - throw CodeGenError( - code: .nonUniqueMethodName, - message: """ - Methods of a service must have unique names. \ - \(methodName) is used as a name for multiple methods of the \(service.name) service. - """ - ) - } - seenNames.insert(methodName) - } - } - private func makeMethodEnum( from method: CodeGenerationRequest.ServiceDescriptor.MethodDescriptor, in service: CodeGenerationRequest.ServiceDescriptor diff --git a/Tests/GRPCCodeGenTests/Internal/Translator/IDLToStructuredSwiftTranslatorSnippetBasedTests.swift b/Tests/GRPCCodeGenTests/Internal/Translator/IDLToStructuredSwiftTranslatorSnippetBasedTests.swift new file mode 100644 index 000000000..fca3f3ee2 --- /dev/null +++ b/Tests/GRPCCodeGenTests/Internal/Translator/IDLToStructuredSwiftTranslatorSnippetBasedTests.swift @@ -0,0 +1,169 @@ +/* + * Copyright 2024, gRPC Authors All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#if os(macOS) || os(Linux) // swift-format doesn't like canImport(Foundation.Process) + +import XCTest + +@testable import GRPCCodeGen + +final class IDLToStructuredSwiftTranslatorSnippetBasedTests: XCTestCase { + typealias MethodDescriptor = GRPCCodeGen.CodeGenerationRequest.ServiceDescriptor.MethodDescriptor + typealias ServiceDescriptor = GRPCCodeGen.CodeGenerationRequest.ServiceDescriptor + + func testSameNameServicesNoNamespaceError() throws { + let serviceA = ServiceDescriptor( + documentation: "Documentation for AService", + name: "AService", + namespace: "", + methods: [] + ) + + let codeGenerationRequest = makeCodeGenerationRequest(services: [serviceA, serviceA]) + let translator = IDLToStructuredSwiftTranslator() + XCTAssertThrowsError( + ofType: CodeGenError.self, + try translator.translate( + codeGenerationRequest: codeGenerationRequest, + client: true, + server: true + ) + ) { + error in + XCTAssertEqual( + error as CodeGenError, + CodeGenError( + code: .nonUniqueServiceName, + message: """ + Services in an empty namespace must have unique names. \ + AService is used as a name for multiple services without namespaces. + """ + ) + ) + } + } + + func testSameNameServicesSameNamespaceError() throws { + let serviceA = ServiceDescriptor( + documentation: "Documentation for AService", + name: "AService", + namespace: "namespacea", + methods: [] + ) + + let codeGenerationRequest = makeCodeGenerationRequest(services: [serviceA, serviceA]) + let translator = IDLToStructuredSwiftTranslator() + XCTAssertThrowsError( + ofType: CodeGenError.self, + try translator.translate( + codeGenerationRequest: codeGenerationRequest, + client: true, + server: true + ) + ) { + error in + XCTAssertEqual( + error as CodeGenError, + CodeGenError( + code: .nonUniqueServiceName, + message: """ + Services within the same namespace must have unique names. \ + AService is used as a name for multiple services in the namespacea namespace. + """ + ) + ) + } + } + + func testSameNameMethodsSameServiceError() throws { + let methodA = MethodDescriptor( + documentation: "Documentation for MethodA", + name: "MethodA", + isInputStreaming: false, + isOutputStreaming: false, + inputType: "NamespaceA_ServiceARequest", + outputType: "NamespaceA_ServiceAResponse" + ) + let service = ServiceDescriptor( + documentation: "Documentation for AService", + name: "AService", + namespace: "namespacea", + methods: [methodA, methodA] + ) + + let codeGenerationRequest = makeCodeGenerationRequest(services: [service]) + let translator = IDLToStructuredSwiftTranslator() + XCTAssertThrowsError( + ofType: CodeGenError.self, + try translator.translate( + codeGenerationRequest: codeGenerationRequest, + client: true, + server: true + ) + ) { + error in + XCTAssertEqual( + error as CodeGenError, + CodeGenError( + code: .nonUniqueMethodName, + message: """ + Methods of a service must have unique names. \ + MethodA is used as a name for multiple methods of the AService service. + """ + ) + ) + } + } + + func testSameNameNoNamespaceServiceAndNamespaceError() throws { + let serviceA = ServiceDescriptor( + documentation: "Documentation for SameName service with no namespace", + name: "SameName", + namespace: "", + methods: [] + ) + let serviceB = ServiceDescriptor( + documentation: "Documentation for BService", + name: "BService", + namespace: "SameName", + methods: [] + ) + let codeGenerationRequest = makeCodeGenerationRequest(services: [serviceA, serviceB]) + let translator = IDLToStructuredSwiftTranslator() + XCTAssertThrowsError( + ofType: CodeGenError.self, + try translator.translate( + codeGenerationRequest: codeGenerationRequest, + client: true, + server: true + ) + ) { + error in + XCTAssertEqual( + error as CodeGenError, + CodeGenError( + code: .nonUniqueServiceName, + message: """ + Services with no namespace must not have the same names as the namespaces. \ + SameName is used as a name for a service with no namespace and a namespace. + """ + ) + ) + } + } +} + +#endif // os(macOS) || os(Linux) diff --git a/Tests/GRPCCodeGenTests/Internal/Translator/TypealiasTranslatorSnippetBasedTests.swift b/Tests/GRPCCodeGenTests/Internal/Translator/TypealiasTranslatorSnippetBasedTests.swift index b677217cd..7bca92243 100644 --- a/Tests/GRPCCodeGenTests/Internal/Translator/TypealiasTranslatorSnippetBasedTests.swift +++ b/Tests/GRPCCodeGenTests/Internal/Translator/TypealiasTranslatorSnippetBasedTests.swift @@ -497,131 +497,6 @@ final class TypealiasTranslatorSnippetBasedTests: XCTestCase { server: true ) } - - func testTypealiasTranslatorSameNameServicesNoNamespaceError() throws { - let serviceA = ServiceDescriptor( - documentation: "Documentation for AService", - name: "AService", - namespace: "", - methods: [] - ) - - let codeGenerationRequest = makeCodeGenerationRequest(services: [serviceA, serviceA]) - let translator = TypealiasTranslator(client: true, server: true) - XCTAssertThrowsError( - ofType: CodeGenError.self, - try translator.translate(from: codeGenerationRequest) - ) { - error in - XCTAssertEqual( - error as CodeGenError, - CodeGenError( - code: .nonUniqueServiceName, - message: """ - Services in an empty namespace must have unique names. \ - AService is used as a name for multiple services without namespaces. - """ - ) - ) - } - } - - func testTypealiasTranslatorSameNameServicesSameNamespaceError() throws { - let serviceA = ServiceDescriptor( - documentation: "Documentation for AService", - name: "AService", - namespace: "namespacea", - methods: [] - ) - - let codeGenerationRequest = makeCodeGenerationRequest(services: [serviceA, serviceA]) - let translator = TypealiasTranslator(client: true, server: true) - XCTAssertThrowsError( - ofType: CodeGenError.self, - try translator.translate(from: codeGenerationRequest) - ) { - error in - XCTAssertEqual( - error as CodeGenError, - CodeGenError( - code: .nonUniqueServiceName, - message: """ - Services within the same namespace must have unique names. \ - AService is used as a name for multiple services in the namespacea namespace. - """ - ) - ) - } - } - - func testTypealiasTranslatorSameNameMethodsSameServiceError() throws { - let methodA = MethodDescriptor( - documentation: "Documentation for MethodA", - name: "MethodA", - isInputStreaming: false, - isOutputStreaming: false, - inputType: "NamespaceA_ServiceARequest", - outputType: "NamespaceA_ServiceAResponse" - ) - let service = ServiceDescriptor( - documentation: "Documentation for AService", - name: "AService", - namespace: "namespacea", - methods: [methodA, methodA] - ) - - let codeGenerationRequest = makeCodeGenerationRequest(services: [service]) - let translator = TypealiasTranslator(client: true, server: true) - XCTAssertThrowsError( - ofType: CodeGenError.self, - try translator.translate(from: codeGenerationRequest) - ) { - error in - XCTAssertEqual( - error as CodeGenError, - CodeGenError( - code: .nonUniqueMethodName, - message: """ - Methods of a service must have unique names. \ - MethodA is used as a name for multiple methods of the AService service. - """ - ) - ) - } - } - - func testTypealiasTranslatorSameNameNoNamespaceServiceAndNamespaceError() throws { - let serviceA = ServiceDescriptor( - documentation: "Documentation for SameName service with no namespace", - name: "SameName", - namespace: "", - methods: [] - ) - let serviceB = ServiceDescriptor( - documentation: "Documentation for BService", - name: "BService", - namespace: "SameName", - methods: [] - ) - let codeGenerationRequest = makeCodeGenerationRequest(services: [serviceA, serviceB]) - let translator = TypealiasTranslator(client: true, server: true) - XCTAssertThrowsError( - ofType: CodeGenError.self, - try translator.translate(from: codeGenerationRequest) - ) { - error in - XCTAssertEqual( - error as CodeGenError, - CodeGenError( - code: .nonUniqueServiceName, - message: """ - Services with no namespace must not have the same names as the namespaces. \ - SameName is used as a name for a service with no namespace and a namespace. - """ - ) - ) - } - } } extension TypealiasTranslatorSnippetBasedTests { From 03e1cdbcb1fc833f54b2dbb6938a13504ed549d4 Mon Sep 17 00:00:00 2001 From: Stefana Dranca Date: Sun, 7 Jan 2024 18:32:57 +0000 Subject: [PATCH 2/3] implemented feedback --- .../IDLToStructuredSwiftTranslator.swift | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift b/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift index b57e6b575..706e39a1c 100644 --- a/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift +++ b/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift @@ -68,17 +68,18 @@ extension IDLToStructuredSwiftTranslator { for servicesByNamespace: [String: [CodeGenerationRequest.ServiceDescriptor]] ) throws { // Check that if there are services in an empty namespace, none have names which match other namespaces - let noNamespaceServices = servicesByNamespace["", default: []] - let namespaces = servicesByNamespace.keys - for service in noNamespaceServices { - if namespaces.contains(service.name) { - throw CodeGenError( - code: .nonUniqueServiceName, - message: """ - Services with no namespace must not have the same names as the namespaces. \ - \(service.name) is used as a name for a service with no namespace and a namespace. - """ - ) + if let noNamespaceServices = servicesByNamespace[""] { + let namespaces = servicesByNamespace.keys + for service in noNamespaceServices { + if namespaces.contains(service.name) { + throw CodeGenError( + code: .nonUniqueServiceName, + message: """ + Services with no namespace must not have the same names as the namespaces. \ + \(service.name) is used as a name for a service with no namespace and a namespace. + """ + ) + } } } From 2a9f2b9ec3c130134c89f7f1493989358150d87c Mon Sep 17 00:00:00 2001 From: Stefana Dranca Date: Mon, 8 Jan 2024 17:14:51 +0000 Subject: [PATCH 3/3] added self --- .../Internal/Translator/IDLToStructuredSwiftTranslator.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift b/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift index 706e39a1c..85fadaeda 100644 --- a/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift +++ b/Sources/GRPCCodeGen/Internal/Translator/IDLToStructuredSwiftTranslator.swift @@ -56,9 +56,9 @@ extension IDLToStructuredSwiftTranslator { grouping: codeGenerationRequest.services, by: { $0.namespace } ) - try checkServiceNamesAreUnique(for: servicesByNamespace) + try self.checkServiceNamesAreUnique(for: servicesByNamespace) for service in codeGenerationRequest.services { - try checkMethodNamesAreUnique(in: service) + try self.checkMethodNamesAreUnique(in: service) } }