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

Add ResourceName to RouteInfo struct #1798

Merged
merged 6 commits into from
Oct 17, 2019
Merged

Add ResourceName to RouteInfo struct #1798

merged 6 commits into from
Oct 17, 2019

Conversation

alexisvisco
Copy link
Contributor

Hello I am doing a small feature that add the resource name to the RouteInfo struct as asked in the issue #1762.

Files changed

routes_info.go
route_mappings.go
route_mappings_test.go

How I have implemented that

In the route_mapping.go file the way to register a new resource is to call app.Resource right ?
To be simple as possible I am just counting the number of routes before all resources handlers are added. Then after all handlers are added I am looping over all routes after the index saved and set the field resourceName to rt.Name().
The Resource method look like this:

func (a *App) Resource(p string, r Resource) *App {
	[...]
	rt := rv.Type()
	resourceName := rt.Name()

	[...]

	indexBeforeResourceAdded := a.routes.Len()

	setFuncKey(r.List, fmt.Sprintf(handlerName, "List"))
	g.GET(p, r.List)

	setFuncKey(r.Show, fmt.Sprintf(handlerName, "Show"))
	g.GET(path.Join(spath), r.Show)

        [...]

	if n, ok := r.(newable); ok {
		setFuncKey(n.New, fmt.Sprintf(handlerName, "New"))
		g.GET(path.Join(p, "new"), n.New)
	}

        [...]

	// set route resource names for each routes added
	// no length check because cause at least 5 routes have been added so there cannot be an out of range
	for i := indexBeforeResourceAdded; i < a.routes.Len(); i++ {
		a.routes[i].ResourceName = resourceName
	}

	return g
}

I have also reordered all the way routes are added to be more concise. Now first there is non optional routes (the Resource interface methods) and then newable and editable. Edit: seems to have an order dependance with Show and New in the Test_ResourceOnResource test function so the order is now identical as before.

The resourceName is a field that will be set only if the handler is part of a resource.
The RouteInfo struct now look like this:

// RouteInfo provides information about the underlying route that
// was built.
type RouteInfo struct {
	Method       string     `json:"method"`
	Path         string     `json:"path"`
	HandlerName  string     `json:"handler"`
	ResourceName string     `json:"resourceName,omitempty"`
	PathName     string     `json:"pathName"`
	Aliases      []string   `json:"aliases"`
	MuxRoute     *mux.Route `json:"-"`
	Handler      Handler    `json:"-"`
	App          *App       `json:"-"`
}

To not polluate the Stringer implementation I have set a jsontag to omitempty.

Test add

A new test named Test_App_Routes_Resource check the resourceName on each routes of the resource.

func Test_App_Routes_Resource(t *testing.T) {
	r := require.New(t)

	a := New(Options{})
	r.Nil(a.root)

	a.GET("/foo", voidHandler)
	a.Resource("/r", resourceHandler{})

	routes := a.Routes()
	r.Len(routes, 6)
	route := routes[0]
	r.Equal("GET", route.Method)
	r.Equal("/foo/", route.Path)
	r.NotZero(route.HandlerName)

	for k, v := range routes {
		if k > 0 {
			r.Equal("resourceHandler", v.ResourceName)
		}
	}
}

@alexisvisco alexisvisco requested a review from a team as a code owner October 8, 2019 14:04
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.

Methods like GET and POST return *RouteInfo already, can you set the fields directly, and then not need to could and loop over things?

@alexisvisco
Copy link
Contributor Author

It's embarrassing I did not see that theses methods return a RouteInfo

@alexisvisco
Copy link
Contributor Author

@markbates It is now okay !

@markbates markbates added this to the v0.15.0 milestone Oct 17, 2019
@markbates markbates merged commit 6c27e55 into gobuffalo:development Oct 17, 2019
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