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 panic when both handler and middlewares are nil #2500

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ttrasn
Copy link

@ttrasn ttrasn commented Aug 7, 2023

No description provided.

@aldas
Copy link
Contributor

aldas commented Aug 7, 2023

could you explain why this case should act as no-op instead of panic? most of the cases I think providing nil for these value should be considered as something going very wrong.

@ttrasn
Copy link
Author

ttrasn commented Aug 7, 2023

Showing a 404 error is better than showing a panic error. I don't think it's possible for someone to intentionally not send the handler and only set the route.
An empty handler is like the absence of that page. So 404 is more suitable.

@ttrasn
Copy link
Author

ttrasn commented Sep 19, 2023

Hi @aldas, could you please check this PR?

@aldas
Copy link
Contributor

aldas commented Sep 19, 2023

I really do not want to accept this behavior.

Probably just doing this is better

	if handler == nil {
		panic("can not add route with nil handler")
	}

to fail fast and before even the server is started. Adding route without handler is something deeply wrong in developers code.

return true
}
}
return false
Copy link
Author

Choose a reason for hiding this comment

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

You mean when both of them were nil ?

Suggested change
return false
panic("can not add route with nil handler and middlewares")

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

2 participants