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.

Sign up for GitHub

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

feat: add owner/name filter to list clients #2637

Merged
merged 6 commits into from
Aug 2, 2021
Merged

feat: add owner/name filter to list clients #2637

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ffb8604
feat: add owner/name filter to list clients
pike1212 Jul 13, 2021
2252759
fix: update tests so client has different name
pike1212 Jul 13, 2021
66699f0
fix: add client filter params to swagger doc
pike1212 Jul 14, 2021
a4492df
chore: code rview
aeneasr Jul 30, 2021
3953547
chore: code review
aeneasr Jul 30, 2021
7c86062
Merge branch 'master' into feature/clientFilters
aeneasr Aug 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,13 @@ type Client struct {
Metadata sqlxx.JSONRawMessage `json:"metadata,omitempty" db:"metadata"`
}

type ClientFilters struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be added to the swagger spec:

hydra/client/doc.go

Line 90 in b597c88

type swaggerListClientsParameter struct {

You can also probably re-use / re-purpose that struct as ClientFilter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the name/owner params to that struct, is that all that needs done?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's all that's needed - I think this can be removed then!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ClientFilters struct is used internally to get passed to the client manager, so new filers could be easily added...

Limit int
Offset int
Owner string
Name string
}

func (Client) TableName() string {
return "hydra_client"
}
Expand Down
8 changes: 8 additions & 0 deletions client/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,14 @@ type swaggerListClientsParameter struct {
// The offset from where to start looking.
// in: query
Offset int `json:"offset"`

// The name of the clients to filter by.
// in: query
Name string `json:"name"`

// The owner of the clients to filter by.
// in: query
Owner string `json:"owner"`
}

// A list of clients.
Expand Down
8 changes: 7 additions & 1 deletion client/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,14 @@ func (h *Handler) updateClient(ctx context.Context, c *Client) error {
// 500: jsonError
func (h *Handler) List(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
limit, offset := pagination.Parse(r, 100, 0, 500)
filters := ClientFilters{
Limit: limit,
Offset: offset,
Name: r.URL.Query().Get("client_name"),
Owner: r.URL.Query().Get("owner"),
}

c, err := h.r.ClientManager().GetClients(r.Context(), limit, offset)
c, err := h.r.ClientManager().GetClients(r.Context(), filters)
if err != nil {
h.r.Writer().WriteError(w, r, err)
return
Expand Down
2 changes: 1 addition & 1 deletion client/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type Storage interface {

DeleteClient(ctx context.Context, id string) error

GetClients(ctx context.Context, limit, offset int) ([]Client, error)
GetClients(ctx context.Context, filters ClientFilters) ([]Client, error)

CountClients(ctx context.Context) (int, error)

Expand Down
25 changes: 21 additions & 4 deletions client/manager_test_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func TestHelperCreateGetUpdateDeleteClient(k string, m Storage) func(t *testing.

assert.NoError(t, m.CreateClient(ctx, &Client{
OutfacingID: "2-1234",
Name: "name",
Name: "name2",
Secret: "secret",
RedirectURIs: []string{"http://redirect"},
TermsOfServiceURI: "foo",
Expand All @@ -141,7 +141,7 @@ func TestHelperCreateGetUpdateDeleteClient(k string, m Storage) func(t *testing.

compare(t, c, d, k)

ds, err := m.GetClients(ctx, 100, 0)
ds, err := m.GetClients(ctx, ClientFilters{Limit: 100, Offset: 0})
assert.NoError(t, err)
assert.Len(t, ds, 2)
assert.NotEqual(t, ds[0].OutfacingID, ds[1].OutfacingID)
Expand All @@ -150,13 +150,30 @@ func TestHelperCreateGetUpdateDeleteClient(k string, m Storage) func(t *testing.
assert.Equal(t, ds[0].SecretExpiresAt, 0)
assert.Equal(t, ds[1].SecretExpiresAt, 1)

ds, err = m.GetClients(ctx, 1, 0)
ds, err = m.GetClients(ctx, ClientFilters{Limit: 1, Offset: 0})
assert.NoError(t, err)
assert.Len(t, ds, 1)

ds, err = m.GetClients(ctx, 100, 100)
ds, err = m.GetClients(ctx, ClientFilters{Limit: 100, Offset: 100})
assert.NoError(t, err)

// get by name
ds, err = m.GetClients(ctx, ClientFilters{Limit: 100, Offset: 0, Name: "name"})
assert.NoError(t, err)
assert.Len(t, ds, 1)
assert.Equal(t, ds[0].Name, "name")

// get by name not exist
ds, err = m.GetClients(ctx, ClientFilters{Limit: 100, Offset: 0, Name: "bad name"})
assert.NoError(t, err)
assert.Len(t, ds, 0)

// get by owner
ds, err = m.GetClients(ctx, ClientFilters{Limit: 100, Offset: 0, Owner: "aeneas"})
assert.NoError(t, err)
assert.Len(t, ds, 1)
assert.Equal(t, ds[0].Owner, "aeneas")

err = m.UpdateClient(ctx, &Client{
OutfacingID: "2-1234",
Name: "name-new",
Expand Down
20 changes: 14 additions & 6 deletions persistence/sql/persister_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,21 @@ func (p *Persister) DeleteClient(ctx context.Context, id string) error {
return sqlcon.HandleError(p.Connection(ctx).Destroy(&client.Client{ID: cl.ID}))
}

func (p *Persister) GetClients(ctx context.Context, limit, offset int) ([]client.Client, error) {
func (p *Persister) GetClients(ctx context.Context, filters client.ClientFilters) ([]client.Client, error) {
cs := make([]client.Client, 0)
return cs, sqlcon.HandleError(
p.Connection(ctx).
Paginate(offset/limit+1, limit).
Order("id").
All(&cs))

query := p.Connection(ctx).
Paginate(filters.Offset/filters.Limit+1, filters.Limit).
Order("id")

if filters.Name != "" {
query.Where("client_name = ?", filters.Name)
}
if filters.Owner != "" {
query.Where("owner = ?", filters.Owner)
}
Comment on lines +86 to +91
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably means that we need to index these fields? Otherwise this will get very slow when you have lots of clients. Then again, if we just have like 1000 clients, it probably wouldn't matter too much (maybe 5ms with index, 200ms without). What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For our use case, the index won't make much of a difference

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we can keep the filter omitted for now and add it later.


return cs, sqlcon.HandleError(query.All(&cs))
}

func (p *Persister) CountClients(ctx context.Context) (int, error) {
Expand Down