Add append header modifier #247

hnovikov · 2018-05-30T20:18:05Z

No description provided.

bramhaghosh · 2018-05-30T20:51:22Z

header/header_modifier.go

+	return m.setOrAddHeader(proxyutil.ResponseHeader(res))
+}
+
+func (m *modifier) setOrAddHeader(h *proxyutil.Header) error {


no need to pull this out in a function - it's fine to just have this in ModifyRequest / ModifyResponse

bramhaghosh · 2018-05-30T20:52:06Z

header/header_modifier.go

+func (m *modifier) setOrAddHeader(h *proxyutil.Header) error {
+	if m.add {
+		return h.Add(m.name, m.value)
+	} else {


don't need the "else" - have the Set happen in the normal flow

bramhaghosh · 2018-05-30T20:53:19Z

header/header_modifier.go

 }

 // NewModifier returns a modifier that will set the header at name with
 // the given value for both requests and responses. If the header name already
 // exists all values will be overwritten.
-func NewModifier(name, value string) martian.RequestResponseModifier {
+func NewModifier(name, value string, add bool) martian.RequestResponseModifier {


rather than have this in the constructor, just let the default behavior be Set (same as current)

Add a func on modifier that changes it called SetBehavior

bramhaghosh · 2018-05-30T21:11:53Z

header/header_modifier.go

@@ -29,31 +29,42 @@ func init() {

 type modifier struct {
 	name, value string
+	add         bool


I'm trying to think of a better way to name / represent this...

We have 2 "modes" - Append or Replace - so maybe instead of the add bool, we could have a Mode that's an int that represents the modes that are defined with something like:

const ( ReplaceValues = iota AppendValues )

and then the struct could be:

type modifier struct { name, value string behavior int }

What would that look like on the wire?

{ "name": ... ... "behavior": "replace" | "append" }

?

Another option might be to still have a boolean flag but invert it and default it to true

{ "name": ... ... "replace": false }

Yeah, I was thinking there would be a "behavior" key that was either "replace" or "append"

Your modifierJSON struct would have a

Behavior string `json:"behavior"`

and then in modifierFromJSON you'd have a check like

if msg.Behavior == "append" { modifier.SetBehavior(AppendValues) }

bramhaghosh · 2018-05-30T23:21:39Z

header/header_modifier.go

@@ -27,31 +27,52 @@ func init() {
 	parse.Register("header.Modifier", modifierFromJSON)
 }

-type modifier struct {
+type HeaderSetBehavior int


Add comment for exported type
https://github.com/golang/go/wiki/CodeReviewComments#doc-comments

bramhaghosh · 2018-05-30T23:21:59Z

header/header_modifier.go

+type HeaderSetBehavior int
+
+const (
+	ReplaceValues HeaderSetBehavior = iota


Add comment for exported type
https://github.com/golang/go/wiki/CodeReviewComments#doc-comments

bramhaghosh · 2018-05-30T23:22:20Z

header/header_modifier.go

+	AppendValues
+)
+
+type Modifier struct {


Add comment for exported type
https://github.com/golang/go/wiki/CodeReviewComments#doc-comments

bramhaghosh · 2018-05-30T23:24:22Z

header/header_modifier.go

-func NewModifier(name, value string) martian.RequestResponseModifier {
-	return &modifier{
+func NewModifier(name, value string) *Modifier {
+	return &Modifier{


set the default behavior here:

mod := &Modifier{Name: http.CanonicalHeaderKey(name), Value: value} mod.SetBehavior(ReplaceValues) return mod

bramhaghosh · 2018-05-30T23:25:07Z

header/header_modifier.go

+}
+
+func (m *Modifier) SetBehavior(b HeaderSetBehavior) {
+	m.behavior = b
 }

 // NewModifier returns a modifier that will set the header at name with
 // the given value for both requests and responses. If the header name already


update godoc to say that "by default, if header name already exists..."

bramhaghosh · 2018-05-30T23:28:51Z

header/header_modifier.go

@@ -74,5 +95,13 @@ func modifierFromJSON(b []byte) (*parse.Result, error) {

 	modifier := NewModifier(msg.Name, msg.Value)

+	if msg.Behavior == "" || msg.Behavior == "replace" {


if the default behavior is set in NewModifier, you should only need to do:

if strings.ToLower(msg.Behavior) == "append" { modifier.SetBehavior(AppendValues) }

You could just assume that anything that isn't append gives you the default replace behavior.

But, if you'd like to be explicit and return an error on a value that is neither append or default, use a switch:

switch strings.ToLower(msg.Behavior) { case "append": modifier.SetBehavior(AppendValues) case "replace": modifier.SetBehavior(ReplaceValues) default: return nil, fmt.Errorf("Invalid header modifier behavior %q. Must be either %q or %q", msg.Behavior, "append", "replace") }

Went with a switch here, I think silently ignoring the other val would be annoying to debug

bramhaghosh · 2018-05-30T23:38:18Z

header/header_modifier.go

+	return h.Set(m.name, m.value)
+}
+
+func (m *Modifier) SetBehavior(b HeaderSetBehavior) {


Add comment for exported func
https://github.com/golang/go/wiki/CodeReviewComments#doc-comments

bramhaghosh · 2018-05-31T21:47:43Z

header/header_modifier.go

@@ -27,13 +27,17 @@ func init() {
 	parse.Register("header.Modifier", modifierFromJSON)
 }

+// Behavior when setting a new header with regard to duplicate header names.
 type HeaderSetBehavior int


Comment should be of the form:
HeaderSetBehavior sets the behavior of...

bramhaghosh · 2018-05-31T21:48:03Z

header/header_modifier.go

 type HeaderSetBehavior int

 const (
+	// Replace all previous headers with the same canonicalized name.
 	ReplaceValues HeaderSetBehavior = iota


Comment should be of the form:
ReplaceValues replaces ...

bramhaghosh · 2018-05-31T21:48:19Z

header/header_modifier.go

 	ReplaceValues HeaderSetBehavior = iota
+	// Append new header without replacing existing ones.
 	AppendValues


Comment should be of the form:
AppendValues appends...

bramhaghosh · 2018-05-31T21:52:18Z

header/header_modifier.go

-	if msg.Behavior == "" || msg.Behavior == "replace" {
-		modifier.SetBehavior(ReplaceValues)
-	} else if msg.Behavior == "append" {
+	switch msg.Behavior {


downcase this with strings.ToLower(msg.Behavior)

Do we have any other case insensitive tokens in the Martian API? I'd argue against that sort of behavior only in one place (e.g. why make the behavior enumeration case insensitive and not the "behavior" key itself, or all the JSON keys for that matter?) especially since the API errors if the enumeration doesn't match one of the known values.

ah ok yeah that's fair

bramhaghosh · 2018-05-31T21:53:41Z

header/header_modifier.go

-	} else if msg.Behavior == "append" {
+	switch msg.Behavior {
+	case "":
+		fallthrough


we never really use fallthrough do this in Martian, instead:

case "append", "": modifier.SetBehavior(AppendValues)

bramhaghosh · 2018-05-31T22:15:59Z

from golint:

header/header_modifier.go:31:6: type name will be used as header.HeaderSetBehavior by other packages, and that stutters; consider calling this SetBehavior

hnovikov · 2018-05-31T22:19:43Z

Makes the header set behavior setter's signature sorta interesting

func (m *Modifier) SetBehavior(b SetBehavior) {

What about DuplicateBehavior?

// DuplicateBehavior defines the behavior of setting a new header that already exists.
type DuplicateBehavior int

const (
        // ReplaceValues replaces all previous headers with the same canonicalized name.
        ReplaceValues SetBehavior = iota
        // AppendValues appends the new header without replacing existing ones.
        AppendValues
)

func (m *Modifier) SetBehavior(b DuplicateBehavior) {

bramhaghosh · 2018-06-01T20:51:40Z

header/header_append_modifier.go

+	return proxyutil.ResponseHeader(res).Add(m.name, m.value)
+}
+
+// NewAppendModifier returns a appendModifier that will append a header with


add option to append headers

925b2c2

bramhaghosh reviewed May 30, 2018

View reviewed changes

update header append behavior to be an enumeration

885a0f8

hnovikov force-pushed the add-headers branch from c268abe to 885a0f8 Compare May 30, 2018 23:17

bramhaghosh requested changes May 30, 2018

View reviewed changes

hnovikov added 2 commits May 30, 2018 17:48

fix-up documentation and style

607efde

fix default and add test

d6f2d56

bramhaghosh requested changes May 31, 2018

View reviewed changes

remove switch fallthrough

22339e0

bramhaghosh approved these changes May 31, 2018

View reviewed changes

update doc comments

5b08a44

break out separate modifier

9669eba

hnovikov changed the title ~~add option to append headers~~ Add append header modifier Jun 1, 2018

bramhaghosh approved these changes Jun 1, 2018

View reviewed changes

fix grammar

0c7f3be

hnovikov merged commit 74cdea9 into google:master Jun 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add append header modifier #247

Add append header modifier #247

hnovikov commented May 30, 2018

bramhaghosh May 30, 2018

bramhaghosh May 30, 2018

bramhaghosh May 30, 2018

bramhaghosh May 30, 2018

hnovikov May 30, 2018

bramhaghosh May 30, 2018

bramhaghosh May 30, 2018

bramhaghosh May 30, 2018

bramhaghosh May 30, 2018

bramhaghosh May 30, 2018

bramhaghosh May 30, 2018

bramhaghosh May 30, 2018

bramhaghosh May 30, 2018

hnovikov May 31, 2018

bramhaghosh May 30, 2018

bramhaghosh May 31, 2018

bramhaghosh May 31, 2018

bramhaghosh May 31, 2018

bramhaghosh May 31, 2018

hnovikov May 31, 2018

bramhaghosh May 31, 2018

bramhaghosh May 31, 2018

bramhaghosh commented May 31, 2018

hnovikov commented May 31, 2018

bramhaghosh Jun 1, 2018

		@@ -74,5 +95,13 @@ func modifierFromJSON(b []byte) (*parse.Result, error) {

		modifier := NewModifier(msg.Name, msg.Value)

		if msg.Behavior == "" \|\| msg.Behavior == "replace" {

Add append header modifier #247

Add append header modifier #247

Conversation

hnovikov commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bramhaghosh commented May 31, 2018

hnovikov commented May 31, 2018

Choose a reason for hiding this comment