Skip to content
This repository has been archived by the owner on May 23, 2022. It is now read-only.

Conversation

KevSlashNull
Copy link
Contributor

If a request is made to an URI which doesn’t resolve to a route,
Confetti will currently not execute a middleware. This makes it
impossible to add behavior for handling these soft errors gracefully or
log the request.

Therefore, this makes application middlewares run on routing errors by
storing the middlewares on the RouteCollection and setting them when
creating an error route.

@codecov
Copy link

codecov bot commented Feb 13, 2022

Codecov Report

Merging #27 (284d3a2) into main (dddbef0) will increase coverage by 0.18%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
+ Coverage   76.65%   76.84%   +0.18%     
==========================================
  Files          83       83              
  Lines        2039     2047       +8     
==========================================
+ Hits         1563     1573      +10     
+ Misses        417      415       -2     
  Partials       59       59              
Impacted Files Coverage Δ
http/routing/route_collection.go 93.16% <66.66%> (+0.17%) ⬆️
console/route_list.go 94.44% <0.00%> (+0.89%) ⬆️
http/routing/methods.go 79.31% <0.00%> (+6.89%) ⬆️

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 dddbef0...284d3a2. Read the comment docs.

@reindert-vetter
Copy link
Contributor

Thank you! Looks like a nice solution. I will test and play with it later today.

@@ -143,6 +144,8 @@ func (c *RouteCollection) Middleware(middleware ...inter.HttpMiddleware) inter.R
route.SetMiddleware(middleware)
}

c.globalMiddlewares = append(c.globalMiddlewares, middleware...)
Copy link
Contributor

@reindert-vetter reindert-vetter Feb 14, 2022

Choose a reason for hiding this comment

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

The only problem is that the globalMiddlewares is also filled in groups from other routes, but is not used there. But maybe not so bad.

We can also make use of Fallback Routes. We put it on the api route group and on the web route group (in api.go or in route_service_provider.go). Then it is clear which middlewares are executed and which are not. In addition, you have more control over the difference between web and api. How do you think of that?

https://confetti-framework.github.io/docs/the-basics/routing.html#fallback-routes

Example:

var Api = Group(
	Get("/ping", controllers.Ping).Name("ping"),
	Fallback(func(request inter.Request) inter.Response {
		return outcome.Json(map[string]string{"message": "not found"}).Status(net.StatusNotFound)
	}),
).
	Prefix("/api").
	Middleware(middleware.Api...).
	Name("api.")

Or:

var Api = Group(
	Get("/ping", controllers.Ping).Name("ping"),
	Fallback(controllers.FallbackJson),
).
	Prefix("/api").
	Middleware(middleware.Api...).
	Name("api.")

Copy link
Contributor Author

@KevSlashNull KevSlashNull Feb 14, 2022

Choose a reason for hiding this comment

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

I’m not sure whether I fully understand the problem about middlewares from other routes. Do you mean that the name globalMiddlewares is inaccurate because for a route collection that is not bound as routes singleton on the app, the middlewares aren’t actually global in the scope of the application? If so yes, that’s true. Maybe we could rename it to just middlewares.


I was trying to make a middleware that serves static files from the disk. I didn’t want to manually create a fallback route because I wanted to keep the normal error behavior of encoder.ErrorToHtml to fall back to the error page without writing any code for it.

Using a fallback route here sounds like an interesting idea. This would implicitly alleviate the issue that global middlewares don’t run when a route is not registered for the path. When a fallback route is present on the Web group, all global and route-specific middlewares are executed.

How would you roughly go about adding a standard fallback (if that’s what you’re suggesting 😅) while retaining the standard not-found behavior?


Edit: this is the global fallback that I’ve added to my test app. We don’t have access to the app-specific views package in foundation though.

// in RouteServiceProvider.Boot
collection.Merge(routing.Fallback(func(request inter.Request) inter.Response {
	return outcome.Html(views.Error(request.App(), routing.RouteNotFoundError))
}))

Copy link
Contributor

Choose a reason for hiding this comment

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

You totally convinced me. After some thinking, I don't think a fallback route is a very nice solution.

Actually now my only objection is the naming of globalMiddlewares. Renaming it to middlewares is indeed much better. Than we can use that for something else later too. If you change that, I'll merge it 😄

Copy link
Contributor Author

@KevSlashNull KevSlashNull Feb 14, 2022

Choose a reason for hiding this comment

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

Cool, thank you 😄 I’ve renamed the slice to middlewares, back to you! 🏓

Copy link
Contributor

Choose a reason for hiding this comment

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

Released in v0.11.1

I also pasted the HEAD together with the GET in that release (same as in Laravel)
image

If a request is made to an URI which doesn’t resolve to a route,
Confetti will currently not execute a middleware. This makes it
impossible to add behavior for handling these soft errors gracefully or
log the request.

Therefore, this makes application middlewares run on routing errors by
storing the middlewares on the RouteCollection and setting them when
creating an error route.
@KevSlashNull KevSlashNull force-pushed the Run-application-middlewares-on-routing-errors branch from 6f4271a to 284d3a2 Compare February 14, 2022 22:17
@reindert-vetter reindert-vetter merged commit c905d8b into confetti-framework:main Feb 14, 2022
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