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

CP-1939 Allow content-type to be set manually #153

Merged
merged 1 commit into from
Jun 21, 2016

Conversation

evanweible-wf
Copy link
Contributor

Improvement

Currently every request's content-type is set automatically (with the exception of StreamedRequest) based on the request type and the encoding. This is too inflexible as there are valid use cases for wanting to override the content-type like integration with a server that expects a certain content-type.

It was requested by @stevenosborne-wf that we add support for manually setting the content-type.

Changes

  • FormRequest.contentType, JsonRequest.contentType, and Request.contentType can now be set manually.

  • MultipartRequest.contentType still cannot be set manually because a multipart request requires a specific content-type with a boundary string parameter.

  • StreamedRequest.contentType was already overridable by consumers since the content is not known prior to opening the request. This is still the case, but I fixed a bug where we weren't verifying that the request had not yet been sent when setting the content-type.

  • Previously, the content-type would be automatically updated when the encoding was changed. This is still the default behavior, but once a user manually sets the content-type, this will stop. In other words, we are assuming that a consumer who sets the content-type manually is intentionally overriding the defaults and is taking responsibility of setting the charset parameter appropriately.

  • Technically, since the StreamedRequest already allowed consumers to manually set the content-type but did not enforce the behavior described in the previous point, the expected behavior of existing code could change in only this exact scenario:

    var streamedRequest = new StreamedRequest()
      ..uri = Uri.parse('/example')
      ..contentLength = 100
      ..contentType = new MediaType('application', 'octet-stream')
      ..encoding = UTF8;

    Previously, setting encoding after contentType would have triggered an update to the content-type to include a charset: utf-8 parameter, but after this change, this will no longer happen.

Testing

  • CI passes (tests updated and new tests added, should be 100% patch coverage)
  • Examples still work as expected

Code Review

@trentgrover-wf
@maxwellpeterson-wf
@dustinlessard-wf
@jayudey-wf
@sebastianmalysa-wf
@srinivasdhanwada-wf

fyi: @stevenosborne-wf

@evanweible-wf evanweible-wf added this to the 2.6.0 milestone Jun 16, 2016
@evanweible-wf evanweible-wf changed the title Allow content-type to be set manually CP-1939 Allow content-type to be set manually Jun 16, 2016
@maxwellpeterson-wf
Copy link
Member

+1

@stevenosborne-wf
Copy link

Thanks!

@sebastianmalysa-wf
Copy link

+1 once conflicts are resolved

@evanweible-wf evanweible-wf force-pushed the content-type-override branch from e819788 to 13239d4 Compare June 20, 2016 16:22
@evanweible-wf
Copy link
Contributor Author

@maxwellpeterson-wf @sebastianmalysa-wf Rebased against latest master and fixed a conflict in CHANGELOG.md.

@maxwellpeterson-wf
Copy link
Member

+1

@codecov-io
Copy link

Current coverage is 99.53%

Merging #153 into master will increase coverage by <.01%

Powered by Codecov. Last updated by eb7cb16...13239d4

@sebastianmalysa-wf
Copy link

+1

@evanweible-wf
Copy link
Contributor Author

@jayudey-wf ready for merge

@jayudey-wf
Copy link
Contributor

jayudey-wf commented Jun 21, 2016

QA Resource Approval: +10

Merging into master.

@jayudey-wf jayudey-wf merged commit e26ee3e into master Jun 21, 2016
@evanweible-wf evanweible-wf deleted the content-type-override branch July 22, 2016 22:55
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.

6 participants