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

MG-2287 - Improve Search For Things #2305

Merged
merged 31 commits into from
Jul 4, 2024

Conversation

nyagamunene
Copy link
Contributor

@nyagamunene nyagamunene commented Jun 23, 2024

What type of PR is this?

This is a feature because it improve search for Things

What does this do?

This enables back-end support for filtering by name, ID and tags.

Which issue(s) does this PR fix/relate to?

Have you included tests for your changes?

Yes, I have included tests for my changes.

Did you document any new/modified feature?

No. I have not updated the documentation because it will be done in a different PR

Notes

@nyagamunene nyagamunene marked this pull request as ready for review June 24, 2024 13:43
@@ -190,7 +190,7 @@ func (repo *Repository) RetrieveAll(ctx context.Context, pm clients.Page) (clien
return page, nil
}

func (repo *Repository) RetrieveAllBasicInfo(ctx context.Context, pm clients.Page) (clients.ClientsPage, error) {
func (repo *Repository) SearchBasicInfo(ctx context.Context, pm clients.Page) (clients.ClientsPage, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is not required, since we have SearchBasicInfo function in things postgres

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the comment left by @dborovcanin here, I understood that the renaming is necessary to avoid confusion between retrieve and search.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree
But the RetrieveAllBasicInfo will be no more needed , it will be replaced by search.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay then

// RetrieveAllBasicInfo list all clients only with basic information.
RetrieveAllBasicInfo(ctx context.Context, pm Page) (ClientsPage, error)
// SearchBasicInfo list all clients only with basic information.
SearchBasicInfo(ctx context.Context, pm Page) (ClientsPage, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is not required, since we have SearchBasicInfo function in things postgres

@nyagamunene nyagamunene force-pushed the MG-2287-ImproveSearchThings branch 3 times, most recently from f9e6804 to 2d76539 Compare June 28, 2024 23:19

"github.com/absmach/magistrala/internal/api"
Copy link
Collaborator

@dborovcanin dborovcanin Jun 30, 2024

Choose a reason for hiding this comment

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

Avoid importing API to repo even if it's internal.

@@ -33,6 +35,12 @@ type Repository interface {

// RetrieveBySecret retrieves a client based on the secret (key).
RetrieveBySecret(ctx context.Context, key string) (mgclients.Client, error)

// Delete deletes client with given id
Delete(ctx context.Context, id string) error
Copy link
Collaborator

@dborovcanin dborovcanin Jun 30, 2024

Choose a reason for hiding this comment

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

This change does not belong to this PR.

Comment on lines 201 to 204
query = append(query, "name ~* :name")
}
if pm.Identity != "" {
query = append(query, "id ~* :identity")
Copy link
Collaborator

@dborovcanin dborovcanin Jun 30, 2024

Choose a reason for hiding this comment

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

There is no need to use regular expresions, since we do not have an advanced query. LIKE or ILIKE should be a better fit and faster.

@nyagamunene nyagamunene force-pushed the MG-2287-ImproveSearchThings branch 2 times, most recently from 9954d0e to 3affcca Compare July 2, 2024 12:13
return mgclients.ClientsPage{}, err
}

cp, err := svc.clients.SearchBasicInfo(ctx, pm)
Copy link
Contributor

@arvindh123 arvindh123 Jul 2, 2024

Choose a reason for hiding this comment

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

@nyagamunene

Instead of SearchBasicInfo can rename to SearchClients ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can rename it to SearchClients no problem

@@ -181,7 +181,7 @@ func (svc service) ListClients(ctx context.Context, token, reqUserID string, pm

pm.IDs = ids

tp, err := svc.clients.RetrieveAllByIDs(ctx, pm)
tp, err := svc.SearchThings(ctx, token, pm)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why here we are calling SearchThing function
And not calling repo function svc.clients.SearchBasicInfo(ctx, pm)
`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the SearchThings were are checking if the user has authority to view the thing so it is separated from listclients when retrieving the basic info

Copy link
Contributor

Choose a reason for hiding this comment

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

Authorization are done on above switch case.

@@ -181,7 +181,7 @@ func (svc service) ListClients(ctx context.Context, token, reqUserID string, pm

pm.IDs = ids

tp, err := svc.clients.RetrieveAllByIDs(ctx, pm)
tp, err := svc.SearchThings(ctx, token, pm)
Copy link
Contributor

Choose a reason for hiding this comment

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

Authorization are done on above switch case.

Comment on lines 545 to 550
for _, val := range cp.Clients {
if _, err := svc.authorize(ctx, res.GetDomainId(), auth.UserType, auth.UsersKind, res.GetId(), auth.ViewPermission, auth.ThingType, val.ID); err == nil {
things.Clients = append(things.Clients, val)
things.Page.Total += 1
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not return proper page items.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need this SearchThings function in service layer

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 will remove the SearchThings function 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 authorization is needed because the member should not be able to view things created by other members

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, In ListThings it is handled.

Comment on lines 545 to 550
for _, val := range cp.Clients {
if _, err := svc.authorize(ctx, res.GetDomainId(), auth.UserType, auth.UsersKind, res.GetId(), auth.ViewPermission, auth.ThingType, val.ID); err == nil {
things.Clients = append(things.Clients, val)
things.Page.Total += 1
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, In ListThings it is handled.

}
if len(pm.IDs) != 0 {
query = append(query, fmt.Sprintf("id IN ('%s')", strings.Join(pm.IDs, "','")))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this will make code reusable and make simpler.

Suggested change
}
func buildQueryConditions(pm clients.Page) ([]string) {
var query []string
if pm.Name != "" {
query = append(query, "name ILIKE '%' || :name || '%'")
}
if pm.Identity != "" {
query = append(query, "identity ILIKE '%' || :identity || '%'")
}
if pm.Id != "" {
query = append(query, "id ILIKE '%' || :id || '%'")
}
if pm.Tag != "" {
query = append(query, "EXISTS (SELECT 1 FROM unnest(tags) AS tag WHERE tag ILIKE '%' || :tag || '%')")
}
if len(pm.IDs) != 0 {
query = append(query, fmt.Sprintf("id IN ('%s')", strings.Join(pm.IDs, "','")))
}
return query
}
func applyOrdering(emq string, pm clients.Page) string {
switch pm.Order {
case "name", "identity", "created_at", "updated_at":
emq = fmt.Sprintf("%s ORDER BY %s", emq, pm.Order)
if pm.Dir == api.AscDir || pm.Dir == api.DescDir {
emq = fmt.Sprintf("%s %s", emq, pm.Dir)
}
}
return emq
}
func PageQuery(pm clients.Page) (string, error) {
mq, _, err := postgres.CreateMetadataQuery("", pm.Metadata)
if err != nil {
return "", errors.Wrap(errors.ErrMalformedEntity, err)
}
query := buildQueryConditions(pm)
if mq != "" {
query = append(query, mq)
}
// Additional conditions specific to PageQuery
if pm.Status != clients.AllStatus {
query = append(query, "c.status = :status")
}
if pm.Domain != "" {
query = append(query, "c.domain_id = :domain_id")
}
if pm.Role != clients.AllRole {
query = append(query, "c.role = :role")
}
if len(query) > 0 {
emq = fmt.Sprintf("WHERE %s", strings.Join(query, " AND "))
}
var emq string
if len(query) > 0 {
emq = fmt.Sprintf("WHERE %s", strings.Join(query, " AND "))
}
emq = applyOrdering(emq, pm)
return emq, nil
}
func ConstructSearchQuery(pm clients.Page) (string) {
query := buildQueryConditions(pm)
var emq string
if len(query) > 0 {
emq = fmt.Sprintf("WHERE %s", strings.Join(query, " AND "))
}
emq = applyOrdering(emq, pm)
return emq, tq
}

@@ -364,280 +364,6 @@ func TestCreateThings(t *testing.T) {
}
}

func TestListThings(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this test removed?

if len(query) > 0 {
emq = fmt.Sprintf("WHERE %s", strings.Join(query, " AND "))
}
return emq, nil
}

func ConstructSearchQuery(pm clients.Page) (string, string) {
var query []string
func constructSearchQuery(pm clients.Page) (string, string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is used only once. Remove it and put the code directly to the SearchClients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

if pm.Tag != "" {
query = append(query, "EXISTS (SELECT 1 FROM unnest(tags) AS tag WHERE tag ILIKE '%' || :tag || '%')")
}
if len(query) == 0 && len(pm.IDs) != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are treating IDs differently, i.e. taking it into account only if other criteria is not there, making buildQueryConditions mask the logic of processing IDs. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is allows searchClients to retrieve basic info by id or search. So if either name, tag, identify, or id is set we don't retrieve by id

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't we still combine? If you want only IDs, you send only IDs in the query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can but to get you properly, you are suggesting we combine both searchClients and retrieveAllByIds right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Eventually, that would be the outcome here, yes. I.e. Search users from this pool of IDs with this criteria. It's unlikely to need such a thing, but it makes buildQueryConditions consistent.

Copy link
Contributor

@arvindh123 arvindh123 Jul 3, 2024

Choose a reason for hiding this comment

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

@dborovcanin @nyagamunene
For search with ID, it will work even the ID matches partially.
So we need list of things to which user have access.
As you know, Things service could not have information of user accessiable thing, so it will retreive from spiceDB for all the IDs to which user have access.

Once we find way to list things efficently with access control, then we can remove this IDs from entities

if pm.Tag != "" {
query = append(query, "EXISTS (SELECT 1 FROM unnest(tags) AS tag WHERE tag ILIKE '%' || :tag || '%')")
}
if len(query) == 0 && len(pm.IDs) != 0 {
Copy link
Contributor

@arvindh123 arvindh123 Jul 3, 2024

Choose a reason for hiding this comment

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

@dborovcanin @nyagamunene
For search with ID, it will work even the ID matches partially.
So we need list of things to which user have access.
As you know, Things service could not have information of user accessiable thing, so it will retreive from spiceDB for all the IDs to which user have access.

Once we find way to list things efficently with access control, then we can remove this IDs from entities

pkg/clients/postgres/clients.go Outdated Show resolved Hide resolved
users/service.go Outdated
Identity: pm.Identity,
Role: mgclients.UserRole,
}
pg, err := svc.clients.SearchBasicInfo(ctx, p)
pg, err := svc.clients.SearchClients(ctx, p)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to pulicate the mgclients.Page, instead we can pass variable pm directly to SearchClients

Suggested change
pg, err := svc.clients.SearchClients(ctx, p)
pg, err := svc.clients.SearchClients(ctx, pm)


func buildSearch(query []string, pm clients.Page) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this function and streamline PageQuery to something like:

Suggested change
func buildSearch(query []string, pm clients.Page) string {
func PageQuery(pm clients.Page) (string, error) {
mq, _, err := postgres.CreateMetadataQuery("", pm.Metadata)
if err != nil {
return "", errors.Wrap(errors.ErrMalformedEntity, err)
}
var query []string
if pm.Name != "" {
query = append(query, "name ILIKE '%' || :name || '%'")
}
if pm.Identity != "" {
query = append(query, "identity ILIKE '%' || :identity || '%'")
}
if pm.Id != "" {
query = append(query, "id ILIKE '%' || :id || '%'")
}
if pm.Tag != "" {
query = append(query, "EXISTS (SELECT 1 FROM unnest(tags) AS tag WHERE tag ILIKE '%' || :tag || '%')")
}
if pm.Role != clients.AllRole {
query = append(query, "c.role = :role")
}
// If there are search params presents, use search and ignore other options.
// Always combine role with search params, so len(query) > 1.
if len(query) > 1 {
return fmt.Sprintf("WHERE %s", strings.Join(query, " AND ")), nil
}
if mq != "" {
query = append(query, mq)
}
if len(pm.IDs) != 0 {
query = append(query, fmt.Sprintf("id IN ('%s')", strings.Join(pm.IDs, "','")))
}
if pm.Status != clients.AllStatus {
query = append(query, "c.status = :status")
}
if pm.Domain != "" {
query = append(query, "c.domain_id = :domain_id")
}
var emq string
if len(query) > 0 {
emq = fmt.Sprintf("WHERE %s", strings.Join(query, " AND "))
}
return emq, nil
}

Don't forget comments, it's important because the function is a little cryptic. We must improve, I have opened a ticket for this: #2326.

Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
@dborovcanin dborovcanin merged commit 1302441 into absmach:main Jul 4, 2024
6 checks passed
andychao217 pushed a commit to andychao217/magistrala that referenced this pull request Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Improve search for Users, Things, Channels, and Groups
3 participants