Add GetVarNames()

[eh-steve]

Summary of Changes

1. Added r.GetVarNames() function to list all vars a route might need in order to call r.URL()

[amustaque97]

I will review PR by this weekend.

[amustaque97]

Var names are defined on the application level. Developer will know the variable name. @eh-steve do you know of any such scenario where a developer has no idea of the variable names? So they can use this method to get names.

Moreover, to call r.URL() method arguments should be passed like the below snippet.

r := mux.NewRouter()
r.Host("{subdomain}.example.com").
  Path("/articles/{category}/{id:[0-9]+}").
  Queries("filter", "{filter}").
  HandlerFunc(ArticleHandler).
  Name("article")

// url.String() will be "http://news.example.com/articles/technology/42?filter=gorilla"
url, err := r.Get("article").URL("subdomain", "news",
                                 "category", "technology",
                                 "id", "42",
                                 "filter", "gorilla")

So, URL takes parameters like key, value, key, value, key, value not []string slice from GetVarNames directly. I'm not able to understand the proper use case of method r.GetVarNames. I don't see anyone who has requested something similar in the past also.

What do you think? This will increase our codebase and test cases.

[eh-steve]

The reason for this is where routes are built up programmatically from a configuration, not hard coded by a developer. In this case, the application needs to know the registered vars to be able to ensure they are available to be injected (or retrieve them if necessary)

This may not be a common use case but we're using mux in this way.

[amustaque97]

we're using mux in this way.

Glad to know your use case 😃 but I don't see this as the right time to add this implementation to the project because other users might be building routes programmatically, and their requirements, approaches or expectations could be entirely different from this. It's more based on individual choices.

What do you think?

[eh-steve]

I'm not sure I follow what you mean about choices - right now, there's no way of knowing which vars a route captures (on the basis that the caller might not be the one who built the route), and this adds a fairly non-intrusive way to expose that information.

An alternative choice for would be to force users to make another struct which embeds the mux route and keeps track of all var names by duplicating/wrapping a lot of functionality?

If we wanna freeze the API of this library that's fine, you can close this PR and I can use the fork?

[amustaque97]

If we wanna freeze the API of this library that's fine

Not want to make any premature decision here. If you don't mind could you please share the minimal code example(building of URL programmatically) where the current PR method GetVarNames comes into play? Later we can discuss whether we should merge this PR or not.

[eh-steve]

Sure, so:

Imagine an application that makes HTTP requests based on variables coming from an inbound messages (e.g. from a kafka topic). It could take a set of YAML configurations like:

endpoint:
  host_pattern: "www.{domain}.com"
  path_pattern: "/some/prefix/{group}/{item_id:[0-9]+}"
  method: "POST"
  queries:
    blah: "{some_data1:[0-9]+}"
    something: "{some_data2:[0-9]+}"

and builds up mux routes programatically using these configured endpoints.

Then when a payload comes in from an inbound message like:

{
  "domain": "example",
  "group": "my_group",
  "item_id": 75,
  "some_data1": 123,
  "some_data2": 456,
  "other_irrelevant_data": {
    "massive_and_expensive_to_stringify": "..."
  }
}

It would be able to extract keys from the JSON payload (parsed as a map[string]interface{}) and use them as URL vars. Mux vars values need to be strings, so any non-string values would need to be fmt.Sprint()'ed (e.g. item_id in this example).

If your JSON payload contains any additional fields which aren't needed as route vars (e.g. other_irrelevant_data in this example, which contains the field massive_and_expensive_to_stringify), we would still need to stringify them all (which involves a needless allocation) and pass them into route.URL(pairs...), where they would be ignored anyway.

By exposing GetVarNames(), it allows us to only pass variables which we know are required by the route, and to error early if those variables are not available, or don't adhere to a schema etc.

This is a simplified example compared to how we're actually using it, but it illustrates the point.

[jrs526]

This seems like a harmless change that could benefit some users. I think in this case, the change would have to be identified as a maintenance risk to justify NOT accepting the PR.

[amustaque97]

I was checking this thoroughly before accepting or rejecting the changes. Here is the minimal code I used:

package main

import (
	"fmt"
	"io"
	"net/http"

	"github.com/gorilla/mux"
)

func Func(w http.ResponseWriter, r *http.Request) {
	io.WriteString(w, "hello")
}

func main() {

	r := mux.NewRouter()
	r.Host("{domain}").
		Path("/{group}/{item_id}").
		Queries("some_data1", "{some_data1}").
		Queries("some_data2", "{some_data2}").
		HandlerFunc(Func).
		Name("article")
	fmt.Println("Printing main method vars")
	fmt.Println(r.Get("article").GetHostTemplate())
	fmt.Println(r.Get("article").GetPathTemplate())
	fmt.Println(r.Get("article").GetQueriesTemplates())
	fmt.Println("--------------------------------------")
	fmt.Println(r.Get("article").GetVarNames())
	fmt.Println("--------------------------------------")
	http.ListenAndServe(":8000", r)

}

CURL command:

➜  curl http://localhost:8000/my_group/123\?some_data1\=abc\&some_data2\=def                                  
hello%  

➜ go run main.go
Printing main method vars
{domain} <nil>
/{group}/{item_id} <nil>
[some_data1={some_data1} some_data2={some_data2}] <nil>
--------------------------------------
[domain group item_id some_data1 some_data2] <nil>
--------------------------------------

Without GetVarNames we get output is {domain}, {group}/{item_id} by calling GetHostTemplate and GetPathTemplate respectively.

To remove curly braces {} and slash / writing half of the function identical to newRouteRegexp: https://github.com/gorilla/mux/blob/master/regexp.go#L41 doesn't sounds like a good idea and is more likely to be error-prone if something gets missed or wrong implemented.

So I'm okay with the changes. 👍🏻

[eh-steve]

To remove curly braces {} and slash / writing half of the function identical to newRouteRegexp: https://github.com/gorilla/mux/blob/master/regexp.go#L41 doesn't sounds like a good idea

That's how I first started (on the calling side) before I realised it was a bad idea!

[amustaque97]

One last comment I have added, and then I will approve the PR.

@eh-steve Good work! Thanks for quickly tackling this.

[amustaque97]

Hi @elithrar, please merge this PR. 👍🏻

Notice that there is no CI run for this PR. We need to fix CI.

[codecov]

Codecov Report

Merging #676 (21c25ef) into main (85123bf) will increase coverage by 0.03%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #676      +/-   ##
==========================================
+ Coverage   78.01%   78.04%   +0.03%     
==========================================
  Files           5        5              
  Lines         887      902      +15     
==========================================
+ Hits          692      704      +12     
- Misses        140      142       +2     
- Partials       55       56       +1     

Files Changed	Coverage Δ
route.go	68.93% <80.00%> (+0.47%)	⬆️

[coreydaley]

@apoorvajagtap Can you please take a look at this an approve if it looks good?