Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(testing): add test to detect duplicate endpoint names #3908

Merged
merged 9 commits into from
Jul 19, 2024
3 changes: 3 additions & 0 deletions core/src/main/scala/sttp/tapir/Endpoint.scala
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,9 @@ trait EndpointInfoOps[-R] {
private[tapir] def withInfo(info: EndpointInfo): ThisType[R]

def name(n: String): ThisType[R] = withInfo(info.name(n))
def prefixName(n: String): ThisType[R] = withInfo(info.copy(name = Some(n + info.name.getOrElse(""))))
ThijsBroersen marked this conversation as resolved.
Show resolved Hide resolved
def suffixName(n: String): ThisType[R] = withInfo(info.copy(name = info.name.map(_ + n)))

def summary(s: String): ThisType[R] = withInfo(info.summary(s))
def description(d: String): ThisType[R] = withInfo(info.description(d))
def deprecated(): ThisType[R] = withInfo(info.deprecated(true))
Expand Down
2 changes: 2 additions & 0 deletions doc/docs/openapi.md
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ Options can be customised by providing an instance of `OpenAPIDocsOptions` to th
* `schemaName`: specifies how schema names are created from the full type name. By default, this takes the last
component of a dot-separated type name. Suffixes might be added at a later stage to disambiguate between different
schemas with same names.
* `failOnDuplicateOperationId`: if set to `true`, the interpreter will throw an exception if it encounters two endpoints
with the same operation id. An OpenAPI document with duplicate operation ids is not valid. Code generators can silently drop duplicates.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe mention & link the endpoint verifier?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I'm not 100% sure that such verification should be part of the interpreter. There are other errors that are checked by the EndpointVerifier, which would also cause invalid specs to be generated (like shadowing, duplicate methods, invalid body). So shouldn't we rather rely on the verifier to contain all the verifications?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature (interpreter option) is opt-in, so not enabled by default. This is backwards compatible.
The EndpointVerifier is not per-se focused on OpenAPI compatibility. I think it is fair to say the OpenAPI converter should have a role in this and preferably provide a safe conversion to an Either/Option/Try.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's leave it then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add some more docs


## Inlined and referenced schemas

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,24 @@ private[openapi] object EndpointToOpenAPIDocs {

val base = apiToOpenApi(api, componentsCreator, docsExtensions)

es2.map(pathCreator.pathItem).foldLeft(base) { case (current, (path, pathItem)) =>
val spec = es2.map(pathCreator.pathItem).foldLeft(base) { case (current, (path, pathItem)) =>
current.addPathItem(path, pathItem)
}

if (options.failOnDuplicateOperationId) {

val operationIds = spec.paths.pathItems.flatMap { item =>
List(item._2.get, item._2.put, item._2.post, item._2.delete, item._2.options, item._2.head, item._2.patch, item._2.trace)
.flatMap(_.flatMap(_.operationId))
}

operationIds
.groupBy(identity)
.filter(_._2.size > 1)
.foreach { case (name, _) => throw new IllegalStateException(s"Duplicate endpoints names found: ${name}") }
}

spec
}

private def apiToOpenApi(info: Info, componentsCreator: EndpointToOpenAPIComponents, docsExtensions: List[DocsExtension[_]]): OpenAPI = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ case class OpenAPIDocsOptions(
operationIdGenerator: (AnyEndpoint, Vector[String], Method) => String,
schemaName: SName => String = defaultSchemaName,
defaultDecodeFailureOutput: EndpointInput[_] => Option[EndpointOutput[_]] = OpenAPIDocsOptions.defaultDecodeFailureOutput,
markOptionsAsNullable: Boolean = false
markOptionsAsNullable: Boolean = false,
failOnDuplicateOperationId: Boolean = false
)

object OpenAPIDocsOptions {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import sttp.apispec.openapi.Info
import sttp.tapir.tests._
import org.scalatest.funsuite.AnyFunSuite
import org.scalatest.matchers.should.Matchers
import sttp.tapir.endpoint
import sttp.tapir.AnyEndpoint
import sttp.tapir.tests.Security._
import sttp.tapir.tests.Basic._
Expand Down Expand Up @@ -110,4 +111,27 @@ class EndpointToOpenAPIDocsTest extends AnyFunSuite with Matchers {
OpenAPIDocsInterpreter().toOpenAPI(e, Info("title", "19.2-beta-RC1"))
}
}

test("should fail when OpenAPIDocsOptions.failOnDuplicateOperationId is true and there are duplicate operationIds") {
val e1 = endpoint.get.name("a")
val e2 = endpoint.post.name("a")

val options = OpenAPIDocsOptions.default.copy(failOnDuplicateOperationId = true)

val es = List(e1, e2)

assertThrows[IllegalStateException](
OpenAPIDocsInterpreter(options).toOpenAPI(es, Info("title", "19.2-beta-RC1"))
)
}
test("should pass when OpenAPIDocsOptions.failOnDuplicateOperationId is false and there are duplicate operationIds") {
val e1 = endpoint.get.name("a")
val e2 = endpoint.post.name("a")

val options = OpenAPIDocsOptions.default.copy(failOnDuplicateOperationId = false)

val es = List(e1, e2)

OpenAPIDocsInterpreter(options).toOpenAPI(es, Info("title", "19.2-beta-RC1"))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,7 @@ case class UnexpectedBodyError(e: AnyEndpoint, statusCode: StatusCode) extends E
override def toString: String =
s"An endpoint ${e.show} may return status code ${statusCode} with body, which is not allowed by specificiation."
}

case class DuplicatedNameError(name: String) extends EndpointVerificationError {
override def toString: String = s"Duplicate endpoints names found: $name"
}
12 changes: 11 additions & 1 deletion testing/src/main/scala/sttp/tapir/testing/EndpointVerifier.scala
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ object EndpointVerifier {
findShadowedEndpoints(endpoints, List()).groupBy(_.e).map(_._2.head).toSet ++
findIncorrectPaths(endpoints).toSet ++
findDuplicatedMethodDefinitions(endpoints).toSet ++
findIncorrectStatusWithBody(endpoints).toSet
findIncorrectStatusWithBody(endpoints).toSet ++
findDuplicateNames(endpoints).toSet
}

private def findIncorrectPaths(endpoints: List[AnyEndpoint]): List[IncorrectPathsError] = {
Expand Down Expand Up @@ -99,6 +100,15 @@ object EndpointVerifier {
private def inputDefinedMethods(input: EndpointInput[_]): Vector[Method] = {
input.traverseInputs { case EndpointInput.FixedMethod(m, _, _) => Vector(m) }
}

private def findDuplicateNames(endpoints: List[AnyEndpoint]): List[EndpointVerificationError] = {
endpoints
.filter(_.info.name.isDefined)
.groupBy(_.info.name)
.filter(_._2.size > 1)
.map { case (name, _) => DuplicatedNameError(name.getOrElse("")) }
.toList
}
}

private sealed trait PathComponent
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,15 @@ class EndpointVerifierTest extends AnyFlatSpecLike with Matchers {
UnexpectedBodyError(e5, StatusCode.NoContent)
)
}

it should "detect duplicate names among endpoints, all endpoints names (operationId's) must be unique" in {
val e1 = endpoint.in("a").name("Z")
val e2 = endpoint.in("b").name("Z")

val result = EndpointVerifier(List(e1, e2))

result shouldBe Set(DuplicatedNameError("Z"))
}
}

sealed trait ErrorInfo
Expand Down
Loading