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

Poc router stack backtracking #1791

Merged
merged 6 commits into from
Mar 2, 2021

Conversation

aldas
Copy link
Contributor

@aldas aldas commented Feb 24, 2021

Creating new PR for #1770 as @stffabi seems to be currently missing in action and I can not push to (edit) his PR ('was rejected by remote').

I would like to add PR after this is merged with:

  • renaming short variables in router methods (better readability)
  • refactor tests to table based (easier to debug with breakpoints)

@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #1791 (db75ddc) into master (932976d) will decrease coverage by 0.20%.
The diff coverage is 91.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1791      +/-   ##
==========================================
- Coverage   89.74%   89.54%   -0.21%     
==========================================
  Files          32       32              
  Lines        2672     2640      -32     
==========================================
- Hits         2398     2364      -34     
- Misses        175      178       +3     
+ Partials       99       98       -1     
Impacted Files Coverage Δ
router.go 95.88% <91.48%> (-1.18%) ⬇️
echo.go 91.53% <0.00%> (ø)
binder.go 100.00% <0.00%> (ø)
middleware/rate_limiter.go 100.00% <0.00%> (ø)
middleware/slash.go 91.30% <0.00%> (+0.60%) ⬆️

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 932976d...db75ddc. Read the comment docs.

@aldas aldas requested a review from lammel February 24, 2021 21:48
@aldas aldas force-pushed the poc-router-stack-backtracking branch from 8b07552 to 3093bab Compare February 24, 2021 23:58
@aldas
Copy link
Contributor Author

aldas commented Feb 26, 2021

@lammel could you review this one when you have time (poking with comment to trigger notification email)

@lammel
Copy link
Contributor

lammel commented Feb 26, 2021

Sorry, forgot to respond. I'm busy this week.
Looks good to me overall. Wanted to take a tour and do a little deep dive and debug session with it before approving actually.

Tests are added and green, so we are not far from merging.

router.go Outdated
for {
if search == "" {
break
// backtracking happens when we reach dead end when matching nodes in the router tree. To backtrack we set
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion for comment. Not sure I got it right, take what makes sense to you:

  // Backtracking is needed when a dead end (leaf node) is reached in the router tree.
	// To backtrack the current node will be changed to the parent node and the next kind for the
	// router logic will be returned based on fromKind or kind of the dead end node (static > param > any).
	// For example if there is no static node match we should check parent next sibling by kind (param). 
	// Backtracking itself does not check if there is a next sibling, this is done by the router logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

router.go Outdated
cn = previous.parent
valid = cn != nil

// next node type by priority
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

    // Next node type by priority
		// NOTE: With the current implementation we never backtrack from an `any` route, so `previous.kind` is 
		// always `static` or `any`
		// If this is changed then for any route next kind would be `static` and this statement should be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

router.go Show resolved Hide resolved
Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

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

I think we should merge to have a clean base for further router improvements.
Approved!

@aldas aldas merged commit 6f9b71c into labstack:master Mar 2, 2021
@lammel lammel added this to the v4.2.1 milestone Mar 3, 2021
This was referenced Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants