Cleanup filtering & pagination #451

fabriziosestito · 2021-11-15T08:08:55Z

This PR cleans up the filtering in the new db based services.
It also leverages the database for pagination adding a new gorm scope.

…l count method

…ount method

dottorblaster

LGTM

dottorblaster · 2021-11-16T08:20:25Z

web/services/hosts_next.go

+		if filter != nil && len(filter.Health) > 0 {
+			if !internal.Contains(filter.Health, host.Health) {


I know what you'll say, but making a function for these conditions is always viable to me :D

yep im not sure this will stay i will keep on the radar for the next iterations!

nelsonkopliku

Great!

nelsonkopliku · 2021-11-16T08:24:56Z

web/services/clusters_test.go

@@ -96,7 +96,7 @@ func (suite *ClustersServiceTestSuite) TestClustersService_GetAll() {
 	suite.checksService.On("GetAggregatedChecksResultByCluster", "2").Return(&AggregatedCheckData{WarningCount: 1}, nil)
 	suite.checksService.On("GetAggregatedChecksResultByCluster", "3").Return(&AggregatedCheckData{CriticalCount: 1}, nil)

-	clusters, _ := suite.clustersService.GetAll(map[string][]string{})
+	clusters, _ := suite.clustersService.GetAll(nil, nil)


I find it a bit meh having to call .GetAll(nil. nil) to get the complete list.
I'd maybe design differently the interface with a method that actually gets all the clusters, and one for filtered purposes.

Yet I understand why you did this, so it's a good iteration.

I have to say i did a bit of reasearch and the use of nil is pretty common also in the golang standard library.
There are other tricks with variadic arugments but they mean a lot of boiler code that I'm not confident will be helpful at the end.
On the other hand, adding another method will add more code to test and mantain, so again the idea is to minimize that since we are going to use that method with filtering and pagination 99.9% of the time.

EDIT:
see for example the jpeg library, but the are many of this: https://pkg.go.dev/image/jpeg#Encode

@nelsonkopliku I think the same about it, but bear in mind that ignoring the "nil is evil" line of thoughs is a cornerstone in Golang's design. Hence, the researches done by @fabriziosestito show something that I absolutely dislike, but extremely idiomatic at the same time 😄

@nelsonkopliku @dottorblaster see the edit of my comment, i was also biased against nil eheh if we find a better solution i am all hear but i could not!

makes sense guys, thanks for clarification.
Sometimes I still have to wrap my head around some go quirks, but that's ok, just need to deal with it 😅

arbulu89

Hey @fabriziosestito ,
Nice migration!
I find a bit anti gin-gonic how the query binding is done. Even though that it is not documented properly, the query entries can be bind to an struct in the same way than the post body data.
I have put an example link in one of the comments.

arbulu89 · 2021-11-16T08:26:53Z

web/clusters.go

@@ -292,7 +293,29 @@ func NewClusterListHandler(clustersService services.ClustersService) gin.Handler
 	return func(c *gin.Context) {
 		query := c.Request.URL.Query()

-		clusterList, err := clustersService.GetAll(query)
+		clustersFilter := &services.ClustersFilter{


This way of binding looks quite anti gin-gonic. What about using c.Bind here?
Here an example: gin-gonic/gin#742
In summary, we would need a struct (ClustersFilter) which would get all the information.
I would give a try at least

@arbulu89 thanks i will try this out! we already have ClustersFilter struct in place so it should be cleaner.

@arbulu89 i tried this and i am not sure since it involves adding a form annotation to the ClustersService struct while I would prefer to keep the to layers decoupled or at least not having the service layer depending on the controller one at all.

Well, I guess the ideal would be to have an additional struct that receives the query data with the form annotations, and it can be converted to the other page filter. Now that we have api structs, entities, models, yet another layer hehe

Anyway, feel free to implement as you wish. I just thought that using the gin gonic native way to deal with query parameters was better.

I will remove my request changes review.

web/clusters.go

arbulu89 · 2021-11-16T08:27:58Z

web/hosts_next.go

@@ -27,7 +28,27 @@ func NewHostListNextHandler(hostsService services.HostsNextService) gin.HandlerF
 	return func(c *gin.Context) {
 		query := c.Request.URL.Query()

-		hostList, err := hostsService.GetAll(query)
+		hostsFilter := &services.HostsFilter{


Same here. I would try to use the Bind method

Removing my old review. Other people have already reviewed and my comments might not be implemented

fabriziosestito requested review from arbulu89, dottorblaster, nelsonkopliku and rtorrero November 15, 2021 08:08

fabriziosestito marked this pull request as draft November 15, 2021 08:10

fabriziosestito added 2 commits November 15, 2021 19:55

Add pagination scope and tests

2620e07

Add filtering and pagination parameters to get all clusters; add tota…

a46ca1e

…l count method

fabriziosestito force-pushed the cleanup_filtering_pagination_services branch from 0723c71 to 942ec71 Compare November 15, 2021 18:57

fabriziosestito marked this pull request as ready for review November 15, 2021 18:57

fabriziosestito added 3 commits November 15, 2021 20:00

Refactor clusters list handler with new pagination/filtering

729a894

Add filtering and pagination parameters to get all hosts; add total c…

5375b5b

…ount method

Refactor host list handler with new pagination/filtering

b3c27d0

fabriziosestito force-pushed the cleanup_filtering_pagination_services branch from 942ec71 to b3c27d0 Compare November 15, 2021 19:01

dottorblaster approved these changes Nov 16, 2021

View reviewed changes

nelsonkopliku approved these changes Nov 16, 2021

View reviewed changes

arbulu89 previously requested changes Nov 16, 2021

View reviewed changes

fabriziosestito merged commit 0082194 into trento-project:main Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup filtering & pagination #451

Cleanup filtering & pagination #451

fabriziosestito commented Nov 15, 2021 •

edited

Loading

dottorblaster left a comment

dottorblaster Nov 16, 2021

fabriziosestito Nov 16, 2021

nelsonkopliku left a comment

nelsonkopliku Nov 16, 2021

fabriziosestito Nov 16, 2021 •

edited

Loading

dottorblaster Nov 16, 2021

fabriziosestito Nov 16, 2021

nelsonkopliku Nov 16, 2021

arbulu89 left a comment

arbulu89 Nov 16, 2021

fabriziosestito Nov 16, 2021 •

edited

Loading

fabriziosestito Nov 16, 2021

arbulu89 Nov 16, 2021

arbulu89 Nov 16, 2021

		if filter != nil && len(filter.Health) > 0 {
		if !internal.Contains(filter.Health, host.Health) {

Cleanup filtering & pagination #451

Cleanup filtering & pagination #451

Conversation

fabriziosestito commented Nov 15, 2021 • edited Loading

dottorblaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsonkopliku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziosestito Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziosestito Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziosestito commented Nov 15, 2021 •

edited

Loading

fabriziosestito Nov 16, 2021 •

edited

Loading

fabriziosestito Nov 16, 2021 •

edited

Loading