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

Text\plain content type for several responces with accept application\json header #375

Open
maks56893 opened this issue Jan 28, 2025 · 5 comments
Labels
bug Something isn't working

Comments

@maks56893
Copy link

To Reproduce
I'm testing pet store api from examples

  1. Did this request
curl --location 'http://localhost:9999/pets/' \
                            --header 'Accept: application/json' \
                            --header 'Content-Type: application/json' \
                            -v \
                            --data '{
                          "age": 2,
                          "is_adopted": true,
                          "name": "Napoleon",
                          "references": {
                            "type": "pet-123456",
                            "value": "string"
                          }
                        }'
  1. Got this responce
* Host localhost:9999 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:9999...
* connect to ::1 port 9999 from ::1 port 35584 failed: Connection refused
*   Trying 127.0.0.1:9999...
* Connected to localhost (127.0.0.1) port 9999
> POST /pets/ HTTP/1.1
> Host: localhost:9999
> User-Agent: curl/8.5.0
> Accept: application/json
> Content-Type: application/json
> Content-Length: 129
>
< HTTP/1.1 201 Created
< Server-Timing: controller;dur=0
< Trailer: Server-Timing
< X-Powered-By: Fuego
< X-Request-Id: 34ab508e-47bd-4a7d-b8b4-3ae012ea91d8
< Date: Tue, 28 Jan 2025 08:58:09 GMT
< Content-Type: text/plain; charset=utf-8
< Transfer-Encoding: chunked
<
{"id":"pet-2","name":"Napoleon","age":2,"is_adopted":false,"references":{"type":"","value":""}}
* Connection #0 to host localhost left intact

Expected behavior
I expect, that with header "Accept: application/json" response header "Content-Type" will be same as accept. But actually it's text/plain. Same with accept /, it return text/plain

Framework version (please check if it happens with the last
Fuego version before posting):
v0.17.0

Go version (please check if it happens with the last Go version before posting): 1.23.3

Additional context
I try do another request curl --location -v 'http://localhost:9999/pets/pet-1' and got response with content type application\json. According with doc default content type is html or json if no accept header or same as accept header. Did I miss something?

* Host localhost:9999 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:9999...
* connect to ::1 port 9999 from ::1 port 49152 failed: Connection refused
*   Trying 127.0.0.1:9999...
* Connected to localhost (127.0.0.1) port 9999
> GET /pets/pet-1 HTTP/1.1
> Host: localhost:9999
> User-Agent: curl/8.5.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Type: application/json
< Server-Timing: transformOut;dur=0;desc="transformOut"
< Trailer: Server-Timing
< X-Powered-By: Fuego
< X-Request-Id: ddbf72bb-2872-4f2b-a870-6acb9a845907
< Date: Tue, 28 Jan 2025 09:04:01 GMT
< Transfer-Encoding: chunked
<
{"id":"pet-1","name":"Napoleon","age":2,"is_adopted":false,"references":{"type":"","value":""}}
* Connection #0 to host localhost left intact
@maks56893 maks56893 added the bug Something isn't working label Jan 28, 2025
@EwenQuim
Copy link
Member

Investigating, thanks for the report!

@dylanhitt
Copy link
Collaborator

dylanhitt commented Jan 28, 2025

Oh interesting. I was able to pinpoint what appears to be causing this. Basically when ever the option of option.DefaultStatusCode(201), status code is set. I set it to 200 as well with the same result

@dylanhitt
Copy link
Collaborator

Yeah it's because where we're calling SetDefaultStatusCode under the hood we're calling WriteHeader and that's the response being read by curl.

@dylanhitt
Copy link
Collaborator

Looking through this the only thing we can really do here is attempt to infer the return type for the response before we call SetDefaultStatusCode. This seems to align with how echo and gin work as well.

@dylanhitt
Copy link
Collaborator

dylanhitt commented Jan 28, 2025

Okay a bit of a proposal ContextFlowable interface should be changed too:

type ContextFlowable[B any] interface {
	ContextWithBody[B]

	// Serialize serializes the given data to the response.
	Serialize(data any, code int) error
	// SerializeError serializes the given error to the response.
	SerializeError(err error)
}

Our SetDefaultStatusCode is calling WriteHeader under the hood and this currently true for both of our adaptors as well. What is happening is we're calling this SetDefaultStatusCode function before we're setting the Content-Type response header in Flow. This is really bad as it will also hide a 500 if the route errors later. This thread explains it pretty well https://stackoverflow.com/questions/52849382/how-do-i-set-http-status-code-conditionally-in-a-go-http-server.

A plus with the interface above. This allows is to move setting the default statuscode in Flow now as well. Meaning we do not have to perform this check in our adaptors anymore.

func (c echoContext[B]) Serialize(data any) error {
	status := c.echoCtx.Response().Status
	if status == 0 {
		status = c.DefaultStatusCode <--- we don't need to so this anymore
	}
	if status == 0 {
		status = http.StatusOK <---- we would still want to do this
	}
	c.echoCtx.JSON(status, data)
	return nil
}

On our server it's not possible to set the error code on the fly without it being an error, cause we don't actually provide a way setting an StatusCode much like gin/echo (they basically just set a Status value on their context object or like GinResponse and then wait until they serialize to call WriteHeader.

I'll be putting up a PR showing the change soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants