Skip to content
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

Merged
merged 12 commits into from
Oct 23, 2024
Merged

Add default limit to IS List RPCs #7345

merged 12 commits into from
Oct 23, 2024

Conversation

ryaplots
Copy link
Contributor

@ryaplots ryaplots commented Oct 15, 2024

Summary

References #7327

Changes

  • Add new pagination default
  • Add withLimit to check if the limit is set
  • Use default if req.Limit is not set

Testing

  1. Make a request to get a list of entities with the page limit unset;
  2. The request should return 100 results per page.
Steps

Run tests

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Testing: The steps/process to test this feature are clearly explained including testing for regressions.
  • Infrastructure: If infrastructural changes (e.g., new RPC, configuration) are needed, a separate issue is created in the infrastructural repositories.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@github-actions github-actions bot added c/identity server This is related to the Identity Server compat/db This could affect Database compatibility compat/config This could affect Configuration compatibility labels Oct 15, 2024
@ryaplots ryaplots changed the title Fix/default pagination Add default limit to IS List RPCs Oct 17, 2024
@ryaplots ryaplots marked this pull request as ready for review October 17, 2024 11:51
@ryaplots ryaplots requested review from a team as code owners October 17, 2024 11:51
Copy link
Member

@KrishnaIyer KrishnaIyer left a 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).

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/identityserver/config.go Outdated Show resolved Hide resolved
pkg/identityserver/store/pagination.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nicholaspcr nicholaspcr left a 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:

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.

pkg/identityserver/config.go Outdated Show resolved Hide resolved
pkg/identityserver/config.go Outdated Show resolved Hide resolved
@nicholaspcr
Copy link
Contributor

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

@ryaplots
Copy link
Contributor Author

I've changed the implementation and moved the default check to the store.
Do I still need unit tests for this? @KrishnaIyer

Copy link
Contributor

@nicholaspcr nicholaspcr left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/identityserver/storetest/api_key_store.go Outdated Show resolved Hide resolved
pkg/identityserver/storetest/api_key_store.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nicholaspcr nicholaspcr left a 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

pkg/identityserver/storetest/user_store.go Outdated Show resolved Hide resolved
Copy link
Member

@KrishnaIyer KrishnaIyer left a 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.

@nicholaspcr
Copy link
Contributor

One last comment which is a nit. Squashing fix commits helps if the reviewers wants to look at some specific commit, the bad side of it is that it forces you to do a push -f.

Copy link
Contributor

@nicholaspcr nicholaspcr left a 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.

pkg/identityserver/bunstore/store_test.go Outdated Show resolved Hide resolved
@ryaplots ryaplots merged commit b911c66 into v3.32 Oct 23, 2024
12 of 13 checks passed
@ryaplots ryaplots deleted the fix/default-pagination branch October 23, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/identity server This is related to the Identity Server compat/config This could affect Configuration compatibility compat/db This could affect Database compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants