Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

improved task middleware to show skipped path #1380

Merged
merged 4 commits into from
Oct 18, 2018

Conversation

sio4
Copy link
Member

@sio4 sio4 commented Oct 17, 2018

Hi, as described as issue #1379, command buffalo task middleware does just show middleware stacks per App registered with App.Group() or App.Resource() and we cannot find which middleware was skipped (by App.Middleware.Skip()) in specific paths.

By following routes and check related app, this commit allows user can find which is real middleware stack for each path. Example output is like this:

$ buffalo t middleware
-> /
   github.com/gobuffalo/buffalo.*App.defaultErrorMiddleware
   github.com/gobuffalo/buffalo.*App.PanicHandler
   github.com/gobuffalo/buffalo.RequestLoggerFunc
   github.com/gobuffalo/buffalo.sessionSaver
   github.com/gobuffalo/mw-forcessl.Middleware.func1
   github.com/gobuffalo/mw-paramlogger.ParameterLogger
   github.com/gobuffalo/mw-csrf.glob..func1
   github.com/gobuffalo/buffalo-pop/pop/popmw.Transaction.func2
   github.com/gobuffalo/mw-i18n.*Translator.Middleware.func1
   github.com/hyeoncheon/skel/actions.AuthorizeHandler
-> GET / (by actions.HomeHandler)
   github.com/gobuffalo/buffalo.*App.defaultErrorMiddleware
   github.com/gobuffalo/buffalo.*App.PanicHandler
   github.com/gobuffalo/buffalo.RequestLoggerFunc
   github.com/gobuffalo/buffalo.sessionSaver
   github.com/gobuffalo/mw-forcessl.Middleware.func1
   github.com/gobuffalo/mw-paramlogger.ParameterLogger
   github.com/gobuffalo/mw-csrf.glob..func1
   github.com/gobuffalo/buffalo-pop/pop/popmw.Transaction.func2
   github.com/gobuffalo/mw-i18n.*Translator.Middleware.func1
-> /api/v1
   github.com/gobuffalo/mw-csrf.glob..func1
-> GET /api/v1/ (by actions.HomeHandler)
   [none]
-> /api/v1/app/users has same middleware stack as /api/v1
-> /api/v1/users has same middleware stack as /api/v1
-> /auth
   github.com/gobuffalo/buffalo.*App.defaultErrorMiddleware
   github.com/gobuffalo/buffalo.*App.PanicHandler
   github.com/gobuffalo/buffalo.RequestLoggerFunc
   github.com/gobuffalo/buffalo.sessionSaver
   github.com/gobuffalo/mw-forcessl.Middleware.func1
   github.com/gobuffalo/mw-paramlogger.ParameterLogger
   github.com/gobuffalo/mw-csrf.glob..func1
   github.com/gobuffalo/buffalo-pop/pop/popmw.Transaction.func2
   github.com/gobuffalo/mw-i18n.*Translator.Middleware.func1
-> /users has same middleware stack as /
$ 

For handler which has skipped middleware, it shows like this:

-> GET / (by actions.HomeHandler)
<...>

Stack output format is same as normal App but the title line contains method and handler name.

When there is no middleware for some handler or App, by Clear(), it shows "[none]".

-> GET /api/v1/ (by actions.HomeHandler)
   [none]

In case of the App that have same middleware stack as its parent, it shows like this:

-> /api/v1/app/users has same middleware stack as /api/v1

Please check this patch.

@markbates markbates requested a review from a team October 17, 2018 11:35
@markbates markbates added this to the v0.13.1 milestone Oct 17, 2018
Copy link
Member

@markbates markbates left a comment

Choose a reason for hiding this comment

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

Love this! Just a few cosmetic suggestions

grifts.go Outdated
}
s = strings.Replace(s, "\n", "\n ", -1)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
s = strings.Replace(s, "\n", "\n ", -1)
s = strings.Replace(s, "\n", "\n\t", -1)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I agree with using tab even though it takes five more spaces and can generate line wrap on 80 col terminal like me, anyway that is fancier!

grifts.go Outdated
}
s = strings.Replace(s, "\n", "\n ", -1)
fmt.Printf(" %v\n", strings.TrimSpace(s))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fmt.Printf(" %v\n", strings.TrimSpace(s))
fmt.Printf("\t%v\n", strings.TrimSpace(s))

grifts.go Outdated
fmt.Printf("-> %s\n", r.App.Name)
printMiddlewareStackWithIndent(mws[r.App.Name])
} else {
fmt.Printf("-> %s has same middleware stack as %v\n", r.App.Name, pname)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fmt.Printf("-> %s has same middleware stack as %v\n", r.App.Name, pname)
fmt.Printf("-> %s (see: %v)\n", r.App.Name, pname)

@sio4
Copy link
Member Author

sio4 commented Oct 17, 2018

Anyway, I found a bug, in case of the handler Skip()ed more than two middlewares! At the first time, I think about it but I missed while I focused on how to find the parent of the App since it only has root.

I will fix it as soon as possible, so please wait an hour! :-)

@markbates
Copy link
Member

Awesome. Let me know when it’s ready to go.

@sio4
Copy link
Member Author

sio4 commented Oct 17, 2018

OK, two commits for suggestion and bugfix are pushed and checked for CIs.
Please check this change. Now it just shows us single correct middleware stack for skipped, skipped handlers.

@sio4
Copy link
Member Author

sio4 commented Oct 18, 2018

Hi @markbates,
Did you check updated commits? Just as reminder.

@markbates markbates merged commit 1cecfc0 into gobuffalo:development Oct 18, 2018
@markbates
Copy link
Member

So good! Thank you!

@sio4
Copy link
Member Author

sio4 commented Oct 18, 2018

You're welcome!
Thanks for great framework everyday!

markbates pushed a commit that referenced this pull request Nov 2, 2018
* improved task middleware to show skipped path

* use tab instead of spaces for indenting middlewares

* fixed duplicated and invalid output for two or more skipping
@sio4 sio4 deleted the show-middleware-stack branch November 7, 2021 05:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants