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

Add PathStripRegex rule #1339

Merged
merged 1 commit into from
Apr 28, 2017
Merged

Conversation

seguins
Copy link
Contributor

@seguins seguins commented Mar 24, 2017

Fixes #928

@emilevauge
Copy link
Member

@seguins thanks for your contribution. I think we should avoid adding a dependency to github.com/containous/mux in middlewares/. Do you think you could add some interface tricks into this?

@seguins
Copy link
Contributor Author

seguins commented Apr 7, 2017

I think we can create an interface for the matching, and move the github.com/containous/mux part to the main package:

type PrefixMatcher interface {
	Match(r *http.Request) (bool, string)
}

The stripPrefix file cloud be:

package middlewares

import (
	"net/http"
)

type PrefixMatcher interface {
	Match(r *http.Request) (bool, string)
}

// StripPrefix is a middleware used to strip prefix from an URL request
type StripPrefix struct {
	Handler http.Handler
	matcher PrefixMatcher
}

// NewStripPrefix builds a new StripPrefix given a handler and prefixes
func NewStripPrefix(handler http.Handler, matcher PrefixMatcher) *StripPrefix {
	stripPrefix := StripPrefix{Handler: handler, matcher: matcher}
	return &stripPrefix
}

func (s *StripPrefix) ServeHTTP(w http.ResponseWriter, r *http.Request) {
	if match, prefix := s.matcher.Match(r); match == true {
		r.URL.Path = r.URL.Path[len(prefix):]
		r.RequestURI = r.URL.RequestURI()
		s.Handler.ServeHTTP(w, r)
		return
	}
	http.NotFound(w, r)
}

// SetHandler sets handler
func (s *StripPrefix) SetHandler(Handler http.Handler) {
	s.Handler = Handler
}

For you information, I have checked other files in middlewares, and I found that handlerSwitcher.go and routes.go already use github.com/containous/mux.

@emilevauge do you think we should do this change ?

@emilevauge
Copy link
Member

@seguins
You right, let's keep the dep to github.com/containous/mux.
Would you consider creating a PathStripRegex instead of reusing PathStrip ? I fear that adding regex check in PathStrip will slow down the process for all other requests.

@seguins
Copy link
Contributor Author

seguins commented Apr 28, 2017

@emilevauge, I've pushed a version with the regex matching separated.

@ldez
Copy link
Contributor

ldez commented Apr 28, 2017

Great job @seguins! Could you fix this https://travis-ci.org/containous/traefik/jobs/226814163#L827 ? Thanks.

@ldez ldez added this to the 1.3 milestone Apr 28, 2017
@seguins
Copy link
Contributor Author

seguins commented Apr 28, 2017

Fixed 😃

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

Thanks @seguins, awesome job :)
LGTM

@ldez ldez merged commit 441d544 into traefik:master Apr 28, 2017
@ldez ldez added kind/bug/fix a bug fix and removed status/3-needs-merge kind/enhancement a new or improved feature. labels May 19, 2017
@ldez ldez changed the title Fix regex with PathStrip Add PathStripRegex rule May 25, 2017
@ldez ldez added kind/enhancement a new or improved feature. area/rules and removed kind/bug/fix a bug fix labels May 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rules kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants