Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MIGRATED from PR: Add GetVarNames() to get list #18

Open
tebruno99 opened this issue Dec 15, 2022 · 4 comments · May be fixed by #20
Open

MIGRATED from PR: Add GetVarNames() to get list #18

tebruno99 opened this issue Dec 15, 2022 · 4 comments · May be fixed by #20
Assignees
Labels
enhancement New feature or request

Comments

@tebruno99
Copy link

** MIGRATED ** - New maintainer consider own comments below & comments of upstream

Original PR: @eh-steve gorilla#676

Summary of Changes

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

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.

@tebruno99 tebruno99 added the enhancement New feature or request label Dec 15, 2022
@tebruno99
Copy link
Author

I'm not exactly i fully understand the goal. if "item_id" is in the json body with a value of 75, why would the value in the mux route for "item_id" be needed at all? Wouldn't it also just be 75?

Concerns:
This use case only works with JSON.

I think it is an interesting idea generally to have mux expose all variables in a route instead of the current implementation of having to request specific values. Gin has a system that allows depending on some to be present or optional. Having this may result in some interesting general use cases that I'm currently unable to think of.

@eh-steve
Copy link

To be clear, the JSON payload in this example is not being sent in the request body, it is purely being used as a data source for request vars. The field values could just as easily come from struct fields, a map or a database.

@tebruno99
Copy link
Author

I want to look at a few other toolkit's implementation to see if they use a function, array, etc. If I don't see much different or any other good/better ideas I'll merge.

Thanks!

@tebruno99
Copy link
Author

@amustaque97 thanks for your previous review up stream.

@tebruno99 tebruno99 self-assigned this Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants