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.

Sign up for GitHub

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

Jump to bottom

Don't set application/json Content-Type on FormData bodies #1053

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Don't set application/json Content-Type on FormData bodies #1053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91b3534
Don't set application/json content-type on formData bodies
macleodmac Feb 28, 2024
28306c9
Merge branch 'main' into fix/handle-formdata-successfully
macleodmac Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,16 @@ Note that when you run commands like `encore run` must use the same `encore` bin

### Testing the Daemon run logic
The codegen tests in the `internal/clientgen/client_test.go` file uses many auto generated files from the
`e2e-tests/testdata` directory. To generate the client files and other test files, run `go test -golden-update` from
the `e2e-tests` directory. This will generate client files for all the supported client generation languages.
`e2e-tests/testdata` directory. To generate the client files and other test files, run:

Running `go test ./internal/clientgen` will now work and use the most recent client generated files. If
you change the client or content of the `testdata` folder, you may need to regenerate the client files again.
```bash
go test ./internal/clientgen -golden-update
go test ./e2e-tests -golden-update
```

This will generate client files for all the supported client generation languages.

Running `go test ./internal/clientgen` will now work and use the most recent client generated files. If you change the client or content of the `testdata` folder, you may need to regenerate the client files again.

## Architecture

Expand Down
13 changes: 9 additions & 4 deletions e2e-tests/testdata/echo_client/js/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,10 +557,8 @@ const boundFetch = fetch.bind(this)
class BaseClient {
constructor(baseURL, options) {
this.baseURL = baseURL
this.headers = {
"Content-Type": "application/json",
}

this.headers = {}

// Add User-Agent header if the script is running in the server
// because browsers do not allow setting User-Agent headers to requests
if (typeof window === "undefined") {
Expand Down Expand Up @@ -601,6 +599,13 @@ class BaseClient {
// Merge our headers with any predefined headers
init.headers = {...this.headers, ...init.headers, ...headers}

// If the body is not FormData, set the Content-Type to application/json
// Otherwise allow fetch to set the Content-Type including the boundary
if (!(body instanceof FormData)) {
init.headers['Content-Type'] = 'application/json';
}


// If authorization data generator is present, call it and add the returned data to the request
let authData
if (this.authGenerator) {
Expand Down
10 changes: 7 additions & 3 deletions e2e-tests/testdata/echo_client/ts/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,9 +799,7 @@ class BaseClient {

constructor(baseURL: string, options: ClientOptions) {
this.baseURL = baseURL
this.headers = {
"Content-Type": "application/json",
}
this.headers = {}

// Add User-Agent header if the script is running in the server
// because browsers do not allow setting User-Agent headers to requests
Expand Down Expand Up @@ -843,6 +841,12 @@ class BaseClient {
// Merge our headers with any predefined headers
init.headers = {...this.headers, ...init.headers, ...headers}

// If the body is not FormData, set the Content-Type to application/json
// otherwise allow fetch to set the Content-Type including the boundary
if (!(body instanceof FormData)) {
init.headers['Content-Type'] = 'application/json';
}

// If authorization data generator is present, call it and add the returned data to the request
let authData: echo.AuthParams | undefined
if (this.authGenerator) {
Expand Down
13 changes: 9 additions & 4 deletions internal/clientgen/javascript.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better approach altogether is to drop the this.headers on the client altogether, and simply have each
endpoint add its own headers. The endpoint is the thing that knows how the body should be encoded, so it probably makes more sense to do it there.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd still need this.headers in order to manage things like user agent.

The tricky thing with managing this on a per endpoint basis is that application/json is a sensible default, and the desired behaviour is that the Content-Type header is unset, rather than overridden, as setting the boundary happens at the fetch level, so even if setting the headers was moved to the endpoint, there would still be an issue.

Keen to hear your thoughts though, I may be missing the mark!

Original file line number Diff line number Diff line change
Expand Up @@ -475,10 +475,8 @@ class BaseClient {`)
js.WriteString(`
constructor(baseURL, options) {
this.baseURL = baseURL
this.headers = {
"Content-Type": "application/json",
}

this.headers = {}

// Add User-Agent header if the script is running in the server
// because browsers do not allow setting User-Agent headers to requests
if (typeof window === "undefined") {
Expand Down Expand Up @@ -524,6 +522,13 @@ class BaseClient {`)

// Merge our headers with any predefined headers
init.headers = {...this.headers, ...init.headers, ...headers}

// If the body is not FormData, set the Content-Type to application/json
// Otherwise allow fetch to set the Content-Type including the boundary
if (!(body instanceof FormData)) {
init.headers['Content-Type'] = 'application/json';
}

`)
w := js.newIdentWriter(2)

Expand Down
13 changes: 9 additions & 4 deletions internal/clientgen/testdata/expected_baseauth_javascript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,8 @@ const boundFetch = fetch.bind(this)
class BaseClient {
constructor(baseURL, options) {
this.baseURL = baseURL
this.headers = {
"Content-Type": "application/json",
}

this.headers = {}

// Add User-Agent header if the script is running in the server
// because browsers do not allow setting User-Agent headers to requests
if (typeof window === "undefined") {
Expand Down Expand Up @@ -146,6 +144,13 @@ class BaseClient {
// Merge our headers with any predefined headers
init.headers = {...this.headers, ...init.headers, ...headers}

// If the body is not FormData, set the Content-Type to application/json
// Otherwise allow fetch to set the Content-Type including the boundary
if (!(body instanceof FormData)) {
init.headers['Content-Type'] = 'application/json';
}


// If authorization data generator is present, call it and add the returned data to the request
let authData
if (this.authGenerator) {
Expand Down
10 changes: 7 additions & 3 deletions internal/clientgen/testdata/expected_baseauth_typescript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,7 @@ class BaseClient {

constructor(baseURL: string, options: ClientOptions) {
this.baseURL = baseURL
this.headers = {
"Content-Type": "application/json",
}
this.headers = {}

// Add User-Agent header if the script is running in the server
// because browsers do not allow setting User-Agent headers to requests
Expand Down Expand Up @@ -211,6 +209,12 @@ class BaseClient {
// Merge our headers with any predefined headers
init.headers = {...this.headers, ...init.headers, ...headers}

// If the body is not FormData, set the Content-Type to application/json
// otherwise allow fetch to set the Content-Type including the boundary
if (!(body instanceof FormData)) {
init.headers['Content-Type'] = 'application/json';
}

// If authorization data generator is present, call it and add the returned data to the request
let authData: string | undefined
if (this.authGenerator) {
Expand Down
13 changes: 9 additions & 4 deletions internal/clientgen/testdata/expected_javascript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,10 +270,8 @@ const boundFetch = fetch.bind(this)
class BaseClient {
constructor(baseURL, options) {
this.baseURL = baseURL
this.headers = {
"Content-Type": "application/json",
}

this.headers = {}

// Add User-Agent header if the script is running in the server
// because browsers do not allow setting User-Agent headers to requests
if (typeof window === "undefined") {
Expand Down Expand Up @@ -314,6 +312,13 @@ class BaseClient {
// Merge our headers with any predefined headers
init.headers = {...this.headers, ...init.headers, ...headers}

// If the body is not FormData, set the Content-Type to application/json
// Otherwise allow fetch to set the Content-Type including the boundary
if (!(body instanceof FormData)) {
init.headers['Content-Type'] = 'application/json';
}


// If authorization data generator is present, call it and add the returned data to the request
let authData
if (this.authGenerator) {
Expand Down
13 changes: 9 additions & 4 deletions internal/clientgen/testdata/expected_noauth_javascript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,8 @@ const boundFetch = fetch.bind(this)
class BaseClient {
constructor(baseURL, options) {
this.baseURL = baseURL
this.headers = {
"Content-Type": "application/json",
}

this.headers = {}

// Add User-Agent header if the script is running in the server
// because browsers do not allow setting User-Agent headers to requests
if (typeof window === "undefined") {
Expand Down Expand Up @@ -119,6 +117,13 @@ class BaseClient {
// Merge our headers with any predefined headers
init.headers = {...this.headers, ...init.headers, ...headers}

// If the body is not FormData, set the Content-Type to application/json
// Otherwise allow fetch to set the Content-Type including the boundary
if (!(body instanceof FormData)) {
init.headers['Content-Type'] = 'application/json';
}


// Make the actual request
const queryString = query ? '?' + encodeQuery(query) : ''
const response = await this.fetcher(this.baseURL+path+queryString, init)
Expand Down
10 changes: 7 additions & 3 deletions internal/clientgen/testdata/expected_noauth_typescript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,7 @@ class BaseClient {

constructor(baseURL: string, options: ClientOptions) {
this.baseURL = baseURL
this.headers = {
"Content-Type": "application/json",
}
this.headers = {}

// Add User-Agent header if the script is running in the server
// because browsers do not allow setting User-Agent headers to requests
Expand Down Expand Up @@ -162,6 +160,12 @@ class BaseClient {
// Merge our headers with any predefined headers
init.headers = {...this.headers, ...init.headers, ...headers}

// If the body is not FormData, set the Content-Type to application/json
// otherwise allow fetch to set the Content-Type including the boundary
if (!(body instanceof FormData)) {
init.headers['Content-Type'] = 'application/json';
}

// Make the actual request
const queryString = query ? '?' + encodeQuery(query) : ''
const response = await this.fetcher(this.baseURL+path+queryString, init)
Expand Down
10 changes: 7 additions & 3 deletions internal/clientgen/testdata/expected_typescript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,9 +459,7 @@ class BaseClient {

constructor(baseURL: string, options: ClientOptions) {
this.baseURL = baseURL
this.headers = {
"Content-Type": "application/json",
}
this.headers = {}

// Add User-Agent header if the script is running in the server
// because browsers do not allow setting User-Agent headers to requests
Expand Down Expand Up @@ -503,6 +501,12 @@ class BaseClient {
// Merge our headers with any predefined headers
init.headers = {...this.headers, ...init.headers, ...headers}

// If the body is not FormData, set the Content-Type to application/json
// otherwise allow fetch to set the Content-Type including the boundary
if (!(body instanceof FormData)) {
init.headers['Content-Type'] = 'application/json';
}

// If authorization data generator is present, call it and add the returned data to the request
let authData: authentication.AuthData | undefined
if (this.authGenerator) {
Expand Down
10 changes: 7 additions & 3 deletions internal/clientgen/typescript.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,9 +702,7 @@ class BaseClient {

constructor(baseURL: string, options: ClientOptions) {
this.baseURL = baseURL
this.headers = {
"Content-Type": "application/json",
}
this.headers = {}

// Add User-Agent header if the script is running in the server
// because browsers do not allow setting User-Agent headers to requests
Expand Down Expand Up @@ -751,6 +749,12 @@ class BaseClient {

// Merge our headers with any predefined headers
init.headers = {...this.headers, ...init.headers, ...headers}

// If the body is not FormData, set the Content-Type to application/json
// otherwise allow fetch to set the Content-Type including the boundary
if (!(body instanceof FormData)) {
init.headers['Content-Type'] = 'application/json';
}
`)
w := ts.newIdentWriter(2)

Expand Down