Encoding of ID in GET URL is wrong for spaces

[jmazzitelli]

I had a space in my metric ID. Some of the APIs were not working (it was saying the metric didn't exist when it did, for example).

I believe the problem is here:

https://github.com/hawkular/hawkular-client-go/blob/master/metrics/client.go#L697

// SingleMetricEndpoint is a URL endpoint for requesting single metricID
func SingleMetricEndpoint(id string) Endpoint {
    return func(u *url.URL) {
        addToURL(u, url.QueryEscape(id))  // <----- look here!
    }
}

This is using "QueryEscape" which is for escaping strings for use in the query string portion of a URL - NOT within a path - but unfortunately, it is being used to encode path strings as in here:

https://github.com/hawkular/hawkular-client-go/blob/master/metrics/client.go#L336

// . But this is used for escaping strings for use in a path, lDefinition returns a single metric definition
func (c *Client) Definition(t MetricType, id string, o ...Modifier) (*MetricDefinition, error) {
    o = prepend(o, c.URL("GET", TypeEndpoint(t), SingleMetricEndpoint(id)))

Which is the place where I was seeing my error. I had a metric Id like "My Metric ID", which gets encoded as "My+Metric+ID". The GET URL that gets built here looked like this:

http://192.168.1.15:8080/hawkular/metrics/counters/My+Metric+ID

This causes the lookup to not find my metric even though it exists. If I change the code such that the URL used %20 instead of + for the space encoding, it worked and my metric was found:

http://192.168.1.15:8080/hawkular/metrics/counters/My%20Metric%20ID

My quick and dirty code change that saw it work by encoding with %20 was:

// SingleMetricEndpoint is a URL endpoint for requesting single metricID
func SingleMetricEndpoint(id string) Endpoint {
    return func(u *url.URL) {
        escaped := url.QueryEscape(id)
        escaped = strings.Replace(escaped, "+", "%20", -1) // <-- replace the + with %20
        addToURL(u, escaped)
    }
}

[jmazzitelli]

FYI: You can see this bug if you edit client_test.go and change the tag "ab" to "a b" (i.e. put a space between "a" and "b" in the tag name) : https://github.com/hawkular/hawkular-client-go/blob/master/metrics/client_test.go#L180

// Add tags
tags := make(map[string]string)
tags["a b"] = "ac" // <-- change the tag name key from "ab" to "a b"
...

[burmanm]

Yes, this is a bug in the Golang golang/go#4013

[jmazzitelli]

I can't figure it out - why was the Golang issue closed? Did they actually fix this? I don't see anywhere in that issue where they report its actually fixed?

UPDATE - bah, never mind. QueryEscape is working and why that issue probably was closed - its escaping fine for query strings. We just need a way to escape the PATH because that's where the string is going to go.

[jmazzitelli]

I suggest, then, that we implement the fix by replacing space with %20 since apparently that is valid for both paths and query strings:

// SingleMetricEndpoint is a URL endpoint for requesting single metricID
func SingleMetricEndpoint(id string) Endpoint {
    return func(u *url.URL) {
        escaped := url.QueryEscape(id)
        escaped = strings.Replace(escaped, "+", "%20", -1) // <-- replace the + with %20
        addToURL(u, escaped)
    }
}

Of course, I have no idea if this is good enough because what if the string has a literal "+" in it??? We need to think about this some more.

[burmanm]

A bug for HWKMETRICS was logged to HWKMETRICS-530 (+ sign isn't handled correctly on the server side)

[mpalmer]

what if the string has a literal "+" in it?

url.QueryEscape should turn a literal + into %2B, so the strings.Replace (which should really be strings.ReplaceAll) won't replace it.