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

404 Error instead of 405 Status Code #60

Closed
harshvladha opened this issue Apr 2, 2017 · 8 comments
Closed

404 Error instead of 405 Status Code #60

harshvladha opened this issue Apr 2, 2017 · 8 comments

Comments

@harshvladha
Copy link
Contributor

mux := bone.New()
mux.Get("/test/", http.HandlerFunc(myHandler))

If I make requests to /test/ endpoint with POST method then I get 404 Not Found error instead of 405 Method Not Allowed.

Probable reason:
map[string][]*Route the key in this map is method name (e.g., GET, POST, etc..) and the requested endpoint is searched in the slice of the requested method, and if the endpoint is not found then 404 is returned.

Proposed Solution:

  1. Search all Method's slices. (too costly operation)
    OR
  2. Register routes with the different way.
@harshvladha
Copy link
Contributor Author

Same as #37

@harshvladha
Copy link
Contributor Author

I could have sent a PR about this, but it involves major changes, that's why I want to discuss the same.

@squiidz
Copy link
Member

squiidz commented Apr 2, 2017

Hi, i have no problems with changes if it's not broking the public api.

@harshvladha
Copy link
Contributor Author

harshvladha commented Apr 3, 2017

Working on it! 👍

@squiidz
Copy link
Member

squiidz commented Apr 3, 2017

great :)

@harshvladha
Copy link
Contributor Author

I was thinking to change current Mux struct to

type Mux struct {
	Routes        []*Route //changed
	prefix        string
	notFound      http.Handler
	Serve         func(rw http.ResponseWriter, req *http.Request)
	CaseSensitive bool
}

and Route will have information of allowed methods and their handlers, something like:

type Route struct {
	Path           string
	methods        int //changed
	Size           int
	Atts           int
	wildPos        int
	Token          Token
	Pattern        map[int]string
	Compile        map[int]*regexp.Regexp
	Tag            map[int]string
	MethodHandlers map[string]*http.Handler //added
}

Route.methods will have ORed int values of Methods, as done for Route.Atts: PARAM, SUB, WC, REGEX.

func (r *Route) setMethod(m string, h http.Handler) {
	r.methods |= methodValues[m]
	r.MethodHandlers[m] = &h
}

will set the Method for the given route.

func (r *Route) handler(m string) (*http.Handler) {
	if methodValues[m]&r.methods != 0 {
		return r.MethodHandlers[m]
	}
	return nil
}

But in this case, I will need to iterate through all the value in Mux.Route slice and try to parse it, which is equivalent to iterating all the slices for respective methods in the current implementation.

So I would like to go with solution 1, proposed in the 1st comment of this thread.

@squiidz
Copy link
Member

squiidz commented Apr 4, 2017

Iterating over all routes is really costly in term of performance... Maybe you could try to make a fail over, only when a route is not found, you try to found a matching one by url instead ?

@harshvladha
Copy link
Contributor Author

harshvladha commented Apr 5, 2017

@squiidz Coded as per your last comment. Commit id: 0cef4cb
Initiated a PullRequest #61

PS: Ignore commit a9baaab as this was not go-Fmt'ed

@squiidz squiidz closed this as completed in 0cef4cb Apr 5, 2017
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

No branches or pull requests

2 participants