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

feat: optimize code and bug fix #13

Merged
merged 14 commits into from
Aug 28, 2023
Merged

feat: optimize code and bug fix #13

merged 14 commits into from
Aug 28, 2023

Conversation

devhaozi
Copy link
Member

Closes goravel/goravel#237

📑 Description

  1. 同步来自gin的有关修改
  2. 特殊处理fiber中间件只能全局生效的问题,🐛 Not Found Handler wih subgroup middleware gofiber/fiber#1959
  3. 补全缺失的单元测试
  4. 修复QueryArrayQueryMap函数的问题

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

ℹ Additional Information

@devhaozi devhaozi requested review from hwbrzzl and kkumar-gcc August 20, 2023 18:06
@devhaozi devhaozi self-assigned this Aug 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2023

Codecov Report

Patch coverage: 73.16% and project coverage change: -2.56% ⚠️

Comparison is base (64c2500) 78.65% compared to head (81f9039) 76.10%.
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
- Coverage   78.65%   76.10%   -2.56%     
==========================================
  Files           7        8       +1     
  Lines         567      724     +157     
==========================================
+ Hits          446      551     +105     
- Misses        103      144      +41     
- Partials       18       29      +11     
Files Changed Coverage Δ
context.go 71.87% <ø> (ø)
service_provider.go 0.00% <0.00%> (ø)
response.go 71.05% <33.33%> (-0.95%) ⬇️
request.go 67.01% <60.27%> (-3.43%) ⬇️
route.go 87.25% <72.41%> (ø)
cors.go 80.48% <80.48%> (ø)
group.go 100.00% <100.00%> (+3.57%) ⬆️
utils.go 92.85% <100.00%> (+0.54%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

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

A perfect PR!

cors.go Outdated Show resolved Hide resolved
cors_test.go Outdated Show resolved Hide resolved
cors_test.go Outdated Show resolved Hide resolved
group.go Outdated Show resolved Hide resolved
group.go Outdated Show resolved Hide resolved
route.go Outdated Show resolved Hide resolved
route_test.go Show resolved Hide resolved
route_test.go Show resolved Hide resolved
tls.go Outdated Show resolved Hide resolved
utils.go Outdated Show resolved Hide resolved
@devhaozi devhaozi requested a review from hwbrzzl August 21, 2023 10:11
Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

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

The print of fiber is a bit strange, PTAL

image

@devhaozi
Copy link
Member Author

The print of fiber is a bit strange, PTAL

image

I can't make fiber work well, the method is such a shit.

@devhaozi
Copy link
Member Author

devhaozi commented Aug 21, 2023

The print of fiber is a bit strange, PTAL
image

I can't make fiber work well, the method is such a shit.

Fiber's middleware method Use is very unique. It can only register global middleware or prefixed middleware, and it can only be registered once, otherwise it will be repeated.

Fiber's Group method also very unique, it require call like:

api := app.Group("/api", middleware) // /api

v1 := api.Group("/v1", middleware)   // /api/v1
v1.Get("/list", handler)             // /api/v1/list
v1.Get("/user", handler)             // /api/v1/user

v2 := api.Group("/v2", middleware)   // /api/v2
v2.Get("/list", handler)             // /api/v2/list
v2.Get("/user", handler)             // /api/v2/user

not support:

app.Group("/v1", middleware).Get("/list", handler)             // /v1/list
app.Group("/v1", middleware).Get("/user", handler)             // /v1/user

app.Group("/v2", hande).Get("/list", handler)                  // /v2/list
app.Group("/v2", middleware).Get("/user", handler)             // /v2/user

So bad, need many changes to do....

@devhaozi
Copy link
Member Author

image
This screenshot shows the middleware being registered repeatedly because the Use method is called repeatedly.
I don't have any solution to this problem...

@devhaozi
Copy link
Member Author

The print of fiber is a bit strange, PTAL

image

This is because cors middleware add to /.
image

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Aug 22, 2023

Let me make a test, too.

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Aug 22, 2023

The print of fiber is a bit strange, PTAL
image

I can't make fiber work well, the method is such a shit.

Fiber's middleware method Use is very unique. It can only register global middleware or prefixed middleware, and it can only be registered once, otherwise it will be repeated.

Fiber's Group method also very unique, it require call like:

api := app.Group("/api", middleware) // /api

v1 := api.Group("/v1", middleware)   // /api/v1
v1.Get("/list", handler)             // /api/v1/list
v1.Get("/user", handler)             // /api/v1/user

v2 := api.Group("/v2", middleware)   // /api/v2
v2.Get("/list", handler)             // /api/v2/list
v2.Get("/user", handler)             // /api/v2/user

not support:

app.Group("/v1", middleware).Get("/list", handler)             // /v1/list
app.Group("/v1", middleware).Get("/user", handler)             // /v1/user

app.Group("/v2", hande).Get("/list", handler)                  // /v2/list
app.Group("/v2", middleware).Get("/user", handler)             // /v2/user

So bad, need many changes to do....

It seems to support?

image

@devhaozi
Copy link
Member Author

devhaozi commented Aug 22, 2023

The print of fiber is a bit strange, PTAL
image

I can't make fiber work well, the method is such a shit.

Fiber's middleware method Use is very unique. It can only register global middleware or prefixed middleware, and it can only be registered once, otherwise it will be repeated.
Fiber's Group method also very unique, it require call like:

api := app.Group("/api", middleware) // /api

v1 := api.Group("/v1", middleware)   // /api/v1
v1.Get("/list", handler)             // /api/v1/list
v1.Get("/user", handler)             // /api/v1/user

v2 := api.Group("/v2", middleware)   // /api/v2
v2.Get("/list", handler)             // /api/v2/list
v2.Get("/user", handler)             // /api/v2/user

not support:

app.Group("/v1", middleware).Get("/list", handler)             // /v1/list
app.Group("/v1", middleware).Get("/user", handler)             // /v1/user

app.Group("/v2", hande).Get("/list", handler)                  // /v2/list
app.Group("/v2", middleware).Get("/user", handler)             // /v2/user

So bad, need many changes to do....

It seems to support?

image

But yesterday, in the my project test, I found that the route was registered repeatedly and make middlewares invalidated. After remove Group call, the middleware work again(But middleware still call registered repeated).

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Aug 22, 2023

I checked the logic of fiber, and found the repetition is expected, I think just filtering them is ok.

	if item.Path == "/" {
		pass := false
		for _, handler := range item.Handlers {
			if strings.Contains(runtime.FuncForPC(reflect.ValueOf(handler).Pointer()).Name(), "recover") {
				pass = true
				break
			}
		}
		if pass {
			continue
		}
	}
image image

@devhaozi
Copy link
Member Author

devhaozi commented Aug 22, 2023

I checked the logic of fiber, and found the repetition is expected, I think just filtering them is ok.

	if item.Path == "/" {
		pass := false
		for _, handler := range item.Handlers {
			if strings.Contains(runtime.FuncForPC(reflect.ValueOf(handler).Pointer()).Name(), "recover") {
				pass = true
				break
			}
		}
		if pass {
			continue
		}
	}

image image

How about middleware repeat?

Fiber doesn't support register middleware for a route.

@devhaozi
Copy link
Member Author

image

The Use method also seems to be related to the routes order. This paragraph in the document shows that if you add a Use at the end, it is equivalent to Gin’s Fallback method.

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Aug 22, 2023

I think we can register a middleware for a route like below(don't use the Use method):

image

@devhaozi
Copy link
Member Author

I think we can register a middleware for a route like below(don't use the Use method):

image

image
middleware added, but not use.

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Aug 22, 2023

I'll make a deeper test.

@hwbrzzl hwbrzzl merged commit 4b70326 into master Aug 28, 2023
7 checks passed
@hwbrzzl hwbrzzl deleted the haozi/optimize branch August 28, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 [Bug] Some issues for goravel/fiber
3 participants