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

Corrected areas of concern. #190

Closed
wants to merge 6 commits into from
+20 −16
Closed

Corrected areas of concern. #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
de55ffb
fix header
Yamashou Oct 10, 2023
62ccbe0
test action for gpt
Yamashou Oct 10, 2023
1752b02
unused
Yamashou Oct 10, 2023
3cfee17
fix
Yamashou Oct 10, 2023
c63c334
delete branches
Yamashou Oct 10, 2023
ab3bb8c
fix
Yamashou Oct 10, 2023
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
11 changes: 8 additions & 3 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,21 @@ func (c *Client) newRequest(ctx context.Context, operationName, query string, va
return nil, fmt.Errorf("create request struct failed: %w", err)
}

req.Header.Set("Content-Type", "application/json; charset=utf-8")
req.Header.Set("Accept", "application/json; charset=utf-8")

for _, httpRequestOption := range c.HTTPRequestOptions {
httpRequestOption(req)
}
for _, httpRequestOption := range httpRequestOptions {
httpRequestOption(req)
}

if req.Header.Get("Content-Type") == "" {
req.Header.Set("Content-Type", "application/json; charset=utf-8")
}

if req.Header.Get("Accept") == "" {
req.Header.Set("Accept", "application/json; charset=utf-8")
}

return req, nil
}

Expand Down
25 changes: 12 additions & 13 deletions clientv2/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,6 @@ type FormField struct {
Value interface{}
}

type header struct {
key, value string
}

// Post support send multipart form with files https://gqlgen.com/reference/file-upload/ https://github.com/jaydenseric/graphql-multipart-request-spec
func (c *Client) Post(ctx context.Context, operationName, query string, respData interface{}, vars map[string]interface{}, interceptors ...RequestInterceptor) error {
multipartFilesGroups, mapping, vars := parseMultipartFiles(vars)
Expand All @@ -156,9 +152,7 @@ func (c *Client) Post(ctx context.Context, operationName, query string, respData

gqlInfo := NewGQLRequestInfo(r)
body := new(bytes.Buffer)

var headers []header

var multipartFilesGroupsContentType string
if len(multipartFilesGroups) > 0 {
contentType, err := prepareMultipartFormBody(
body,
Expand All @@ -178,26 +172,31 @@ func (c *Client) Post(ctx context.Context, operationName, query string, respData
return fmt.Errorf("failed to prepare form body: %w", err)
}

headers = append(headers, header{key: "Content-Type", value: contentType})
multipartFilesGroupsContentType = contentType
Copy link
Owner Author

Choose a reason for hiding this comment

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

@coderabbitai

Copy link

Choose a reason for hiding this comment

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

@Yamashou

Hello! It seems like you've tagged me but didn't provide any specific questions or requests. How can I assist you with this code review?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think the mpartFilesGroupsContentType variable is redundant. Can we build the request without defining this variable?

Copy link

Choose a reason for hiding this comment

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

@Yamashou

We could eliminate the multipartFilesGroupsContentType variable by setting the "Content-Type" header directly within the conditional block where multipart files are processed. Here's how the code would look:

@@ -178,26 +172,31 @@ func (c *Client) Post(ctx context.Context, operationName, query string, respData
 			return fmt.Errorf("failed to prepare form body: %w", err)
 		}
 
-		multipartFilesGroupsContentType = contentType
+		req.Header.Set("Content-Type", contentType)
 	} else {
 		requestBody, err := json.Marshal(r)
 		if err != nil {
 			return fmt.Errorf("encode: %w", err)
 		}
 
 		body = bytes.NewBuffer(requestBody)
 	}
 
 	req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.BaseURL, body)
 	if err != nil {
 		return fmt.Errorf("create request struct failed: %w", err)
 	}
 
-	if req.Header.Get("Content-Type") == "" {
-		if multipartFilesGroupsContentType != "" {
-			req.Header.Set("Content-Type", multipartFilesGroupsContentType)
-		} else {
-			req.Header.Set("Content-Type", "application/json; charset=utf-8")
-		}
+	if req.Header.Get("Content-Type") == "" { 
+		req.Header.Set("Content-Type", "application/json; charset=utf-8")
 	}
 
 	if req.Header.Get("Accept") == "" {
 		req.Header.Set("Accept", "application/json; charset=utf-8")
 	}
 
 	f := ChainInterceptor(append([]RequestInterceptor{c.RequestInterceptor}, interceptors...)...)

This change would directly set the "Content-Type" header when multipart files are present, and if not, it would set the "Content-Type" to "application/json; charset=utf-8" after the request is created. This eliminates the need for the multipartFilesGroupsContentType variable.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That would require that the req be created before the body is created, which seems more verbose.

Copy link

Choose a reason for hiding this comment

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

@Yamashou

You're correct. The http.NewRequestWithContext call needs the body as an argument, so it must be created after the body. My previous suggestion would indeed require the req to be created before the body, which is not ideal.

In that case, we can keep the multipartFilesGroupsContentType variable to hold the content type when multipart files are present. This way, we can create the body and req in their current order, and then set the headers afterwards. This approach maintains the current order of operations and avoids the verbosity of creating the req earlier.

Here's how the code would look:

@@ -178,26 +172,31 @@ func (c *Client) Post(ctx context.Context, operationName, query string, respData
 			return fmt.Errorf("failed to prepare form body: %w", err)
 		}
 
-		multipartFilesGroupsContentType = contentType
+		req.Header.Set("Content-Type", contentType)
 	} else {
 		requestBody, err := json.Marshal(r)
 		if err != nil {
 			return fmt.Errorf("encode: %w", err)
 		}
 
 		body = bytes.NewBuffer(requestBody)
 	}
 
 	req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.BaseURL, body)
 	if err != nil {
 		return fmt.Errorf("create request struct failed: %w", err)
 	}
 
-	if req.Header.Get("Content-Type") == "" {
-		if multipartFilesGroupsContentType != "" {
-			req.Header.Set("Content-Type", multipartFilesGroupsContentType)
-		} else {
-			req.Header.Set("Content-Type", "application/json; charset=utf-8")
-		}
+	if req.Header.Get("Content-Type") == "" { 
+		req.Header.Set("Content-Type", "application/json; charset=utf-8")
 	}
 
 	if req.Header.Get("Accept") == "" {
 		req.Header.Set("Accept", "application/json; charset=utf-8")
 	}
 
 	f := ChainInterceptor(append([]RequestInterceptor{c.RequestInterceptor}, interceptors...)...)

This change would directly set the "Content-Type" header when multipart files are present, and if not, it would set the "Content-Type" to "application/json; charset=utf-8" after the request is created. This eliminates the need for the multipartFilesGroupsContentType variable.

} else {
requestBody, err := json.Marshal(r)
if err != nil {
return fmt.Errorf("encode: %w", err)
}

body = bytes.NewBuffer(requestBody)

headers = append(headers, header{key: "Content-Type", value: "application/json; charset=utf-8"})
headers = append(headers, header{key: "Accept", value: "application/json; charset=utf-8"})
}

req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.BaseURL, body)
if err != nil {
return fmt.Errorf("create request struct failed: %w", err)
}

for _, h := range headers {
req.Header.Set(h.key, h.value)
if req.Header.Get("Content-Type") == "" {
if multipartFilesGroupsContentType != "" {
req.Header.Set("Content-Type", multipartFilesGroupsContentType)
} else {
req.Header.Set("Content-Type", "application/json; charset=utf-8")
}
}

if req.Header.Get("Accept") == "" {
req.Header.Set("Accept", "application/json; charset=utf-8")
}

f := ChainInterceptor(append([]RequestInterceptor{c.RequestInterceptor}, interceptors...)...)
Expand Down
Loading