diff --git a/src/main/kotlin/com/coder/gateway/util/LinkHandler.kt b/src/main/kotlin/com/coder/gateway/util/LinkHandler.kt index 6ac93efa..aec1cd47 100644 --- a/src/main/kotlin/com/coder/gateway/util/LinkHandler.kt +++ b/src/main/kotlin/com/coder/gateway/util/LinkHandler.kt @@ -327,25 +327,24 @@ internal fun getMatchingAgent( } // If the agent is missing and the workspace has only one, use that. - // Prefer the ID over the name if both are set. - val agent = - if (!parameters.agentID().isNullOrBlank()) { - agents.firstOrNull { it.id.toString() == parameters.agentID() } - } else if (!parameters.agentName().isNullOrBlank()) { - agents.firstOrNull { it.name == parameters.agentName() } - } else if (agents.size == 1) { - agents.first() - } else { - null - } + // Prefer the name over the id if both are set. + val agent = if (!parameters.agentName().isNullOrBlank()) { + agents.firstOrNull { it.name == parameters.agentName() } + } else if (!parameters.agentID().isNullOrBlank()) { + agents.firstOrNull { it.id.toString() == parameters.agentID() } + } else if (agents.size == 1) { + agents.first() + } else { + null + } if (agent == null) { - if (!parameters.agentID().isNullOrBlank()) { - throw IllegalArgumentException("The workspace \"${workspace.name}\" does not have an agent with ID \"${parameters.agentID()}\"") - } else if (!parameters.agentName().isNullOrBlank()) { + if (!parameters.agentName().isNullOrBlank()) { throw IllegalArgumentException( - "The workspace \"${workspace.name}\"does not have an agent named \"${parameters.agentName()}\"", + "The workspace \"${workspace.name}\" does not have an agent named \"${parameters.agentName()}\"", ) + } else if (!parameters.agentID().isNullOrBlank()) { + throw IllegalArgumentException("The workspace \"${workspace.name}\" does not have an agent with ID \"${parameters.agentID()}\"") } else { throw MissingArgumentException( "Unable to determine which agent to connect to; one of \"$AGENT_NAME\" or \"$AGENT_ID\" must be set because the workspace \"${workspace.name}\" has more than one agent", diff --git a/src/main/kotlin/com/coder/gateway/util/LinkMap.kt b/src/main/kotlin/com/coder/gateway/util/LinkMap.kt index 4c93d221..c79be91e 100644 --- a/src/main/kotlin/com/coder/gateway/util/LinkMap.kt +++ b/src/main/kotlin/com/coder/gateway/util/LinkMap.kt @@ -29,6 +29,7 @@ fun Map.owner() = this[OWNER] fun Map.agentName() = this[AGENT_NAME] +@Deprecated("Use the agent name instead") fun Map.agentID() = this[AGENT_ID] fun Map.folder() = this[FOLDER] diff --git a/src/test/kotlin/com/coder/gateway/util/LinkHandlerTest.kt b/src/test/kotlin/com/coder/gateway/util/LinkHandlerTest.kt index 8925fc44..662dc976 100644 --- a/src/test/kotlin/com/coder/gateway/util/LinkHandlerTest.kt +++ b/src/test/kotlin/com/coder/gateway/util/LinkHandlerTest.kt @@ -12,199 +12,396 @@ import kotlin.test.assertEquals import kotlin.test.assertFailsWith internal class LinkHandlerTest { - /** - * Create, start, and return a server that uses the provided handler. - */ - private fun mockServer(handler: HttpHandler): Pair { - val srv = HttpServer.create(InetSocketAddress(0), 0) - srv.createContext("/", handler) - srv.start() - return Pair(srv, "http://localhost:" + srv.address.port) - } - /** - * Create, start, and return a server that mocks redirects. - */ - private fun mockRedirectServer( - location: String, - temp: Boolean, - ): Pair = mockServer { exchange -> - exchange.responseHeaders.set("Location", location) - exchange.sendResponseHeaders( - if (temp) HttpURLConnection.HTTP_MOVED_TEMP else HttpURLConnection.HTTP_MOVED_PERM, - -1, - ) - exchange.close() - } + // Test data setup + private companion object { + val AGENT_1 = AgentTestData(name = "agent_name", id = "9a920eee-47fb-4571-9501-e4b3120c12f2") + val AGENT_2 = AgentTestData(name = "agent_name_2", id = "fb3daea4-da6b-424d-84c7-36b90574cfef") + val AGENT_3 = AgentTestData(name = "agent_name_3", id = "b0e4c54d-9ba9-4413-8512-11ca1e826a24") - private val agents = - mapOf( - "agent_name_3" to "b0e4c54d-9ba9-4413-8512-11ca1e826a24", - "agent_name_2" to "fb3daea4-da6b-424d-84c7-36b90574cfef", - "agent_name" to "9a920eee-47fb-4571-9501-e4b3120c12f2", - ) - private val oneAgent = - mapOf( - "agent_name_3" to "b0e4c54d-9ba9-4413-8512-11ca1e826a24", + val ALL_AGENTS = mapOf( + AGENT_3.name to AGENT_3.id, + AGENT_2.name to AGENT_2.id, + AGENT_1.name to AGENT_1.id ) + val SINGLE_AGENT = mapOf(AGENT_3.name to AGENT_3.id) + } + @Test - fun getMatchingAgent() { - val ws = DataGen.workspace("ws", agents = agents) - - val tests = - listOf( - Pair(mapOf("agent" to "agent_name"), "9a920eee-47fb-4571-9501-e4b3120c12f2"), - Pair(mapOf("agent_id" to "9a920eee-47fb-4571-9501-e4b3120c12f2"), "9a920eee-47fb-4571-9501-e4b3120c12f2"), - Pair(mapOf("agent" to "agent_name_2"), "fb3daea4-da6b-424d-84c7-36b90574cfef"), - Pair(mapOf("agent_id" to "fb3daea4-da6b-424d-84c7-36b90574cfef"), "fb3daea4-da6b-424d-84c7-36b90574cfef"), - Pair(mapOf("agent" to "agent_name_3"), "b0e4c54d-9ba9-4413-8512-11ca1e826a24"), - Pair(mapOf("agent_id" to "b0e4c54d-9ba9-4413-8512-11ca1e826a24"), "b0e4c54d-9ba9-4413-8512-11ca1e826a24"), - // Prefer agent_id. - Pair( - mapOf( - "agent" to "agent_name", - "agent_id" to "b0e4c54d-9ba9-4413-8512-11ca1e826a24", - ), - "b0e4c54d-9ba9-4413-8512-11ca1e826a24", - ), + fun `getMatchingAgent finds agent by name`() { + val ws = DataGen.workspace("ws", agents = ALL_AGENTS) + + val testCases = listOf( + AgentMatchTestCase( + "matches agent_name", + mapOf("agent" to AGENT_1.name), + AGENT_1.id + ), + AgentMatchTestCase( + "matches agent_name_2", + mapOf("agent" to AGENT_2.name), + AGENT_2.id + ), + AgentMatchTestCase( + "matches agent_name_3", + mapOf("agent" to AGENT_3.name), + AGENT_3.id ) + ) - tests.forEach { - assertEquals(UUID.fromString(it.second), getMatchingAgent(it.first, ws).id) + testCases.forEach { testCase -> + assertEquals( + UUID.fromString(testCase.expectedAgentId), + getMatchingAgent(testCase.params, ws).id, + "Failed: ${testCase.description}" + ) } } @Test - fun failsToGetMatchingAgent() { - val ws = DataGen.workspace("ws", agents = agents) - val tests = - listOf( - Triple(emptyMap(), MissingArgumentException::class, "Unable to determine"), - Triple(mapOf("agent" to ""), MissingArgumentException::class, "Unable to determine"), - Triple(mapOf("agent_id" to ""), MissingArgumentException::class, "Unable to determine"), - Triple(mapOf("agent" to null), MissingArgumentException::class, "Unable to determine"), - Triple(mapOf("agent_id" to null), MissingArgumentException::class, "Unable to determine"), - Triple(mapOf("agent" to "ws"), IllegalArgumentException::class, "agent named"), - Triple(mapOf("agent" to "ws.agent_name"), IllegalArgumentException::class, "agent named"), - Triple(mapOf("agent" to "agent_name_4"), IllegalArgumentException::class, "agent named"), - Triple(mapOf("agent_id" to "not-a-uuid"), IllegalArgumentException::class, "agent with ID"), - Triple(mapOf("agent_id" to "ceaa7bcf-1612-45d7-b484-2e0da9349168"), IllegalArgumentException::class, "agent with ID"), - // Will ignore agent if agent_id is set even if agent matches. - Triple( - mapOf( - "agent" to "agent_name", - "agent_id" to "ceaa7bcf-1612-45d7-b484-2e0da9349168", - ), - IllegalArgumentException::class, - "agent with ID", - ), + fun `getMatchingAgent finds agent by ID`() { + val ws = DataGen.workspace("ws", agents = ALL_AGENTS) + + val testCases = listOf( + AgentMatchTestCase( + "matches by agent_1 ID", + mapOf("agent_id" to AGENT_1.id), + AGENT_1.id + ), + AgentMatchTestCase( + "matches by agent_2 ID", + mapOf("agent_id" to AGENT_2.id), + AGENT_2.id + ), + AgentMatchTestCase( + "matches by agent_3 ID", + mapOf("agent_id" to AGENT_3.id), + AGENT_3.id ) + ) - tests.forEach { - val ex = - assertFailsWith( - exceptionClass = it.second, - block = { getMatchingAgent(it.first, ws).id }, - ) - assertContains(ex.message.toString(), it.third) + testCases.forEach { testCase -> + assertEquals( + UUID.fromString(testCase.expectedAgentId), + getMatchingAgent(testCase.params, ws).id, + "Failed: ${testCase.description}" + ) } } @Test - fun getsFirstAgentWhenOnlyOne() { - val ws = DataGen.workspace("ws", agents = oneAgent) - val tests = - listOf( + fun `getMatchingAgent prefers agent name over agent_id`() { + val ws = DataGen.workspace("ws", agents = ALL_AGENTS) + + val params = mapOf( + "agent" to AGENT_3.name, + "agent_id" to AGENT_2.id + ) + + assertEquals(AGENT_3.uuid, getMatchingAgent(params, ws).id) + } + + @Test + fun `getMatchingAgent fails with missing parameters`() { + val ws = DataGen.workspace("ws", agents = ALL_AGENTS) + + val testCases = listOf( + AgentFailureTestCase( + "empty parameters (i.e. no agent name or id provided)", emptyMap(), + MissingArgumentException::class, + "Unable to determine which agent to connect to; one of \"agent\" or \"agent_id\" must be set because the workspace \"ws\" has more than one agent" + ), + AgentFailureTestCase( + "empty agent name", mapOf("agent" to ""), + MissingArgumentException::class, + "Unable to determine which agent to connect to; one of \"agent\" or \"agent_id\" must be set because the workspace \"ws\" has more than one agent" + ), + AgentFailureTestCase( + "empty agent id", mapOf("agent_id" to ""), + MissingArgumentException::class, + "Unable to determine which agent to connect to; one of \"agent\" or \"agent_id\" must be set because the workspace \"ws\" has more than one agent" + ), + AgentFailureTestCase( + "null agent name", mapOf("agent" to null), + MissingArgumentException::class, + "Unable to determine which agent to connect to; one of \"agent\" or \"agent_id\" must be set because the workspace \"ws\" has more than one agent" + ), + AgentFailureTestCase( + "null agent id", mapOf("agent_id" to null), + MissingArgumentException::class, + "Unable to determine which agent to connect to; one of \"agent\" or \"agent_id\" must be set because the workspace \"ws\" has more than one agent" ) + ) + + testCases.forEach { testCase -> + val ex = assertFailsWith( + exceptionClass = testCase.expectedException, + message = "Failed: ${testCase.description}" + ) { + getMatchingAgent(testCase.params, ws).id + } + assertContains(ex.message.toString(), testCase.expectedMessageFragment) + } + } - tests.forEach { + @Test + fun `getMatchingAgent fails with invalid agent references`() { + val ws = DataGen.workspace("ws", agents = ALL_AGENTS) + + val testCases = listOf( + AgentFailureTestCase( + "workspace name instead of agent", + mapOf("agent" to "ws"), + IllegalArgumentException::class, + "The workspace \"ws\" does not have an agent named \"ws\"" + ), + AgentFailureTestCase( + "qualified agent name", + mapOf("agent" to "ws.agent_name"), + IllegalArgumentException::class, + "The workspace \"ws\" does not have an agent named \"ws.agent_name\"" + ), + AgentFailureTestCase( + "non-existent agent name", + mapOf("agent" to "agent_name_4"), + IllegalArgumentException::class, + "The workspace \"ws\" does not have an agent named \"agent_name_4\"" + ), + AgentFailureTestCase( + "invalid UUID format", + mapOf("agent_id" to "not-a-uuid"), + IllegalArgumentException::class, + "The workspace \"ws\" does not have an agent with ID \"not-a-uuid\"" + ), + AgentFailureTestCase( + "non-existent agent ID", + mapOf("agent_id" to "ceaa7bcf-1612-45d7-b484-2e0da9349168"), + IllegalArgumentException::class, + "The workspace \"ws\" does not have an agent with ID \"ceaa7bcf-1612-45d7-b484-2e0da9349168\"" + ), + AgentFailureTestCase( + "ignores valid agent_id when agent name is invalid", + mapOf( + "agent" to "unknown_agent_name", + "agent_id" to "b0e4c54d-9ba9-4413-8512-11ca1e826a24" + ), + IllegalArgumentException::class, + "The workspace \"ws\" does not have an agent named \"unknown_agent_name\"" + ) + ) + + testCases.forEach { testCase -> + val ex = assertFailsWith( + exceptionClass = testCase.expectedException, + message = "Failed: ${testCase.description}" + ) { + getMatchingAgent(testCase.params, ws).id + } + assertContains(ex.message.toString(), testCase.expectedMessageFragment) + } + } + + @Test + fun `getMatchingAgent returns only agent when workspace has one agent`() { + val ws = DataGen.workspace("ws", agents = SINGLE_AGENT) + + val testCases = listOf( + "empty parameters (i.e. no agent name or id provided)" to emptyMap(), + "empty agent name" to mapOf("agent" to ""), + "empty agent id" to mapOf("agent_id" to ""), + "null agent name" to mapOf("agent" to null), + "null agent id" to mapOf("agent_id" to null) + ) + + testCases.forEach { (description, params) -> assertEquals( - UUID.fromString("b0e4c54d-9ba9-4413-8512-11ca1e826a24"), - getMatchingAgent( - it, - ws, - ).id, + AGENT_3.uuid, + getMatchingAgent(params, ws).id, + "Failed: $description" ) } } @Test - fun failsToGetAgentWhenOnlyOne() { - val ws = DataGen.workspace("ws", agents = oneAgent) - val tests = - listOf( - Triple(mapOf("agent" to "ws"), IllegalArgumentException::class, "agent named"), - Triple(mapOf("agent" to "ws.agent_name_3"), IllegalArgumentException::class, "agent named"), - Triple(mapOf("agent" to "agent_name_4"), IllegalArgumentException::class, "agent named"), - Triple(mapOf("agent_id" to "ceaa7bcf-1612-45d7-b484-2e0da9349168"), IllegalArgumentException::class, "agent with ID"), + fun `getMatchingAgent fails with invalid references in single-agent workspace`() { + val ws = DataGen.workspace("ws", agents = SINGLE_AGENT) + + val testCases = listOf( + AgentFailureTestCase( + "invalid agent name provided, i.e. the workspace name", + mapOf("agent" to "ws"), + IllegalArgumentException::class, + "The workspace \"ws\" does not have an agent named \"ws\"" + ), + AgentFailureTestCase( + "qualified agent name", + mapOf("agent" to "ws.agent_name_3"), + IllegalArgumentException::class, + "The workspace \"ws\" does not have an agent named \"ws.agent_name_3\"" + ), + AgentFailureTestCase( + "non-existent agent", + mapOf("agent" to "agent_name_4"), + IllegalArgumentException::class, + "The workspace \"ws\" does not have an agent named \"agent_name_4\"" + ), + AgentFailureTestCase( + "non-existent agent ID", + mapOf("agent_id" to "ceaa7bcf-1612-45d7-b484-2e0da9349168"), + IllegalArgumentException::class, + "The workspace \"ws\" does not have an agent with ID \"ceaa7bcf-1612-45d7-b484-2e0da9349168\"" ) + ) - tests.forEach { - val ex = - assertFailsWith( - exceptionClass = it.second, - block = { getMatchingAgent(it.first, ws).id }, - ) - assertContains(ex.message.toString(), it.third) + testCases.forEach { testCase -> + val ex = assertFailsWith( + exceptionClass = testCase.expectedException, + message = "Failed: ${testCase.description}" + ) { + getMatchingAgent(testCase.params, ws).id + } + assertContains(ex.message.toString(), testCase.expectedMessageFragment) } } @Test - fun failsToGetAgentWithoutAgents() { + fun `getMatchingAgent fails when workspace has no agents`() { val ws = DataGen.workspace("ws") - val tests = - listOf( - Triple(emptyMap(), IllegalArgumentException::class, "has no agents"), - Triple(mapOf("agent" to ""), IllegalArgumentException::class, "has no agents"), - Triple(mapOf("agent_id" to ""), IllegalArgumentException::class, "has no agents"), - Triple(mapOf("agent" to null), IllegalArgumentException::class, "has no agents"), - Triple(mapOf("agent_id" to null), IllegalArgumentException::class, "has no agents"), - Triple(mapOf("agent" to "agent_name"), IllegalArgumentException::class, "has no agents"), - Triple(mapOf("agent_id" to "9a920eee-47fb-4571-9501-e4b3120c12f2"), IllegalArgumentException::class, "has no agents"), + + val testCases = listOf( + AgentFailureTestCase( + "empty map", + emptyMap(), + IllegalArgumentException::class, + "The workspace \"ws\" has no agents" + ), + AgentFailureTestCase( + "empty agent string", + mapOf("agent" to ""), + IllegalArgumentException::class, + "The workspace \"ws\" has no agents" + ), + AgentFailureTestCase( + "empty agent_id string", + mapOf("agent_id" to ""), + IllegalArgumentException::class, + "The workspace \"ws\" has no agents" + ), + AgentFailureTestCase( + "null agent", + mapOf("agent" to null), + IllegalArgumentException::class, + "The workspace \"ws\" has no agents" + ), + AgentFailureTestCase( + "null agent_id", + mapOf("agent_id" to null), + IllegalArgumentException::class, + "The workspace \"ws\" has no agents" + ), + AgentFailureTestCase( + "valid agent name", + mapOf("agent" to "agent_name"), + IllegalArgumentException::class, + "The workspace \"ws\" has no agents" + ), + AgentFailureTestCase( + "valid agent ID", + mapOf("agent_id" to "9a920eee-47fb-4571-9501-e4b3120c12f2"), + IllegalArgumentException::class, + "The workspace \"ws\" has no agents" ) + ) - tests.forEach { - val ex = - assertFailsWith( - exceptionClass = it.second, - block = { getMatchingAgent(it.first, ws).id }, - ) - assertContains(ex.message.toString(), it.third) + testCases.forEach { testCase -> + val ex = assertFailsWith( + exceptionClass = testCase.expectedException, + message = "Failed: ${testCase.description}" + ) { + getMatchingAgent(testCase.params, ws).id + } + assertContains(ex.message.toString(), testCase.expectedMessageFragment) } } @Test - fun followsRedirects() { - val (srv1, url1) = - mockServer { exchange -> - exchange.sendResponseHeaders(HttpURLConnection.HTTP_OK, -1) - exchange.close() - } - val (srv2, url2) = mockRedirectServer(url1, false) - val (srv3, url3) = mockRedirectServer(url2, true) + fun `resolveRedirects follows redirect chain`() { + val finalServer = mockOkServer() + val permanentRedirect = mockRedirectServer(finalServer.url, isPermanent = true) + val temporaryRedirect = mockRedirectServer(permanentRedirect.url, isPermanent = false) - assertEquals(url1.toURL(), resolveRedirects(java.net.URL(url3))) - - srv1.stop(0) - srv2.stop(0) - srv3.stop(0) + try { + assertEquals( + finalServer.url.toURL(), + resolveRedirects(temporaryRedirect.url.toURL()) + ) + } finally { + finalServer.stop() + permanentRedirect.stop() + temporaryRedirect.stop() + } } @Test - fun followsMaximumRedirects() { - val (srv, url) = mockRedirectServer(".", true) + fun `resolveRedirects fails on infinite redirect loop`() { + val server = mockRedirectServer(".", isPermanent = false) - assertFailsWith( - exceptionClass = Exception::class, - block = { resolveRedirects(java.net.URL(url)) }, - ) + try { + assertFailsWith { + resolveRedirects(server.url.toURL()) + } + } finally { + server.stop() + } + } + + internal data class AgentTestData(val name: String, val id: String) { + val uuid: UUID get() = UUID.fromString(id) + } + + internal data class AgentMatchTestCase( + val description: String, + val params: Map, + val expectedAgentId: String + ) - srv.stop(0) + internal data class AgentFailureTestCase( + val description: String, + val params: Map, + val expectedException: kotlin.reflect.KClass, + val expectedMessageFragment: String + ) + + // Helper classes for cleaner test server management + private data class TestServer(val httpServer: HttpServer, val url: String) { + fun stop() = httpServer.stop(0) + } + + private fun mockServer(handler: HttpHandler): TestServer { + val server = HttpServer.create(InetSocketAddress(0), 0) + server.createContext("/", handler) + server.start() + return TestServer(server, "http://localhost:${server.address.port}") } -} + + private fun mockOkServer(): TestServer = mockServer { exchange -> + exchange.sendResponseHeaders(HttpURLConnection.HTTP_OK, -1) + exchange.close() + } + + private fun mockRedirectServer(location: String, isPermanent: Boolean): TestServer = + mockServer { exchange -> + exchange.responseHeaders.set("Location", location) + exchange.sendResponseHeaders( + if (isPermanent) HttpURLConnection.HTTP_MOVED_PERM else HttpURLConnection.HTTP_MOVED_TEMP, + -1 + ) + exchange.close() + } +} \ No newline at end of file