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

Remove Content-Type for empty responses #1249

Merged
merged 2 commits into from Nov 6, 2017

Conversation

databus23
Copy link
Contributor

@databus23 databus23 commented Nov 6, 2017

This is an attempt at fixing go-openapi/runtime#64.

Some clients trip over responses with empty bodies and Content-Type: application/json as they feed the body directly to a JSON parser. As a zero length string is not a valid JSON this causes a parse error.
As a prominent client jQuery barfs on this intentionally since 1.9: https://jquery.com/upgrade-guide/1.9/#jquery-ajax-returning-a-json-result-of-an-empty-string :(

Note that at the moment this change will cause empty responses to have Content-Type: text/plain set because its what go's http server does by default: golang/go#20784.
Hopefully this will change in later go versions.
Nonetheless this fixes pedantic clients as an empty string is valid text/plain

@codecov
Copy link

codecov bot commented Nov 6, 2017

Codecov Report

Merging #1249 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1249   +/-   ##
=======================================
  Coverage   67.54%   67.54%           
=======================================
  Files          28       28           
  Lines        7812     7812           
=======================================
  Hits         5277     5277           
  Misses       2048     2048           
  Partials      487      487
Impacted Files Coverage Δ
generator/bindata.go 60.32% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc446ee...512184a. Read the comment docs.

databus23 added a commit to sapcc/kubernikus that referenced this pull request Nov 6, 2017
@casualjim casualjim merged commit 22e6510 into go-swagger:master Nov 6, 2017
databus23 added a commit to sapcc/kubernikus that referenced this pull request Nov 7, 2017
databus23 added a commit to sapcc/kubernikus that referenced this pull request Nov 7, 2017
@databus23
Copy link
Contributor Author

Just adding that golang/go#20784 was closed so the spurious text/plain content-type should be gone with go 1.10

@pheepi
Copy link
Contributor

pheepi commented Sep 2, 2020

Unfortunately, this change causes unexpected behavior when the content type is previously set by WithContentType or SetContentType methods. The generated WriteResponse method might look like this:

// WriteResponse to the client
func (o *HeadOK) WriteResponse(rw http.ResponseWriter, producer runtime.Producer) {

	// response header Content-Type

	contentType := o.ContentType
	if contentType != "" {
		rw.Header().Set("Content-Type", contentType)
	}

	rw.Header().Del(runtime.HeaderContentType) //Remove Content-Type on empty responses

	rw.WriteHeader(200)
}

The content type is deleted even if it is wanted. I do not agree that empty response should not carry Content-type header from go-openapi/runtime#64, at least in case of HEAD requests, because it is one of often function.

@vincentvanderweele
Copy link
Contributor

Agree with @pheepi. I'm running in the issue now that the Content-Type header is stripped from my HEAD responses and there is no way to work around this.

As far as I know, WriteResponse is scoped to a model, not an operation, so it cannot decide if this header should be there or not.

Should this change be reverted and the burden moved to the application developer to just not set the Content-Type header if it is undesired?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants