diff --git a/codegen/src/main/kotlin/tools/samt/codegen/http/HttpTransport.kt b/codegen/src/main/kotlin/tools/samt/codegen/http/HttpTransport.kt index 6f313544..1b9feef2 100644 --- a/codegen/src/main/kotlin/tools/samt/codegen/http/HttpTransport.kt +++ b/codegen/src/main/kotlin/tools/samt/codegen/http/HttpTransport.kt @@ -25,126 +25,159 @@ object HttpTransportConfigurationParser : TransportConfigurationParser { ?: HttpTransportConfiguration.SerializationMode.Json val services = config.getFieldOrNull("operations")?.asObject?.let { operations -> + val parsedServices = mutableListOf() - operations.asObject.fields.map { (operationsKey, operationsField) -> - val servicePath = operations.getFieldOrNull("basePath")?.asValue?.asString ?: "" + for ((operationsKey, operationsField) in operations.asObject.fields) { val service = operationsKey.asServiceName val serviceName = service.name val operationConfiguration = operationsField.asObject + val servicePath = operationConfiguration.getFieldOrNull("basePath")?.asValue?.asString ?: "" - val parsedOperations = operationConfiguration.fields - .filterKeys { it.asIdentifier != "basePath" } - .mapNotNull { (key, value) -> - val operationConfig = value.asValue - val operation = key.asOperationName(service) - val operationName = operation.name + val operationConfigurations = operationConfiguration.fields.filterKeys { it.asIdentifier != "basePath" } + val parsedOperations = mutableListOf() - if (!(operationConfig.asString matches isValidRegex)) { - params.reportError( - "Invalid operation config for '$operationName', expected ' '. A valid example: 'POST /${operationName} {parameter1, parameter2 in query}'", - operationConfig - ) - return@mapNotNull null + operationConfigLoop@for ((key, value) in operationConfigurations) { + val operationConfig = value.asValue + val operation = key.asOperationName(service) + val operationName = operation.name + + if (!(operationConfig.asString matches isValidRegex)) { + params.reportError( + "Invalid operation config for '$operationName', expected ' '. A valid example: 'POST /${operationName} {parameter1, parameter2 in query}'", + operationConfig + ) + continue + } + + val methodEndpointResult = methodEndpointRegex.matchEntire(operationConfig.asString) + if (methodEndpointResult == null) { + params.reportError( + "Invalid operation config for '$operationName', expected ' '", + operationConfig + ) + continue + } + + val (method, path, parameterPart) = methodEndpointResult.destructured + + val methodEnum = when (method) { + "GET" -> HttpTransportConfiguration.HttpMethod.Get + "POST" -> HttpTransportConfiguration.HttpMethod.Post + "PUT" -> HttpTransportConfiguration.HttpMethod.Put + "DELETE" -> HttpTransportConfiguration.HttpMethod.Delete + "PATCH" -> HttpTransportConfiguration.HttpMethod.Patch + else -> { + params.reportError("Invalid http method '$method'", operationConfig) + continue } + } + + // FIXME: This approach has the drawback that it can only detect invalid configuration if they + // are explicitly declared in the config object. If the user implements a method but does + // not provide a configuration for it, it will not be detected as an error. + // + // In order to fix this we would need to pass the implemented services and operations to + // the parser and read configurations on demand. That way the parser knows all operations + // and can generate default configurations for operations that have no explicit configuration. + + // check for duplicate method/path combinations within current service + for (operation in parsedOperations) { + if (operation.path == path && operation.method == methodEnum) { + val duplicate = operation + params.reportError("Operation '${serviceName}.${operationName}' cannot be mapped to the same method and path combination ($method $servicePath$path) as operation '${serviceName}.${duplicate.name}'", operationConfig) + continue@operationConfigLoop + } + } + + // check for duplicate method/path combinations within previously declared services + for (service in parsedServices.filter { it.path == servicePath }) { + val duplicate = service.operations.find { op -> + op.path == path && op.method == methodEnum + } + + if (duplicate != null) { + params.reportError("Operation '${serviceName}.${operationName}' cannot be mapped to the same method and path combination ($method ${service.path}$path) as operation '${service.name}.${duplicate.name}'", operationConfig) + continue@operationConfigLoop + } + } + + val parameters = mutableListOf() - val methodEndpointResult = methodEndpointRegex.matchEntire(operationConfig.asString) - if (methodEndpointResult == null) { + // parse path and path parameters + val pathComponents = path.split("/") + for (component in pathComponents) { + if (!component.startsWith("{") || !component.endsWith("}")) continue + + val pathParameterName = component.substring(1, component.length - 1) + + if (pathParameterName.isEmpty()) { params.reportError( - "Invalid operation config for '$operationName', expected ' '", + "Expected parameter name between curly braces in '$path'", operationConfig ) - return@mapNotNull null + continue } - val (method, path, parameterPart) = methodEndpointResult.destructured + if (operation.parameters.none { it.name == pathParameterName }) { + params.reportError("Path parameter '$pathParameterName' not found in operation '$operationName'", operationConfig) + continue + } + + parameters += HttpTransportConfiguration.ParameterConfiguration( + name = pathParameterName, + transportMode = HttpTransportConfiguration.TransportMode.Path, + ) + } - val methodEnum = when (method) { - "GET" -> HttpTransportConfiguration.HttpMethod.Get - "POST" -> HttpTransportConfiguration.HttpMethod.Post - "PUT" -> HttpTransportConfiguration.HttpMethod.Put - "DELETE" -> HttpTransportConfiguration.HttpMethod.Delete - "PATCH" -> HttpTransportConfiguration.HttpMethod.Patch + val parameterResults = parameterRegex.findAll(parameterPart) + // parse parameter declarations + for (parameterResult in parameterResults) { + val (names, type) = parameterResult.destructured + val transportMode = when (type) { + "query" -> HttpTransportConfiguration.TransportMode.Query + "header" -> HttpTransportConfiguration.TransportMode.Header + "body" -> HttpTransportConfiguration.TransportMode.Body + "cookie" -> HttpTransportConfiguration.TransportMode.Cookie else -> { - params.reportError("Invalid http method '$method'", operationConfig) - return@mapNotNull null + params.reportError("Invalid transport mode '$type'", operationConfig) + continue } } - val parameters = mutableListOf() - - // parse path and path parameters - val pathComponents = path.split("/") - for (component in pathComponents) { - if (!component.startsWith("{") || !component.endsWith("}")) continue - - val pathParameterName = component.substring(1, component.length - 1) - - if (pathParameterName.isEmpty()) { - params.reportError( - "Expected parameter name between curly braces in '$path'", - operationConfig - ) + for (name in names.split(",").map { it.trim() }) { + if (operation.parameters.none { it.name == name }) { + params.reportError("Parameter '$name' not found in operation '$operationName'", operationConfig) continue } - if (operation.parameters.none { it.name == pathParameterName }) { - params.reportError("Path parameter '$pathParameterName' not found in operation '$operationName'", operationConfig) + if (transportMode == HttpTransportConfiguration.TransportMode.Body && methodEnum == HttpTransportConfiguration.HttpMethod.Get) { + params.reportError("HTTP GET method doesn't accept '$name' as a BODY parameter", operationConfig) continue } parameters += HttpTransportConfiguration.ParameterConfiguration( - name = pathParameterName, - transportMode = HttpTransportConfiguration.TransportMode.Path, + name = name, + transportMode = transportMode, ) } - - val parameterResults = parameterRegex.findAll(parameterPart) - // parse parameter declarations - for (parameterResult in parameterResults) { - val (names, type) = parameterResult.destructured - val transportMode = when (type) { - "query" -> HttpTransportConfiguration.TransportMode.Query - "header" -> HttpTransportConfiguration.TransportMode.Header - "body" -> HttpTransportConfiguration.TransportMode.Body - "cookie" -> HttpTransportConfiguration.TransportMode.Cookie - else -> { - params.reportError("Invalid transport mode '$type'", operationConfig) - continue - } - } - - for (name in names.split(",").map { it.trim() }) { - if (operation.parameters.none { it.name == name }) { - params.reportError("Parameter '$name' not found in operation '$operationName'", operationConfig) - continue - } - - if (transportMode == HttpTransportConfiguration.TransportMode.Body && methodEnum == HttpTransportConfiguration.HttpMethod.Get) { - params.reportError("HTTP GET method doesn't accept '$name' as a BODY parameter", operationConfig) - continue - } - - parameters += HttpTransportConfiguration.ParameterConfiguration( - name = name, - transportMode = transportMode, - ) - } - } - - HttpTransportConfiguration.OperationConfiguration( - name = operationName, - method = methodEnum, - path = path, - parameters = parameters, - ) } - HttpTransportConfiguration.ServiceConfiguration( + parsedOperations += HttpTransportConfiguration.OperationConfiguration( + name = operationName, + method = methodEnum, + path = path, + parameters = parameters, + ) + } + + parsedServices += HttpTransportConfiguration.ServiceConfiguration( name = serviceName, operations = parsedOperations, path = servicePath ) } + + parsedServices } ?: emptyList() return HttpTransportConfiguration( diff --git a/codegen/src/test/kotlin/tools/samt/codegen/http/HttpTransportTest.kt b/codegen/src/test/kotlin/tools/samt/codegen/http/HttpTransportTest.kt index cc363db4..a2b275a7 100644 --- a/codegen/src/test/kotlin/tools/samt/codegen/http/HttpTransportTest.kt +++ b/codegen/src/test/kotlin/tools/samt/codegen/http/HttpTransportTest.kt @@ -27,7 +27,10 @@ class HttpTransportTest { assertEquals(HttpTransportConfiguration.HttpMethod.Post, config.getMethod("service", "operation")) assertEquals("", config.getPath("service")) assertEquals("/operation", config.getPath("service", "operation")) - assertEquals(HttpTransportConfiguration.TransportMode.Body, config.getTransportMode("service", "operation", "parameter")) + assertEquals( + HttpTransportConfiguration.TransportMode.Body, + config.getTransportMode("service", "operation", "parameter") + ) } @Test @@ -87,18 +90,36 @@ class HttpTransportTest { assertEquals(HttpTransportConfiguration.HttpMethod.Post, transport.getMethod("Greeter", "greet")) assertEquals("/greet/{id}", transport.getPath("Greeter", "greet")) - assertEquals(HttpTransportConfiguration.TransportMode.Path, transport.getTransportMode("Greeter", "greet", "id")) - assertEquals(HttpTransportConfiguration.TransportMode.Header, transport.getTransportMode("Greeter", "greet", "name")) - assertEquals(HttpTransportConfiguration.TransportMode.Cookie, transport.getTransportMode("Greeter", "greet", "type")) - assertEquals(HttpTransportConfiguration.TransportMode.Body, transport.getTransportMode("Greeter", "greet", "reference")) + assertEquals( + HttpTransportConfiguration.TransportMode.Path, + transport.getTransportMode("Greeter", "greet", "id") + ) + assertEquals( + HttpTransportConfiguration.TransportMode.Header, + transport.getTransportMode("Greeter", "greet", "name") + ) + assertEquals( + HttpTransportConfiguration.TransportMode.Cookie, + transport.getTransportMode("Greeter", "greet", "type") + ) + assertEquals( + HttpTransportConfiguration.TransportMode.Body, + transport.getTransportMode("Greeter", "greet", "reference") + ) assertEquals(HttpTransportConfiguration.HttpMethod.Get, transport.getMethod("Greeter", "greetAll")) assertEquals("/greet/all", transport.getPath("Greeter", "greetAll")) - assertEquals(HttpTransportConfiguration.TransportMode.Query, transport.getTransportMode("Greeter", "greetAll", "names")) + assertEquals( + HttpTransportConfiguration.TransportMode.Query, + transport.getTransportMode("Greeter", "greetAll", "names") + ) assertEquals(HttpTransportConfiguration.HttpMethod.Get, transport.getMethod("Greeter", "get")) assertEquals("/", transport.getPath("Greeter", "get")) - assertEquals(HttpTransportConfiguration.TransportMode.Query, transport.getTransportMode("Greeter", "get", "name")) + assertEquals( + HttpTransportConfiguration.TransportMode.Query, + transport.getTransportMode("Greeter", "get", "name") + ) assertEquals(HttpTransportConfiguration.HttpMethod.Put, transport.getMethod("Greeter", "put")) assertEquals("/", transport.getPath("Greeter", "put")) @@ -159,7 +180,12 @@ class HttpTransportTest { } """.trimIndent() - parseAndCheck(source to listOf("Error: Invalid transport mode 'yeet'", "Error: Parameter 'name' not found in operation 'foo'")) + parseAndCheck( + source to listOf( + "Error: Invalid transport mode 'yeet'", + "Error: Parameter 'name' not found in operation 'foo'" + ) + ) } @Test @@ -186,7 +212,12 @@ class HttpTransportTest { } """.trimIndent() - parseAndCheck(source to listOf("Error: Expected parameter name between curly braces in '/greet/{}/me'", "Error: Path parameter 'name' not found in operation 'foo'")) + parseAndCheck( + source to listOf( + "Error: Expected parameter name between curly braces in '/greet/{}/me'", + "Error: Path parameter 'name' not found in operation 'foo'" + ) + ) } @Test @@ -291,11 +322,85 @@ class HttpTransportTest { } """.trimIndent() + parseAndCheck( + source to listOf( + "Error: HTTP GET method doesn't accept 'name' as a BODY parameter" + ) + ) + } + + + @Test + fun `cannot map two operations to the same method path combination`() { + val source = """ + package tools.samt.greeter + + service Greeter { + greet(name: String): String + greetTwo(name1: String, name2: String): String + greetThree(name: String): String + greetFour(name: String): String + } + + provide GreeterEndpoint { + implements Greeter { greet, greetTwo, greetThree, greetFour } + + transport http { + operations: { + Greeter: { + greet: "GET /greet", + greetTwo: "GET /greet", + greetThree: "POST /greet/{name}", + greetFour: "POST /greet/{name}" + } + } + } + } + """.trimIndent() + parseAndCheck(source to listOf( - "Error: HTTP GET method doesn't accept 'name' as a BODY parameter" + "Error: Operation 'Greeter.greetTwo' cannot be mapped to the same method and path combination (GET /greet) as operation 'Greeter.greet'", + "Error: Operation 'Greeter.greetFour' cannot be mapped to the same method and path combination (POST /greet/{name}) as operation 'Greeter.greetThree'" )) } + @Test + fun `operations within different services cannot have the same explicit path mapping`() { + val source = """ + package tools.samt.greeter + + service HelloSayer { + say(name: String): String + } + + service GoodbyeSayer { + say(name: String): String + } + + provide SayerEndpoint { + implements HelloSayer { say } + implements GoodbyeSayer { say } + + transport http { + operations: { + HelloSayer: { + basePath: "/sayer", + say: "GET /say" + }, + GoodbyeSayer: { + basePath: "/sayer", + say: "GET /say" + } + } + } + } + """.trimIndent() + + parseAndCheck(source to listOf( + "Error: Operation 'GoodbyeSayer.say' cannot be mapped to the same method and path combination (GET /sayer/say) as operation 'HelloSayer.say'", + )) + } + private fun parseAndCheck( vararg sourceAndExpectedMessages: Pair>, ): TransportConfiguration { @@ -315,7 +420,9 @@ class HttpTransportTest { val publicApiMapper = PublicApiMapper(listOf(HttpTransportConfigurationParser), diagnosticController) - val transport = semanticModel.global.allSubPackages.map { publicApiMapper.toPublicApi(it) }.flatMap { it.providers }.single().transport + val transport = + semanticModel.global.allSubPackages.map { publicApiMapper.toPublicApi(it) }.flatMap { it.providers } + .single().transport for ((source, expectedMessages) in sourceAndExpectedMessages) { val messages = diagnosticController.contexts