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

fix(server): enable original key filter #57

Merged
merged 3 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
140 changes: 82 additions & 58 deletions mongox/pagination.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,64 +27,7 @@ func (c *Collection) Paginate(ctx context.Context, rawFilter any, s *usecasex.So
filter = And(rawFilter, "", pFilter)
}

sortKey := idKey
sortOrder := 1
if s != nil && s.Key != "" {
sortKey = s.Key
if s.Reverted {
sortOrder = -1
}
}

if p.Cursor != nil && p.Cursor.Last != nil {
sortOrder *= -1
}

sort := bson.D{{Key: sortKey, Value: sortOrder}}
if sortKey != idKey {
sort = append(sort, bson.E{Key: idKey, Value: sortOrder})
}

findOpts := options.Find().
SetSort(sort).
SetLimit(limit(*p))

if p.Offset != nil {
findOpts.SetSkip(p.Offset.Offset)
}

cursor, err := c.collection.Find(ctx, filter, append([]*options.FindOptions{findOpts}, opts...)...)
if err != nil {
return nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("failed to find: %w", err))
}
defer func() {
_ = cursor.Close(ctx)
}()

count, err := c.collection.CountDocuments(ctx, rawFilter)
if err != nil {
return nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("failed to count: %w", err))
}

items, startCursor, endCursor, hasMore, err := consume(ctx, cursor, limit(*p))
if err != nil {
return nil, err
}

if p.Cursor != nil && p.Cursor.Last != nil {
reverse(items)
startCursor, endCursor = endCursor, startCursor
}

for _, item := range items {
if err := consumer.Consume(item); err != nil {
return nil, err
}
}

hasNextPage, hasPreviousPage := pageInfo(p, hasMore)

return usecasex.NewPageInfo(count, startCursor, endCursor, hasNextPage, hasPreviousPage), nil
return c.paginate(ctx, rawFilter, s, p, filter, consumer)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent use of rawFilter vs filter when calling paginate

In the Paginate method, you pass rawFilter as the rawFilter argument to c.paginate, while filter may include additional pagination conditions (pFilter). This could lead to inconsistencies during document counting in paginate, as rawFilter doesn't contain these additional conditions.

To ensure consistency between the documents counted and those retrieved, consider passing filter instead of rawFilter:

-return c.paginate(ctx, rawFilter, s, p, filter, consumer)
+return c.paginate(ctx, filter, s, p, filter, consumer)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return c.paginate(ctx, rawFilter, s, p, filter, consumer)
return c.paginate(ctx, filter, s, p, filter, consumer)

}

func (c *Collection) PaginateAggregation(ctx context.Context, pipeline []any, s *usecasex.Sort, p *usecasex.Pagination, consumer Consumer, opts ...*options.AggregateOptions) (*usecasex.PageInfo, error) {
Expand Down Expand Up @@ -327,3 +270,84 @@ func sortDirection(p usecasex.Pagination, s *usecasex.Sort) int {
}
return 1
}

func (c *Collection) PaginateProject(ctx context.Context, rawFilter any, s *usecasex.Sort, p *usecasex.Pagination, consumer Consumer, opts ...*options.FindOptions) (*usecasex.PageInfo, error) {
if p == nil || (p.Cursor == nil && p.Offset == nil) {
return nil, nil
}

pFilter, err := c.pageFilter(ctx, *p, s)
if err != nil {
return nil, rerror.ErrInternalByWithContext(ctx, err)
}

filter := rawFilter
if pFilter != nil {
filter = AddCondition(rawFilter, "", pFilter)
}

return c.paginate(ctx, rawFilter, s, p, filter, consumer)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential mismatch in document count due to rawFilter usage

In PaginateProject, you're passing rawFilter to the paginate method. Since filter may have added conditions (like pagination filters), using rawFilter for counting documents can result in an inaccurate count that doesn't reflect the actual query.

Consider updating the call to use filter:

-return c.paginate(ctx, rawFilter, s, p, filter, consumer)
+return c.paginate(ctx, filter, s, p, filter, consumer)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return c.paginate(ctx, rawFilter, s, p, filter, consumer)
return c.paginate(ctx, filter, s, p, filter, consumer)


}

func (c *Collection) paginate(ctx context.Context, rawFilter any, s *usecasex.Sort, p *usecasex.Pagination, filter any, consumer Consumer) (*usecasex.PageInfo, error) {

sortKey := idKey
sortOrder := 1
if s != nil && s.Key != "" {
sortKey = s.Key
if s.Reverted {
sortOrder = -1
}
}

if p.Cursor != nil && p.Cursor.Last != nil {
sortOrder *= -1
}

sort := bson.D{{Key: sortKey, Value: sortOrder}}
if sortKey != idKey {
sort = append(sort, bson.E{Key: idKey, Value: sortOrder})
}

findOpts := options.Find().
SetSort(sort).
SetLimit(limit(*p))

if p.Offset != nil {
findOpts.SetSkip(p.Offset.Offset)
}

cursor, err := c.collection.Find(ctx, filter, append([]*options.FindOptions{findOpts}, opts...)...)
if err != nil {
return nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("failed to find: %w", err))
}
defer func() {
_ = cursor.Close(ctx)
}()

count, err := c.collection.CountDocuments(ctx, rawFilter)
if err != nil {
return nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("failed to count: %w", err))
}
hexaforce marked this conversation as resolved.
Show resolved Hide resolved

items, startCursor, endCursor, hasMore, err := consume(ctx, cursor, limit(*p))
if err != nil {
return nil, err
}

if p.Cursor != nil && p.Cursor.Last != nil {
reverse(items)
startCursor, endCursor = endCursor, startCursor
}

for _, item := range items {
if err := consumer.Consume(item); err != nil {
return nil, err
}
}

hasNextPage, hasPreviousPage := pageInfo(p, hasMore)

return usecasex.NewPageInfo(count, startCursor, endCursor, hasNextPage, hasPreviousPage), nil
}
55 changes: 55 additions & 0 deletions mongox/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,3 +157,58 @@ func And(filter interface{}, key string, f interface{}) interface{} {
}
return AppendE(filter, bson.E{Key: key, Value: f})
}

func isEmptyCondition(condition interface{}) bool {
switch c := condition.(type) {
case bson.M:
return len(c) == 0
case bson.D:
return len(c) == 0
case bson.A:
return len(c) == 0
case []bson.M:
return len(c) == 0
case []bson.D:
return len(c) == 0
case []bson.A:
return len(c) == 0
case []interface{}:
return len(c) == 0
default:
return false
}
}

func AddCondition(filter interface{}, key string, condition interface{}) interface{} {
if condition == nil || isEmptyCondition(condition) {
return filter
}

filterMap, ok := filter.(bson.M)
if !ok || len(filterMap) == 0 {
filterMap = bson.M{}
}

var newCondition interface{}
if key != "" {
if _, exists := filterMap[key]; exists {
return filter
}
newCondition = bson.M{key: condition}
} else {
newCondition = condition
}

if existingAnd, ok := filterMap["$and"].(bson.A); ok {
filterMap["$and"] = append(existingAnd, newCondition)
} else {
existingConditions := make(bson.A, 0)
for k, v := range filterMap {
if k != "$and" {
existingConditions = append(existingConditions, bson.M{k: v})
}
}
filterMap["$and"] = append(existingConditions, newCondition)
}
return filterMap
}
Loading
Loading