contrib/internal/httputil: generate all possible http interfaces for the response writer wrapper #278

dd-caleb · 2018-07-24T22:29:46Z

This fixes: #256

So here's the idea. We need to wrap the WriteHeader call because that's really the only place to get the http response code. But we also want to make sure we don't accidentally remove interface implementations on the response writer (Flusher, Hijacker, etc).

One solution would be to implement a super struct:

w = struct {
	http.ResponseWriter
	http.Flusher
	http.Pusher
	http.CloseNotifier
	http.Hijacker
}{w, hFlusher, hPusher, hCloseNotifier, hHijacker}

Because of field embedding we can call the methods of each interface on the outer struct (w.Flush() -> w.Flusher.Flush()), which in turn makes it so that we now actually implement each of the interfaces.

Unfortunately this won't work because if we didn't already implement one of the http interfaces, we would falsely indicate to the calling code that we did. For example client code might have:

if flusher, ok := w.(http.Flusher); ok {

Which would now return true even though we didn't actually implement the method originally.

So to overcome this problem I wrote a script which generates all the possible combinations and creates an anonymous struct for each case implementing only the interfaces that the original response writer did.

Does that make sense?

gbbr · 2018-07-25T09:11:53Z

contrib/internal/httputil/gen/gen.go

+// This code is generated because we have to account for all the permutations
+// of the interfaces.
+func wrapResponseWriter(w http.ResponseWriter, span ddtrace.Span) http.ResponseWriter {
+	`)


Would you mind using a template here? This is too cryptic and hard to follow.

gbbr · 2018-07-25T09:49:07Z

This is a great solution. I love it. But please use a template so it's easy to see and work with the generated output.

gbbr · 2018-07-25T10:10:57Z

contrib/internal/httputil/trace_gen.go

@@ -0,0 +1,119 @@
+// Code generated by ./gen do not modify


Let's respect the standard comment heading for generated files outlined here: golang/go#13560 (comment)

gbbr

This is a fantastic solution

gbbr · 2018-07-31T08:27:26Z

contrib/internal/httputil/gen/gen.go

+	})
+}
+
+func combinations(strs []string, pick int) (all [][]string) {


I've spent quite a lot of time here trying to make sense of this algorithm. Even though it looks short and witty, and possibly a well known algorithm for computer science aficiandos, I'd expect a large percentage of people visiting this code to be like me and not understand what's going on without spending time.

Would it be possible to augment it with documentation describing in detail how it works? Or, if it's a common algorithm that has a Wiki page, simply to link to it?

I was also wondering if we could rename strs to list. "strs" makes me think "strings" which is unclear when reading. But maybe it's just me.

gbbr · 2018-07-31T08:34:06Z

contrib/internal/httputil/gen/gen.go

+
+func combinations(strs []string, pick int) (all [][]string) {
+	switch pick {
+	case 0:


I usually like to put a comment in switch cases like this. For example:

case 0: // nothing to do

gbbr · 2018-07-31T08:37:44Z

contrib/internal/httputil/gen/gen.go

+		combos = append(combos, combinations(interfaces, pick))
+	}
+
+	template.Must(template.ParseFiles("./gen.gohtml")).Execute(os.Stdout, map[string]interface{}{


Thanks for being patient again. And I'm sorry for being unclear. I think it would be nicer, easier to handle and make sense of this code, if both the template text and the Go code would be in the same file. Basically, at the end of the Go code we'd put a const template=... with a backtick, like here or here.

Just like in those two examples, we could also just keep this in the httputil folder using the // +build !ignore flag. All in one file: make_responsewriter.go (or similar). I found this worked well in the past, avoiding the need for a new package.

gbbr · 2018-07-31T08:42:11Z

contrib/internal/httputil/gen/gen.gohtml

+	w = newResponseWriter(w, span)
+	switch {
+{{- range .Combinations }}
+{{- range . }}


Could we possibly indent all these {{ - range . }} and {{- end }} commands? It's slightly difficult to see where each starts and ends.

Also, what is this - prefix? I've not used it before.

The - prefix removes whitespace before the block.

gbbr · 2018-07-31T08:44:45Z

contrib/internal/httputil/gen/gen.go

@@ -0,0 +1,36 @@
+package main


Let's document this package and explain what it does. It shouldn't be a package level documentation, it should be the documentation of this utility, something like "This program generates the implementation of all possible combinations of the http.ResponseWriter's interface implementations [...]" and so on. To make it clear that it's not package level documentation we should add a space between the documentation and package declaration.

dd-caleb · 2018-07-31T14:11:41Z

@gbbr thanks for the review. I renamed the code generated and added some additional documentation.

Sorry that combinations was unclear. It's an algorithm I have a hard time reasoning about, so I tried to add an explanation of what it's doing and how it works.

gbbr

Thanks for adding the explanation. It makes a huge difference. I am wondering if it would be fun to generate the tests for this code too, using the same combinations.

It would also be nice to have a basic unit test for the "combinations" function.

gbbr · 2018-08-01T08:03:41Z

contrib/internal/httputil/make_responsewriter.go

+}
+
+// combinations returns all possible unique selections of size `pick` of a list
+// of strings for which order does not matter: wikipedia.org/wiki/Combination


Not sure if the link adds much. It doesn't link to this specific algorithm implemented here. If we use a link it should serve as a shortcut to understanding this algorithm. The wiki page is too vast.

gbbr · 2018-08-01T08:22:49Z

contrib/internal/httputil/make_responsewriter.go

+	switch {
+{{- range .Combinations }}
+	{{- range . }}
+	case {{ range $i, $v := . }}{{ if gt $i 0 }} && {{ end }}ok{{.}}{{ end }}:


Is it possible to remove $v here just like in Go code? Not sure. It's unused.

…the response writer wrapper

- update comment to match convention - add more tests

…dd additional documentation explaining what it does and how it works

…ackage so it can be tested

dd-caleb · 2018-08-01T14:46:13Z

@gbbr I moved the combinations function to a separate, internal lists package so that I could add a test for it

gbbr

Thanks!

…the response writer wrapper (DataDog#278)

gbbr reviewed Jul 25, 2018

View reviewed changes

gbbr added this to the 1.1.0 milestone Jul 25, 2018

gbbr added the bug unintended behavior that has to be fixed label Jul 25, 2018

gbbr reviewed Jul 25, 2018

View reviewed changes

gbbr reviewed Jul 31, 2018

View reviewed changes

gbbr reviewed Aug 1, 2018

View reviewed changes

dd-caleb added 4 commits August 1, 2018 10:16

contrib/internal/httputil: generate all possible http interfaces for …

f7a01f5

…the response writer wrapper

contrib/internal/httputil: refactor generator script to use a template

482424b

- update comment to match convention - add more tests

contrib/internal/httputil: rename gen.go to make_responsewriter.go, a…

89d07c1

…dd additional documentation explaining what it does and how it works

contrib/internal/httputil: move combinations function to a separate p…

4ae3cdd

…ackage so it can be tested

dd-caleb force-pushed the caleb/response-writer branch from 61428f2 to 4ae3cdd Compare August 1, 2018 14:45

gbbr approved these changes Aug 1, 2018

View reviewed changes

Merge branch 'v1' into caleb/response-writer

6676d65

dd-caleb merged commit 588a6ff into v1 Aug 1, 2018

dd-caleb deleted the caleb/response-writer branch August 1, 2018 15:00

mingrammer pushed a commit to mingrammer/dd-trace-go that referenced this pull request Dec 22, 2020

contrib/internal/httputil: generate all possible http interfaces for …

1d5bccf

…the response writer wrapper (DataDog#278)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib/internal/httputil: generate all possible http interfaces for the response writer wrapper #278

contrib/internal/httputil: generate all possible http interfaces for the response writer wrapper #278

dd-caleb commented Jul 24, 2018

gbbr Jul 25, 2018

gbbr commented Jul 25, 2018

gbbr Jul 25, 2018

gbbr left a comment

gbbr Jul 31, 2018

gbbr Jul 31, 2018

gbbr Jul 31, 2018 •

edited

Loading

gbbr Jul 31, 2018

dd-caleb Jul 31, 2018

gbbr Jul 31, 2018 •

edited

Loading

dd-caleb commented Jul 31, 2018

gbbr left a comment

gbbr Aug 1, 2018

gbbr Aug 1, 2018

dd-caleb commented Aug 1, 2018

gbbr left a comment

contrib/internal/httputil: generate all possible http interfaces for the response writer wrapper #278

contrib/internal/httputil: generate all possible http interfaces for the response writer wrapper #278

Conversation

dd-caleb commented Jul 24, 2018

Choose a reason for hiding this comment

gbbr commented Jul 25, 2018

Choose a reason for hiding this comment

gbbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbbr Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbbr Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

dd-caleb commented Jul 31, 2018

gbbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dd-caleb commented Aug 1, 2018

gbbr left a comment

Choose a reason for hiding this comment

gbbr Jul 31, 2018 •

edited

Loading

gbbr Jul 31, 2018 •

edited

Loading