feat: require explicit @param required flag and improve run failure output with params details
This commit is contained in:
@@ -208,10 +208,14 @@ fun validateScriptMetadata(scriptContent: String): List<String> {
|
||||
errors += "line $lineNo: duplicate @param name '$name'. param names must be unique."
|
||||
}
|
||||
|
||||
var hasRequiredOption = false
|
||||
parts.drop(1).forEach { part ->
|
||||
when {
|
||||
part.equals("required", ignoreCase = true) -> Unit
|
||||
part.equals("required", ignoreCase = true) -> {
|
||||
hasRequiredOption = true
|
||||
}
|
||||
part.startsWith("required=", ignoreCase = true) -> {
|
||||
hasRequiredOption = true
|
||||
val v = part.substringAfter("=").trim()
|
||||
if (!v.equals("true", ignoreCase = true) && !v.equals("false", ignoreCase = true)) {
|
||||
errors += "line $lineNo: invalid required value '$v'. expected true/false."
|
||||
@@ -224,6 +228,9 @@ fun validateScriptMetadata(scriptContent: String): List<String> {
|
||||
}
|
||||
}
|
||||
}
|
||||
if (!hasRequiredOption) {
|
||||
errors += "line $lineNo: missing required option. expected '@param: name | required=true|false | default=value | desc=text'."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -282,6 +289,14 @@ fun loadMetadataFromComments(scriptFile: File): ScriptMetadata {
|
||||
}
|
||||
|
||||
fun evalAndCapture(scriptFile: File, requestContext: ScriptRequestContext = ScriptRequestContext()): ScriptExecutionResult {
|
||||
return evalAndCapture(scriptFile, requestContext, enforceRequiredParams = true)
|
||||
}
|
||||
|
||||
fun evalAndCapture(
|
||||
scriptFile: File,
|
||||
requestContext: ScriptRequestContext = ScriptRequestContext(),
|
||||
enforceRequiredParams: Boolean,
|
||||
): ScriptExecutionResult {
|
||||
synchronized(evalLock) {
|
||||
val oldOut = System.out
|
||||
val oldErr = System.err
|
||||
@@ -298,30 +313,40 @@ fun evalAndCapture(scriptFile: File, requestContext: ScriptRequestContext = Scri
|
||||
.filter { p ->
|
||||
p.required && requestContext.args.none { token ->
|
||||
token.substringBefore("=", missingDelimiterValue = "") == p.name
|
||||
} && p.defaultValue == null
|
||||
}
|
||||
}
|
||||
.map { it.name }
|
||||
|
||||
val injected = injectArgsDeclaration(original, requestContext.args)
|
||||
val result = evalSource(injected.toScriptSource(scriptFile.name))
|
||||
val returnValueError = (result as? ResultWithDiagnostics.Success)
|
||||
?.value
|
||||
?.returnValue as? ResultValue.Error
|
||||
val hasErrorDiagnostics = result.reports.any {
|
||||
it.severity == ScriptDiagnostic.Severity.ERROR || it.severity == ScriptDiagnostic.Severity.FATAL
|
||||
}
|
||||
val diagnostics = result.reports
|
||||
.filter { it.severity > ScriptDiagnostic.Severity.DEBUG }
|
||||
.joinToString("\n") {
|
||||
val ex = it.exception?.let { e -> ": ${e::class.simpleName}: ${e.message}" } ?: ""
|
||||
"[${it.severity}] ${it.message}$ex"
|
||||
}
|
||||
val missingMessage = if (missingRequired.isEmpty()) "" else
|
||||
val missingMessage = if (!enforceRequiredParams || missingRequired.isEmpty()) "" else
|
||||
"[ERROR] Missing required parameters: ${missingRequired.joinToString(", ")}"
|
||||
|
||||
val output = buffer.toString(Charsets.UTF_8.name()).trim()
|
||||
val finalText = buildString {
|
||||
if (output.isNotEmpty()) appendLine(output)
|
||||
if (returnValueError != null) appendLine("[ERROR] ${returnValueError.error::class.simpleName}: ${returnValueError.error.message}")
|
||||
if (diagnostics.isNotEmpty()) appendLine(diagnostics)
|
||||
if (missingMessage.isNotEmpty()) appendLine(missingMessage)
|
||||
}.trim()
|
||||
|
||||
ScriptExecutionResult(
|
||||
ok = result is ResultWithDiagnostics.Success && missingRequired.isEmpty(),
|
||||
ok = result is ResultWithDiagnostics.Success &&
|
||||
!hasErrorDiagnostics &&
|
||||
returnValueError == null &&
|
||||
(!enforceRequiredParams || missingRequired.isEmpty()),
|
||||
output = finalText,
|
||||
metadata = metadata,
|
||||
missingRequiredParams = missingRequired,
|
||||
@@ -340,10 +365,11 @@ fun evalAndCaptureWithTimeout(
|
||||
scriptFile: File,
|
||||
requestContext: ScriptRequestContext = ScriptRequestContext(),
|
||||
timeoutMs: Long,
|
||||
enforceRequiredParams: Boolean = true,
|
||||
): ScriptExecutionResult {
|
||||
val boundedTimeout = timeoutMs.coerceAtLeast(1)
|
||||
val future = evalExecutor.submit<ScriptExecutionResult> {
|
||||
evalAndCapture(scriptFile, requestContext)
|
||||
evalAndCapture(scriptFile, requestContext, enforceRequiredParams = enforceRequiredParams)
|
||||
}
|
||||
return try {
|
||||
future.get(boundedTimeout, TimeUnit.MILLISECONDS)
|
||||
|
||||
@@ -62,6 +62,34 @@ private fun metadataValidationMessage(errors: List<String>): String =
|
||||
appendLine("// @param: name | required=false | default=world | desc=Name to greet")
|
||||
}.trim()
|
||||
|
||||
private fun formatParamsHint(metadata: ScriptMetadata): String =
|
||||
buildString {
|
||||
appendLine("params:")
|
||||
if (metadata.params.isEmpty()) {
|
||||
appendLine("- (none)")
|
||||
return@buildString
|
||||
}
|
||||
metadata.params.forEach { p ->
|
||||
val defaultPart = p.defaultValue?.let { ", default=$it" } ?: ""
|
||||
val descPart = p.description?.let { ", desc=$it" } ?: ""
|
||||
appendLine("- ${p.name} (required=${p.required}$defaultPart$descPart)")
|
||||
}
|
||||
}.trimEnd()
|
||||
|
||||
private fun runFailureMessage(result: ScriptExecutionResult): String {
|
||||
val title = when {
|
||||
result.timedOut -> "[ERROR] script execution timed out"
|
||||
result.missingRequiredParams.isNotEmpty() ->
|
||||
"[ERROR] script execution failed: missing required params: ${result.missingRequiredParams.joinToString(", ")}"
|
||||
else -> "[ERROR] script execution failed"
|
||||
}
|
||||
return buildString {
|
||||
appendLine(title)
|
||||
if (result.output.isNotBlank()) appendLine(result.output)
|
||||
append(formatParamsHint(result.metadata))
|
||||
}.trim()
|
||||
}
|
||||
|
||||
fun metadataJson(scriptName: String, metadata: ScriptMetadata, source: String): String {
|
||||
val description = metadata.description?.let { "\"${it.jsonEscaped()}\"" } ?: "null"
|
||||
val params = metadata.params.joinToString(",") { param ->
|
||||
@@ -105,7 +133,7 @@ suspend fun handleCreateScript(call: ApplicationCall, scriptsDir: File) {
|
||||
script.writeText(content)
|
||||
removeCachedMetadata(script)
|
||||
|
||||
val result = evalAndCapture(script, ScriptRequestContext())
|
||||
val result = evalAndCapture(script, ScriptRequestContext(), enforceRequiredParams = false)
|
||||
if (!result.ok) {
|
||||
script.delete()
|
||||
removeCachedMetadata(script)
|
||||
@@ -169,7 +197,7 @@ suspend fun handleUpdateScript(call: ApplicationCall, scriptsDir: File) {
|
||||
script.writeText(newContent)
|
||||
removeCachedMetadata(script)
|
||||
|
||||
val result = evalAndCapture(script, ScriptRequestContext())
|
||||
val result = evalAndCapture(script, ScriptRequestContext(), enforceRequiredParams = false)
|
||||
if (!result.ok) {
|
||||
script.writeText(previousContent)
|
||||
removeCachedMetadata(script)
|
||||
@@ -221,7 +249,11 @@ suspend fun handleRunRequest(call: ApplicationCall, scriptsDir: File, consumeBod
|
||||
}
|
||||
|
||||
call.respondText(
|
||||
result.output.ifBlank { if (result.ok) "OK" else "FAILED" },
|
||||
if (result.ok) {
|
||||
result.output.ifBlank { "OK" }
|
||||
} else {
|
||||
runFailureMessage(result)
|
||||
},
|
||||
status = status,
|
||||
contentType = ContentType.Text.Plain
|
||||
)
|
||||
|
||||
@@ -182,6 +182,63 @@ class WebHostApiTest {
|
||||
assertTrue(run.bodyAsText().contains("timed out"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun metadataRequiresExplicitRequiredField() = withApp { _ ->
|
||||
val create = client.post("/scripts/badmeta") {
|
||||
bearerRoot()
|
||||
setBody(
|
||||
"""
|
||||
// @desc: bad metadata
|
||||
// @param: name | default=world | desc=missing required
|
||||
val args: Array<String> = emptyArray()
|
||||
println("ok")
|
||||
""".trimIndent()
|
||||
)
|
||||
}
|
||||
assertEquals(HttpStatusCode.BadRequest, create.status)
|
||||
val body = create.bodyAsText()
|
||||
assertTrue(body.contains("metadata validation failed"))
|
||||
assertTrue(body.contains("missing required option"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun runErrorResponseIncludesParamsAndRequiredCheck() = withApp { _ ->
|
||||
val create = client.post("/scripts/runner") {
|
||||
bearerRoot()
|
||||
setBody(
|
||||
"""
|
||||
// @desc: run test
|
||||
// @param: must | required=true | default=world | desc=Must be provided explicitly
|
||||
// @param: boom | required=false | default=false | desc=Trigger runtime failure
|
||||
val args: Array<String> = emptyArray()
|
||||
val kv = args.mapNotNull {
|
||||
val i = it.indexOf('=')
|
||||
if (i <= 0) null else it.substring(0, i) to it.substring(i + 1)
|
||||
}.toMap()
|
||||
if ((kv["boom"] ?: "false").equals("true", ignoreCase = true)) {
|
||||
error("boom")
|
||||
}
|
||||
println("must=" + (kv["must"] ?: "none"))
|
||||
""".trimIndent()
|
||||
)
|
||||
}
|
||||
assertEquals(HttpStatusCode.Created, create.status)
|
||||
|
||||
val missingRequired = client.get("/run/runner") { bearerRoot() }
|
||||
assertEquals(HttpStatusCode.BadRequest, missingRequired.status)
|
||||
val missingBody = missingRequired.bodyAsText()
|
||||
assertTrue(missingBody.contains("missing required params: must"))
|
||||
assertTrue(missingBody.contains("params:"))
|
||||
assertTrue(missingBody.contains("must (required=true"))
|
||||
|
||||
val runtimeError = client.get("/run/runner?must=ok&boom=true") { bearerRoot() }
|
||||
assertEquals(HttpStatusCode.InternalServerError, runtimeError.status)
|
||||
val runtimeErrorBody = runtimeError.bodyAsText()
|
||||
assertTrue(runtimeErrorBody.contains("script execution failed"))
|
||||
assertTrue(runtimeErrorBody.contains("params:"))
|
||||
assertTrue(runtimeErrorBody.contains("boom (required=false"))
|
||||
}
|
||||
|
||||
private fun withApp(testBlock: suspend io.ktor.server.testing.ApplicationTestBuilder.(java.nio.file.Path) -> Unit) {
|
||||
val scriptsDir = createTempDirectory("webhost-api-test-")
|
||||
tempDirs.add(scriptsDir)
|
||||
|
||||
Reference in New Issue
Block a user