Custom http response headers

[rigved-telang]

Description:

• Added a new struct called Response which can be used to set custom headers to be sent back with response
• Closes how to return custom http response data and header  #1130

Additional Information:

• Usage:

headers := map[string]string{
	"X-Custom-Header":  "CustomValue",
	"X-Another-Header": "AnotherValue",
}

return response.Response{
	Data:    "Hello World from new Server",
	Headers: headers,
}, nil

Checklist:

• I have formatted my code using goimport and golangci-lint.
• All new code is covered by unit tests.
• This PR does not decrease the overall code coverage.
• I have reviewed the code comments and documentation for clarity.

[shashank-zopsmart]

Any specific reason to update the go.sum?

[rigved-telang]

None. When I ran go install golang.org/x/tools/cmd/goimports@latest, it updated the go.sum, I can. remove it if you think it is necessary.

[shashank-zopsmart]

Sure, in my opinion, go.mod & go.sum should only be updated if there are dependency update that are adding some value to the feature/enhancement/bug fix in the PR

[rigved-telang]

Understood. Will remove the additions in go.sum.

[shashank-zopsmart]

Can we add logic to skip headers that gofr already sets, such as 'Content-type'?

[rigved-telang]

if resp, ok := result.(response.Response); ok {
	for key, value := range resp.Headers {
		if w.Header().Get(key) == "" {
			w.Header().Set(key, value)
		}
	}
	result = resp.Data
}

How about this, we'll check if a Header has been set and then set it only if it hasn't been set.

[Umang01-hash]

Yes maybe this can help

[shashank-zopsmart]

LGTM.

[shashank-zopsmart]

@Umang01-hash @aryan-mehrotra-zs @vipul-rawat How about having a list of non overridable response headers (or response headers that can only be overridden by some special methods provided by Gofr)? So that a check can be implemented for those headers only. Intention behind is that, user deliberately wants to override those headers (such as content-type) but since Gofr is internally setting those headers, so the overall working of the framework should not break.

PS: If required, this can be done as future enhancement. For now, this PR is good.

[aryanmehrotra]

I would suggest if the user has provided some header which is already set by gofr that should also be over-ridden, if user doesn't set - the default which is set by gofr is used.

[rigved-telang]

@ccoVeille @aryanmehrotra @Umang01-hash @shashank-zopsmart I have extracted logError and handleWebSocketUpgrade to reduce the Cognitive Complexity error which was being raised by codeclimate. Kindly review that as well.

[ccoVeille]

I'm unsure about the formatting here

In a godoc, I would have understand

Here I don't get what it brings.

I might not have raised this, but I also face a misunderstanding about the fact I don't get why there is a need to use emphasis on this

I could have understood if it was

Suggested change

	// **Handle Custom Headers** if `result` is a `Response`.
	// handle **Custom Headers** if `result` is a `Response`.

But then, I would be lost anyway. Why would we need to emphasis on the fact it's custom headers, maybe you could explain

And finally, why using emphasis and not simply use quote.

Suggested change

	// **Handle Custom Headers** if `result` is a `Response`.
	// handle "Custom Headers" if `result` is a `Response`.

Thanks for reading this almost useless feedback about a comment 😅😬😂

[rigved-telang]

Hey @ccoVeille there's no particular reason for the formatting, I am new to Go and hence was taking ChatGPT's help while writing the code, it put the comment, I didn't really think about the way the comment was formatted. I will simplify it to just read //handle custom headers if 'result' is 'Response'

[ccoVeille]

Maybe a comment explaining the reason behind this.

Here is my understanding:

There is a need to propagate the header to the caller.

Then explain existing headers are not overwritten.

For this exact reason, I would create a method in

Suggested change

	if resp, ok := result.(response.Response); ok {
	for key, value := range resp.Headers {
	if w.Header().Get(key) == "" {
	w.Header().Set(key, value)
	}
	}
	result = resp.Data
	}
	if resp, ok := result.(response.Response); ok {
	resp.PropagateHeaders(w)
	result = resp.Data
	}

With something like this in response.Response struct

func(response.Response resp) PropagateHeaders(w *http.Response)
	for key, value := range resp.Headers {
		if w.Header().Get(key) != "" {
			// do not overwrite existing header
			continue
		}
		w.Header().Set(key, value)
	}
}

I wrote this on a phone, there might be some errors 😅

[rigved-telang]

I think this approach is better. I just think the function should be called SetCustomHeaders instead of PropogateHeaders. That would explain the purpose better. Let me know what you think.

[rigved-telang]

@shashank-zopsmart @Umang01-hash what do you guys think, should we extract the setting of custom headers to another function or a simple if condition should suffice in the ServerHTTP func itself

[Umang01-hash]

@rigved-telang extracting the setting of custom headers to another function is not a bad idea as it would lead to a more clear code and auto explain what the function is intended to do

[ccoVeille]

Suggested change

	if err != nil {
	errorLog := &ErrorLogEntry{TraceID: traceID, Error: err.Error()}
	h.container.Logger.Error(errorLog)
	}
	if err == nil {
	return
	}
	errorLog := &ErrorLogEntry{TraceID: traceID, Error: err.Error()}
	h.container.Logger.Error(errorLog)

[rigved-telang]

I'd just extracted the existing code to a new function, also isn't the logic wrong here, the error should get logged when it is not nil right?

[ccoVeille]

Indeed, I edited my post it should be OK now

[ccoVeille]

Suggested change

	data interface{}
	data any

[Umang01-hash]

@rigved-telang can you please also resolve this linter ?