Css 9402/resources #1347
Css 9402/resources #1347
Conversation
There was a problem hiding this comment.
Initial run over, will go over again, looking more closely at query and pagination. Not keen on having test with integration in the name... You can definitely unit test just the handler with mocks to test the handlers logic. This probably something to discuss at standup.
SimoneDutto
force-pushed
the
CSS-9402/resources
branch
from
765b103 to
0c68eb4
Compare
In the rebac admin folder we use this approach of having |
| @@ -62,6 +62,22 @@ func CreatePagination(sizeP, pageP *int, total int) (int, *int, LimitOffsetPagin | |||
| return page, nextPage, NewOffsetFilter(pageSize, offset) | |||
| } | |||
|
|
|||
| // CreatePagination returns the current page, the page size, and the pagination.LimitOffsetPagination. | |||
| // This method is different approach to the method `CreatePagination` when we don't have the total number of records. | |||
| func CreatePaginationWithoutTotal(sizeP, pageP *int) (int, int, LimitOffsetPagination) { | |||
There was a problem hiding this comment.
We should probably return a struct here and in CreatePagination because I'm very confused looking at this function and even the test, what int, int represent.
There was a problem hiding this comment.
Or I like the idea of named returns here.
There was a problem hiding this comment.
yeah, you are right on this. It's pretty confusing without names
There was a problem hiding this comment.
changed to named returns and improved godoc
| c := qt.New(t) | ||
| pPage := utils.IntToPointer(0) | ||
| pSize := utils.IntToPointer(10) | ||
| page, size, pag := pagination.CreatePaginationWithoutTotal(pSize, pPage) |
There was a problem hiding this comment.
page is the offset?
size is the number of returned elements? Why is it the page size + 1?
And pag if the filter, maybe the name could be changed.
internal/db/resources.go
Outdated
| FROM application_offers | ||
| JOIN models ON application_offers.model_id = models.id |
There was a problem hiding this comment.
Why do we need the parent objects? Does the rebac-admin spec require that?
There was a problem hiding this comment.
yes, you can find in the issue on Jira the entities who need the parent as well https://warthogs.atlassian.net/browse/CSS-9402
internal/rebac_admin/resources.go
Outdated
| if len(res) == pageSize { | ||
| nPage := page + 1 | ||
| nextPage = &nPage | ||
| res = res[:len(res)-1] | ||
| } |
There was a problem hiding this comment.
I get it, but it's confusing to read this.
There was a problem hiding this comment.
Just make a func maybe call geTnextPageAndResponse or something
There was a problem hiding this comment.
did it, thank you for the suggestion
| Next: resources.Next{ | ||
| Page: nextPage, | ||
| }, |
There was a problem hiding this comment.
Shouldn't this whole object be nil if nextPage is nil?
There was a problem hiding this comment.
it could, but i think it complicates the code to add an if nextPage == nil to exclude the Next argument.
What do you think?
There was a problem hiding this comment.
Also, we used the same approach for all the other endpoints
internal/rebac_admin/resources.go
Outdated
| // Then, we set the next page if we have this many records. | ||
| // If it does we return the records minus 1 and advide the consumer there is another page. |
There was a problem hiding this comment.
The wording is confusing here,
Reading this I see
- If we have as many records as requested, we set the next page.
- If we don't have as many records as request, we return records minus 1 and tell client there is another page.
- Else we return the records and set the next page as empty.
Aren't points 1 and 3 saying the same thing?
There was a problem hiding this comment.
you are right, i made a mess with c/p. Now it's updated.
Description
This pr adds the resources endpoint.
The expected response is a paginated list of Resources (clouds, controllers, models, service_accounts, application offers).
We implemented this with a raw query with multiple union for the different tables.
The reason behind a raw query instead of a view is that view doesn't provide any cons in our situation.
Fixes JIRA/GitHub issue number
Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers