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

fix(tree): reassign fullpath when register new node which the same current node #2366

Merged
merged 4 commits into from May 11, 2020
Merged

fix(tree): reassign fullpath when register new node which the same current node #2366

merged 4 commits into from May 11, 2020

Conversation

@vinhha96
Copy link
Contributor

@vinhha96 vinhha96 commented May 9, 2020

Proposed changes

While using func context.FullPath(), I encountered an issue that it always returns previous registered parent path. It's fine when I change the registration order.

Example:

func (s *testSuite) registerTest() {
	s.router.GET("/user/:id/status", s.processWithStatus())
	s.router.GET("/user/:id", s.processWithID())
}

func (s *testSuite) processWithStatus() gin.HandlerFunc {
	return func(c *gin.Context) {
		logger.Infof("XXX - FullPath /user/:id/status ---> %s", c.FullPath())
		return
	}
}

func (s *testSuite) processWithID() gin.HandlerFunc {
	return func(c *gin.Context) {
		logger.Infof("XXX - FullPath /user/:id ---> %s", c.FullPath())
		return
	}
}

Result:

XXX - FullPath /user/:id/status ---> /service/api/v1/user/:id/status
XXX - FullPath /user/:id ---> /service/api/v1/user/:id/status

But, when I register with different the order of router, such as:

func (s *testSuite) registerTest() {
	s.router.GET("/user/:id", s.processWithID())
	s.router.GET("/user/:id/status", s.processWithStatus())
}

It's every thing so ok.

I have fix this bug and updated the test case.
Hope you spend your time review and accept my pull request with this issue.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)

Checklist

  • Lint and unit tests pass locally with my changes
@vinhha96 vinhha96 changed the title fix(tree): reassign fullpath when new node is the same current node fix(tree): reassign fullpath when register new node which the same current node May 9, 2020
@codecov
Copy link

@codecov codecov bot commented May 9, 2020

Codecov Report

Merging #2366 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2366   +/-   ##
=======================================
  Coverage   98.63%   98.63%           
=======================================
  Files          41       41           
  Lines        2349     2350    +1     
=======================================
+ Hits         2317     2318    +1     
  Misses         18       18           
  Partials       14       14           
Impacted Files Coverage Δ
tree.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d17270d...a43651d. Read the comment docs.

@vinhha96
Copy link
Contributor Author

@vinhha96 vinhha96 commented May 11, 2020

Hi @appleboy , @thinkerou. Please help me review this pull request.
Many thanks.

"/project/:name",
"/",
"/news/home",
"/news",
"/simple-two/one",
"/simple-two/one-two",
"/project/:name/build/*params",
"/project/:name/bui",
Copy link
Member

@appleboy appleboy May 11, 2020

put these changes back and add a new route as above.

Copy link
Contributor Author

@vinhha96 vinhha96 May 11, 2020

Done @appleboy. I added new route.

@appleboy appleboy added the bug label May 11, 2020
@appleboy appleboy added this to the 1.7 milestone May 11, 2020
@appleboy appleboy requested review from thinkerou and appleboy May 11, 2020
Copy link
Member

@thinkerou thinkerou left a comment

lgtm

@thinkerou thinkerou merged commit a6e8665 into gin-gonic:master May 11, 2020
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants