Skip to content

fix(filter): fix FilterULID select implementations#4368

Merged
tothandras merged 1 commit into
mainfrom
fix/filter-ulid
May 15, 2026
Merged

fix(filter): fix FilterULID select implementations#4368
tothandras merged 1 commit into
mainfrom
fix/filter-ulid

Conversation

@tothandras
Copy link
Copy Markdown
Contributor

@tothandras tothandras commented May 15, 2026

Summary by CodeRabbit

  • Bug Fixes

    • Refined filter evaluation logic to properly handle complex filter combinations.
  • Tests

    • Added comprehensive test coverage for nested filter conditions and AND/OR operations.

Review Change Stack

@tothandras tothandras requested a review from a team as a code owner May 15, 2026 19:12
@tothandras tothandras added the release-note/bug-fix Release note: Bug Fixes label May 15, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0dfbea9b-db82-42fd-bfcb-e378a8cb0399

📥 Commits

Reviewing files that changed from the base of the PR and between ff255ce and 73844b7.

📒 Files selected for processing (2)
  • pkg/filter/filter.go
  • pkg/filter/filter_test.go

📝 Walkthrough

Walkthrough

This PR extends FilterULID to properly handle composed And/Or filters by treating them as SQL predicates. Previously, emptiness checks and SQL generation ignored composition; now they recursively process child filters and correctly identify when filters are truly empty.

Changes

FilterULID And/Or Composition Support

Layer / File(s) Summary
Empty filter contract
pkg/filter/filter.go
IsEmpty() now requires both the embedded FilterString and the And/Or fields to be absent for a filter to be considered empty.
SQL generation for composed filters
pkg/filter/filter.go
SelectWhereExpr() and Select() now recursively convert And/Or child filters into q.And() / q.Or() and sql.AndPredicates() / sql.OrPredicates() calls; non-composed filters delegate to embedded FilterString logic.
Test coverage for And/Or recursion and emptiness
pkg/filter/filter_test.go
New tests verify that SelectWhereExpr and Select correctly recurse through And/Or wrappers and generate correct SQL for both go-sqlbuilder and Ent paths; IsEmpty() tests confirm behavior for zero-value and partially-filled filters.

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: fixing FilterULID's select implementations to properly handle And/Or composition and update IsEmpty logic.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/filter-ulid

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tothandras tothandras enabled auto-merge (squash) May 15, 2026 19:14
@tothandras tothandras merged commit 6f51b29 into main May 15, 2026
27 of 28 checks passed
@tothandras tothandras deleted the fix/filter-ulid branch May 15, 2026 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/bug-fix Release note: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants