Ambigous path and query parameters
soujiro32167 opened this issue · 5 comments
Hey Devon!
I'm back to the crazy plan to generate a client for the Kubernetes API. Thanks again for all the help last Lambdaconf =]
Here is a snippet of the generated client that does not compile:
Core_v1Client.scala
def connectCoreV1PostNamespacedPodProxyWithPath(traceBuilder: TraceBuilder[F], name: String, namespace: String, path: String, path: Option[String] = None, methodName: String = "connect-core-v1-post-namespaced-pod-proxy-with-path", headers: List[Header] = List.empty): F[ConnectCoreV1PostNamespacedPodProxyWithPathResponse] = {
val tracingHttpClient = traceBuilder(s"$clientName:$methodName")(httpClient)
val allHeaders = headers ++ List[Option[Header]]().flatten
val req = Request[F](method = Method.POST, uri = Uri.unsafeFromString(host + basePath + "/api/v1/namespaces/" + Formatter.addPath(namespace) + "/pods/" + Formatter.addPath(name) + "/proxy/" + Formatter.addPath(path) + "?" + Formatter.addArg("path", path)), headers = Headers(allHeaders))
...The path parameter appears twice, as both String and Option[String]. It is used both as a path parameter and query parameter.
Below is the corresponding OpenAPI snippet. What happens is that they define a path parameter with the same name as a query parameter:
/api/v1/namespaces/{namespace}/pods/{name}/proxy/{path}
...
"parameters": [
...
{
"description": "Path is the URL path to use for the current proxy request to pod.",
"in": "query",
"name": "path",
"type": "string",
"uniqueItems": true
}
I'd be happy to help with resolution, given some direction
Cheers,
Eli
"/api/v1/namespaces/{namespace}/pods/{name}/proxy/{path}": {
"delete": {
"consumes": [
"*/*"
],
"description": "connect DELETE requests to proxy of Pod",
"operationId": "connectCoreV1DeleteNamespacedPodProxyWithPath",
"produces": [
"*/*"
],
"responses": {
"200": {
"description": "OK",
"schema": {
"type": "string"
}
},
"401": {
"description": "Unauthorized"
}
},
"schemes": [
"https"
],
"tags": [
"core_v1"
],
"x-kubernetes-action": "connect",
"x-kubernetes-group-version-kind": {
"group": "",
"kind": "PodProxyOptions",
"version": "v1"
}
},
"get": {
"consumes": [
"*/*"
],
"description": "connect GET requests to proxy of Pod",
"operationId": "connectCoreV1GetNamespacedPodProxyWithPath",
"produces": [
"*/*"
],
"responses": {
"200": {
"description": "OK",
"schema": {
"type": "string"
}
},
"401": {
"description": "Unauthorized"
}
},
"schemes": [
"https"
],
"tags": [
"core_v1"
],
"x-kubernetes-action": "connect",
"x-kubernetes-group-version-kind": {
"group": "",
"kind": "PodProxyOptions",
"version": "v1"
}
},
"head": {
"consumes": [
"*/*"
],
"description": "connect HEAD requests to proxy of Pod",
"operationId": "connectCoreV1HeadNamespacedPodProxyWithPath",
"produces": [
"*/*"
],
"responses": {
"200": {
"description": "OK",
"schema": {
"type": "string"
}
},
"401": {
"description": "Unauthorized"
}
},
"schemes": [
"https"
],
"tags": [
"core_v1"
],
"x-kubernetes-action": "connect",
"x-kubernetes-group-version-kind": {
"group": "",
"kind": "PodProxyOptions",
"version": "v1"
}
},
"options": {
"consumes": [
"*/*"
],
"description": "connect OPTIONS requests to proxy of Pod",
"operationId": "connectCoreV1OptionsNamespacedPodProxyWithPath",
"produces": [
"*/*"
],
"responses": {
"200": {
"description": "OK",
"schema": {
"type": "string"
}
},
"401": {
"description": "Unauthorized"
}
},
"schemes": [
"https"
],
"tags": [
"core_v1"
],
"x-kubernetes-action": "connect",
"x-kubernetes-group-version-kind": {
"group": "",
"kind": "PodProxyOptions",
"version": "v1"
}
},
"parameters": [
{
"description": "name of the PodProxyOptions",
"in": "path",
"name": "name",
"required": true,
"type": "string",
"uniqueItems": true
},
{
"description": "object name and auth scope, such as for teams and projects",
"in": "path",
"name": "namespace",
"required": true,
"type": "string",
"uniqueItems": true
},
{
"description": "path to the resource",
"in": "path",
"name": "path",
"required": true,
"type": "string",
"uniqueItems": true
},
{
"description": "Path is the URL path to use for the current proxy request to pod.",
"in": "query",
"name": "path",
"type": "string",
"uniqueItems": true
}
],
"patch": {
"consumes": [
"*/*"
],
"description": "connect PATCH requests to proxy of Pod",
"operationId": "connectCoreV1PatchNamespacedPodProxyWithPath",
"produces": [
"*/*"
],
"responses": {
"200": {
"description": "OK",
"schema": {
"type": "string"
}
},
"401": {
"description": "Unauthorized"
}
},
"schemes": [
"https"
],
"tags": [
"core_v1"
],
"x-kubernetes-action": "connect",
"x-kubernetes-group-version-kind": {
"group": "",
"kind": "PodProxyOptions",
"version": "v1"
}
},
"post": {
"consumes": [
"*/*"
],
"description": "connect POST requests to proxy of Pod",
"operationId": "connectCoreV1PostNamespacedPodProxyWithPath",
"produces": [
"*/*"
],
"responses": {
"200": {
"description": "OK",
"schema": {
"type": "string"
}
},
"401": {
"description": "Unauthorized"
}
},
"schemes": [
"https"
],
"tags": [
"core_v1"
],
"x-kubernetes-action": "connect",
"x-kubernetes-group-version-kind": {
"group": "",
"kind": "PodProxyOptions",
"version": "v1"
}
},
"put": {
"consumes": [
"*/*"
],
"description": "connect PUT requests to proxy of Pod",
"operationId": "connectCoreV1PutNamespacedPodProxyWithPath",
"produces": [
"*/*"
],
"responses": {
"200": {
"description": "OK",
"schema": {
"type": "string"
}
},
"401": {
"description": "Unauthorized"
}
},
"schemes": [
"https"
],
"tags": [
"core_v1"
],
"x-kubernetes-action": "connect",
"x-kubernetes-group-version-kind": {
"group": "",
"kind": "PodProxyOptions",
"version": "v1"
}
}
}Hey Eli, please pardon the delayed reply!
This does seem quite unfortunate, considering that the documentation for the parameters indicates that they are clearly intended for different purposes. This seems like an odd edge case -- I'd honestly say this is more of a bug with the specification, as the path parameter name could be changed to disambiguate without needing to alter the form parameter.
If guardrail generates bogus code, maybe we could emit a warning or an error guiding the user on how to resolve the issue (possibly providing Tracker[_]'s history for all offending parameters), but I'm relieved that we can still rely on the compiler to prevent accidentally shipping said bogus code into production.
If we want to support this kind of behavior, ensuring stable ordering of parameters and suffixing with some sort of disambiguating key, bounded by some context, seems reasonable -- {name} appearing more than once in the path is OK, and should end up with a single parameter that gets interpolated multiple times, but in this case we may end up with path_name and query_name, intentionally using the underscore to avoid generating conflicting bindings, since guardrail would do its best to convert to camel case in that case (also, attempting to communicate "something's strange here" with the parameter names.)
A few users have run into issues where they had no control of specifications given to them, with the unfortunate side effect of guardrail not being able to be used at all. This seems like it may fall into that bucket.
Given your experience with the problem, what would you like to see happen? Hopefully what you need is something that could be broadly applicable to other use cases as well, avoiding one-off hacks that could end up biting us in the future?
(Also, this should probably be a discussion in https://github.com/twilio/guardrail, this is just the SBT plugin, as I've just realized)
(Also, this should probably be a discussion in https://github.com/twilio/guardrail, this is just the SBT
plugin, as I've just realized)
oops, totally right!
I'd honestly say this is more of a bug with the specification, as the path parameter name could be
changed to disambiguate without needing to alter the form parameter.
Yeah, I wonder about that too. Either the Go lib that generates it doesn't follow spec, or this is actually allowed...
A few users have run into issues where they had no control of specifications given to them
Yep thats definitely me ^_^. The k8s spec is 4.5MB in size - no human can control that beast!
Given your experience with the problem, what would you like to see happen?
The disambiguation way would solve this problem for me. I did it manually by editing the api.json, using something like path0 and path, although the path_{name} and query_{name} make sense. Obviously its not sustainable, but it got me to the finish line.
In fact, the client Guardrail generated works! Check it out: https://github.com/soujiro32167/sk8s/blob/master/modules/examples/src/main/scala/com/example/App.scala
(Tested with docker-for-desktop k8s on Mac)
There are things to improve (since I shamelessly took the auth part from skuber), but its a testament to the power of Guardrail
Wild, great to hear!
So, looking at ScalaParameter (misnomer, from when we only supported one language), it would seem as though we already attempt to do some sort of deduplication: https://github.com/twilio/guardrail/blob/242b41992f662694ffca8559b5f3f1a43d1fc84c/modules/codegen/src/main/scala/com/twilio/guardrail/generators/ScalaParameter.scala#L168
I'm trying to get debug logging working, so unfortunately if you're interested in poking around the best way to do so is to just use println.
Funnily enough, your spec helped find a regression for the 0.58.0 release -- I don't have many tests for extraordinarily large specifications currently, so it was very helpful to be able to bisect using yours.
(See guardrail-dev/guardrail#600 (comment) for timing benchmarks)