-
Notifications
You must be signed in to change notification settings - Fork 310
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
Add default limit to IS List RPCs #7345
Conversation
5279371
to
af342ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality is good but missing tests (unit and e2e).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the idea of introducing the default pagination value is nice and I support it, I have two comments about the current implementation.
1 - The default value seems to small
The default value seems quite low, being at 10. This will be imposed on the CLI requests as well and I imagine that the change will surprise users who are used to fetching applications and gateways via the CLI.
I'm not saying that we shouldn't do it but rather increase it to be at least 100 and update the CLI limit
description to inform that the default is 100
and that 0
is not a valid option.
2 - how the default value is set propagated.
I'm not a fan of injecting the default value of pagination in the context. I rather set this value at the store initialization, especially if it is going to be enforced at every list request.
To be more precise I think it would be better to have functional options in the bunstore initialization:
lorawan-stack/pkg/identityserver/bunstore/store.go
Lines 142 to 148 in 4203826
func NewStore(ctx context.Context, db *bun.DB) (*Store, error) { | |
baseDB, err := newDB(ctx, db) | |
if err != nil { | |
return nil, err | |
} | |
return newStore(baseDB.baseStore()), nil | |
} |
I know this deviates a bit from what is written on the issue in which this PR is based on but I think its worth considering having the responsibility of handling the zero values to the store implementation rather than leaving it on the business logic.
Forgot to tag you @KrishnaIyer |
1beceaa
to
086ca94
Compare
I've changed the implementation and moved the default check to the store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having the responsibility of defining the default limit value inside the store
package, that way its agnostic to the bunstore implementation and the default value of limit
is used every time we call store.WithPagination
with a limit set to zero.
There shouldn't be any problems with setting the default value once before starting the bunstore instance since all further interactions with the store
package's internal variable paginationDefaults
will be read operations only.
I wrote a few comments regarding the tests as I believe they aren't being executed and most likely should yield an error in their current state, but outside of that this looks pretty good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
I left a comment regarding the tests but besides this the PR looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No additional points from me. This can be merged after @nicholaspcr's comments are addressed.
One last comment which is a |
f3e1623
to
30b3710
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Before merging please fix the wrong test reference in TestApplicationStore
.
5e7585e
to
10a63eb
Compare
Summary
References #7327
Changes
withLimit
to check if the limit is setreq.Limit
is not setTesting
Steps
Run tests
Checklist
README.md
for the chosen target branch.CHANGELOG.md
.CONTRIBUTING.md
, there are no fixup commits left.