refactor: move accept header from mux to server options #324

ekumamatthew · 2024-12-30T05:11:19Z

refactor: move accept header from mux to server options
fixes #266

EwenQuim

Thank you for this PR! 🙏🏼

server.go

EwenQuim · 2025-01-09T14:18:57Z

Fixes #266

dylanhitt · 2025-01-10T17:00:08Z

server_test.go

@@ -402,10 +402,10 @@ func TestGroupParams(t *testing.T) {
 	t.Log(document.Paths.Find("/").Get.Parameters[0].Value.Name)
 	require.Len(t, document.Paths.Find("/").Get.Parameters, 1)
 	require.Equal(t, document.Paths.Find("/").Get.Parameters[0].Value.Name, "Accept")


Not your change, but we may want to check if require.NotNil(t, document.Paths.Find("/").Get.Parameters[0].Value)

Also could probably change all the require.Equal below to assert.

This is just a nitpick feel free to resolve

EwenQuim · 2025-01-12T13:22:34Z

Please rebase your branch on main, it is quite outdated :)

Thanks for the PR!

EwenQuim · 2025-01-13T10:26:26Z

Please fix the tests.

I think you can run make golden-update or something like that to update the golden file. But it's weird, it should not change as it is just an internal refactoring

…cross all api

…ccross all api

…accross all api

ccoVeille · 2025-01-14T19:18:00Z

params_test.go

+		route := fuego.Get(s, "/test", func(c fuego.ContextNoBody) (string, error) {
+			name := c.QueryParam("name")
+			age := c.QueryParamInt("age")
+			isok := c.QueryParamBool("is_ok")


nitpicking: I would like to suggest

Suggested change

isok := c.QueryParamBool("is_ok")

isOK := c.QueryParamBool("is_ok")

ccoVeille · 2025-01-14T19:32:18Z

params_test.go

+		require.Equal(t, "Age", route.Params["age"].Description)
+		require.True(t, route.Params["age"].Nullable)
+		require.Equal(t, 18, route.Params["age"].Default)
+		require.Equal(t, "integer", route.Params["age"].GoType)


Suggested change

require.Equal(t, "Age", route.Params["age"].Description)

require.True(t, route.Params["age"].Nullable)

require.Equal(t, 18, route.Params["age"].Default)

require.Equal(t, "integer", route.Params["age"].GoType)

require.Contains(t, route.Params, "age")

require.Equal(t, "Age", route.Params["age"].Description)

require.True(t, route.Params["age"].Nullable)

require.Equal(t, 18, route.Params["age"].Default)

require.Equal(t, "integer", route.Params["age"].GoType)

Or, maybe you could do this, the readability will be increased

Suggested change

require.Equal(t, "Age", route.Params["age"].Description)

require.True(t, route.Params["age"].Nullable)

require.Equal(t, 18, route.Params["age"].Default)

require.Equal(t, "integer", route.Params["age"].GoType)

require.Contains(t, route.Params, "age")

paramAge := route.Params["age"]

require.Equal(t, "Age", paramAge.Description)

require.True(t, paramAge.Nullable)

require.Equal(t, 18, paramAge.Default)

require.Equal(t, "integer", paramAge.GoType)

ccoVeille · 2025-01-14T19:36:10Z

params_test.go

+		require.Equal(t, "Name", route.Params["name"].Description)
+		require.True(t, route.Params["name"].Required)
+		require.Equal(t, "hey", route.Params["name"].Default)
+		require.Equal(t, "you", route.Params["name"].Examples["example1"])
+		require.Equal(t, "string", route.Params["name"].GoType)


This would be safer if something is refactored, it won't panic

Suggested change

require.Equal(t, "Name", route.Params["name"].Description)

require.True(t, route.Params["name"].Required)

require.Equal(t, "hey", route.Params["name"].Default)

require.Equal(t, "you", route.Params["name"].Examples["example1"])

require.Equal(t, "string", route.Params["name"].GoType)

require.Contains(t, route.Params, name)

require.Equal(t, "Name", route.Params["name"].Description)

require.True(t, route.Params["name"].Required)

require.Equal(t, "hey", route.Params["name"].Default)

require.Contains(t, route.Params["name"].Examples, "example1")

require.Equal(t, "you", route.Params["name"].Examples["example1"])

require.Equal(t, "string", route.Params["name"].GoType)

Also this would be more readable

Suggested change

require.Equal(t, "Name", route.Params["name"].Description)

require.True(t, route.Params["name"].Required)

require.Equal(t, "hey", route.Params["name"].Default)

require.Equal(t, "you", route.Params["name"].Examples["example1"])

require.Equal(t, "string", route.Params["name"].GoType)

require.Contains(t, route.Params, name)

paramName := route.Params["name"]

require.Equal(t, "Name", paramName.Description)

require.True(t, paramName.Required)

require.Equal(t, "hey", paramName.Default)

require.Contains(t, paramName.Examples, "example1")

require.Equal(t, "you", paramName.Examples["example1"])

require.Equal(t, "string", paramName.GoType)

ccoVeille · 2025-01-14T19:45:07Z

server_test.go

-	require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "X-Test-Header")
-	require.Equal(t, document.Paths.Find("/api/test2").Get.Parameters[1].Value.Name, "Accept")
-	require.Equal(t, document.Paths.Find("/api/test2").Get.Parameters[0].Value.Name, "X-Test-Header")
+	require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "Accept")


Not related to this PR, but something like this should be added to avoid panics if there is a refactoring

Suggested change

require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "Accept")

require.Len(t, document.Paths.Find("/api/test").Get.Parameters, 2)

require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "Accept")

ccoVeille · 2025-01-14T19:46:01Z

server_test.go

@@ -376,8 +376,8 @@ func TestGroupHeaderParams(t *testing.T) {
 	require.Equal(t, "test-value", route.Operation.Parameters.GetByInAndName("header", "X-Test-Header").Description)

 	document := s.OutputOpenAPISpec()
-	require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[1].Value.Name, "Accept")
-	require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "X-Test-Header")
+	require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "Accept")


Same about testing Get.Parameters with require.Len

And in the next tests you are updating

ekumamatthew · 2025-01-14T20:51:09Z

Please fix the tests.

I think you can run make golden-update or something like that to update the golden file. But it's weird, it should not change as it is just an internal refactoring

dylanhitt · 2025-01-14T22:02:08Z

#324 (comment)

Is that updating that file for you?

dylanhitt · 2025-01-14T22:04:58Z

Damn, it isn't. Hmm something is going on here.

dylanhitt · 2025-01-14T22:05:47Z

Oh nevermind the test s are failing cause their are two functions named TestParams

dylanhitt · 2025-01-14T22:06:53Z

Also the change to golden is the ordering so that should be fine.

ccoVeille · 2025-01-19T15:57:44Z

Not, that's a major issue, but I made 5+ feedbacks, and none were addressed, or at least commented.

I would appreciate if they could be reviewed.

Thanks

ekumamatthew · 2025-01-19T16:18:08Z

i just sync my brnch. i'll be updating my PR shortly

EwenQuim · 2025-01-20T09:01:00Z

server.go

 			OptionAddResponse(http.StatusInternalServerError, "Internal Server Error _(panics)_", Response{Type: HTTPError{}}),
+			OptionHeader("Accept", "Accept header parameter"),
 		),
 	}


Here you're adding the parameter to all routes.

Fuego can also register standard func(w http.ResponseWriter, r *http.Request) routes that don't need this option.

I suggest that you move this to the registerFuegoController() function in net_http_mux.go file

EwenQuim requested changes Dec 30, 2024

View reviewed changes

server.go Outdated Show resolved Hide resolved

dylanhitt reviewed Jan 10, 2025

View reviewed changes

ekumamatthew and others added 9 commits January 14, 2025 12:55

refactor: move accept header form mux to server options

b9ea0f3

refactor: move Accept header config from mux to server options

32e2752

Update openapi.golden.json

d02bf5c

Update net_http_mux.go, removed unused imports

15ecaaf

Update option_test.go, fix test to account for added Accept header ac…

cd42be9

…cross all api

Update param_test.go, fixed test to account for added Accept header a…

80213f7

…ccross all api

Update server.go, use optionHeader

4347487

Update server_test.go, fixed test to account for added Accept header …

6961ce3

…accross all api

Update params_test.go, fixed test to account for added Accept header …

ea60340

…accross all api

dylanhitt force-pushed the feat/#issue266 branch from 900e219 to ea60340 Compare January 14, 2025 17:55

ccoVeille changed the title ~~refactor: move accept header form mux to server options~~ refactor: move accept header from mux to server options Jan 14, 2025

ccoVeille reviewed Jan 14, 2025

View reviewed changes

feat/ add make golden-update

b6498a2

Merge branch 'go-fuego:main' into feat/#issue266

da2da97

EwenQuim requested changes Jan 20, 2025

View reviewed changes

EwenQuim added this to the v0.18 milestone Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move accept header from mux to server options #324

refactor: move accept header from mux to server options #324

ekumamatthew commented Dec 30, 2024 •

edited by ccoVeille

Loading

EwenQuim left a comment

EwenQuim commented Jan 9, 2025

dylanhitt Jan 10, 2025

dylanhitt Jan 10, 2025

EwenQuim commented Jan 12, 2025

EwenQuim commented Jan 13, 2025

ccoVeille Jan 14, 2025

ccoVeille Jan 14, 2025

ccoVeille Jan 14, 2025

ccoVeille Jan 14, 2025

ccoVeille Jan 14, 2025

ccoVeille Jan 14, 2025

ekumamatthew commented Jan 14, 2025 •

edited

Loading

dylanhitt commented Jan 14, 2025

dylanhitt commented Jan 14, 2025

dylanhitt commented Jan 14, 2025

dylanhitt commented Jan 14, 2025

ccoVeille commented Jan 19, 2025

ekumamatthew commented Jan 19, 2025

EwenQuim Jan 20, 2025

	isok := c.QueryParamBool("is_ok")
	isOK := c.QueryParamBool("is_ok")

-		require.Equal(t, "Age", route.Params["age"].Description)
-		require.True(t, route.Params["age"].Nullable)
-		require.Equal(t, 18, route.Params["age"].Default)
-		require.Equal(t, "integer", route.Params["age"].GoType)
+		require.Contains(t, route.Params, "age")
+		paramAge := route.Params["age"]
+		require.Equal(t, "Age", paramAge.Description)
+		require.True(t, paramAge.Nullable)
+		require.Equal(t, 18, paramAge.Default)
+		require.Equal(t, "integer", paramAge.GoType)

-		require.Equal(t, "Name", route.Params["name"].Description)
-		require.True(t, route.Params["name"].Required)
-		require.Equal(t, "hey", route.Params["name"].Default)
-		require.Equal(t, "you", route.Params["name"].Examples["example1"])
-		require.Equal(t, "string", route.Params["name"].GoType)
+		require.Contains(t, route.Params, name)
+		paramName := route.Params["name"]
+		require.Equal(t, "Name", paramName.Description)
+		require.True(t, paramName.Required)
+		require.Equal(t, "hey", paramName.Default)
+		require.Contains(t, paramName.Examples, "example1")
+		require.Equal(t, "you", paramName.Examples["example1"])
+		require.Equal(t, "string", paramName.GoType)

	require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "Accept")
	require.Len(t, document.Paths.Find("/api/test").Get.Parameters, 2)
	require.Equal(t, document.Paths.Find("/api/test").Get.Parameters[0].Value.Name, "Accept")

refactor: move accept header from mux to server options #324

Are you sure you want to change the base?

refactor: move accept header from mux to server options #324

Conversation

ekumamatthew commented Dec 30, 2024 • edited by ccoVeille Loading

EwenQuim left a comment

Choose a reason for hiding this comment

EwenQuim commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EwenQuim commented Jan 12, 2025

EwenQuim commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekumamatthew commented Jan 14, 2025 • edited Loading

dylanhitt commented Jan 14, 2025

dylanhitt commented Jan 14, 2025

dylanhitt commented Jan 14, 2025

dylanhitt commented Jan 14, 2025

ccoVeille commented Jan 19, 2025

ekumamatthew commented Jan 19, 2025

Choose a reason for hiding this comment

ekumamatthew commented Dec 30, 2024 •

edited by ccoVeille

Loading

ekumamatthew commented Jan 14, 2025 •

edited

Loading