Skip to content

Conversation

ajitdsa
Copy link

@ajitdsa ajitdsa commented Oct 1, 2025

This PR fixes a SQL COUNT syntax error in LiteSearch for ActiveRecord relations.

This aligns with our ongoing SQLite-first work using litestack in Rails 8.

Changes

• Addressed the ActiveRecord COUNT wrapping issue that produced the near AS syntax error when counting search relations with custom SELECTs. Specifically, the generated SQL looked like SELECT COUNT(channels.*, -channels_search_idx.rank AS search_rank) FROM … which breaks in SQLite.
• Refactored the search relation construction in lib/litestack/litesearch/model.rb to use a single cohesive SELECT that includes the base table columns plus the negative rank as search_rank, and orders by the index rank. This removes the prior pattern of multiple chained select() calls that confused COUNT.
• Introduced a custom count override on the returned search relation that performs COUNT via a simpler relation without the complex SELECT list, preserving filters (rank != 0), the MATCH clause, and joins to the FTS-backed index.
• Kept all existing search semantics intact: ranking, ordering, MATCH behavior, filter_column usage (e.g., active), polymorphic and custom primary key setups, and multi-model search_all behavior.
• Minor cleanup and consistency pass in tests and code to standardize quoting and schema declarations, while retaining behavior.
• Added test coverage for COUNT on search relations and verified behavior with filter_column :active and varied search terms.

Testing

• Unit tests updated in test/test_ar_search.rb:
▫ Verified polymorphic rich text search via Review and Comment models, ensuring MATCH across associated records.
▫ Confirmed similar() returns expected related records with proper search_rank handling.
▫ Ensured search works across models (Author, Book) and with custom primary keys (User.user_id, Post.post_id).
▫ Validated search_all returns multi-model results and respects ordering.
▫ Checked rebuild-on-modify and rebuild-later flows for the schema, including tokenizers and filters.
▫ Added explicit tests for COUNT on search results:
⁃ Book.search(‘night’).count returns 1, respecting active filter.
⁃ Varied terms (‘tale’, nonexistent) return correct counts (1 and 0 respectively).
• No explicit manual testing steps were attached in the PR body; behavior is demonstrated via automated tests.

Drive-by

I couldn't find contributing guidelines, but in an attempt to leave the campground slightly better than I found it, I ran rubocop on the file that I changed. So most of the changes in the file were rubocop formatting, changing guard clauses, etc. I am marking them in PR comments to help the reviewer.

Previously, calling .count on a LiteSearch search result would generate
invalid SQL with a double AS syntax error:

  SELECT COUNT(channels.*, -channels_search_idx.rank AS search_rank) FROM ...

This occurred because ActiveRecord couldn't handle the complex SELECT
clause with custom columns when wrapping the query in a COUNT operation.

Changes:
- Combined the two separate .select() calls into a single SELECT clause
- Added a custom count method override for search relations that uses
  a simpler query without the complex SELECT clause
- Preserved all search functionality including ranking and ordering

The fix ensures that both search results and count operations work
correctly:

  results = Channel.search("test")     # Works
  count = Channel.search("test").count # Now works!

Fixes the "near AS: syntax error" that occurred when ActiveRecord
tried to wrap LiteSearch queries in COUNT subqueries.

Test coverage added to verify COUNT functionality works with various
search terms and edge cases.
Copy link
Author

@ajitdsa ajitdsa left a comment

Choose a reason for hiding this comment

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

Marked which changes are rubocop formatting and which are my actual changes as a favor to the reviewer.

@@ -1,3 +1,5 @@
# frozen_string_literal: true

Copy link
Author

Choose a reason for hiding this comment

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

rubocop

end
return nil
end
return nil
Copy link
Author

Choose a reason for hiding this comment

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

All the changes above were rubocop formatting and don't change functionality at all. They move around some guard clauses, which cause indentation to be different, which accounts for most of the change.

obj = fetch_row(row["rowid"])
obj.search_rank = row["search_rank"]
obj = fetch_row(row['rowid'])
obj.search_rank = row['search_rank']
Copy link
Author

Choose a reason for hiding this comment

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

all of these " to ' changes are rubocop, so I won't mark any more of those.

"#{ActionText::RichText.table_name}.body"
rescue
"action_text_rich_texts.body"
rescue StandardError
Copy link
Author

Choose a reason for hiding this comment

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

rubocop added the error type here.

end
attributes[:reference] = :record_id
attributes[:conditions] = {record_type: model_class.name}
attributes[:conditions] = { record_type: model_class.name }
Copy link
Author

Choose a reason for hiding this comment

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

rubocop has a spacing preference around { and }


def allowed_attributes
super + [:polymorphic, :as, :action_text]
super + %i[polymorphic as action_text]
Copy link
Author

Choose a reason for hiding this comment

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

rubocop formatting preference.

schema.model_class = self if schema.respond_to? :model_class
end
@schema_not_created = false
end
Copy link
Author

Choose a reason for hiding this comment

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

Starting here is the meat of my changes to fix the SQL syntax error during COUNTs.

Copy link
Author

Choose a reason for hiding this comment

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

In this file, almost all changes were rubocop formatting. The tests I added are at the very bottom of the file.

# we have created 8 models, one ignore regex for each
assert_equal 8, ActiveRecord::SchemaDumper.ignore_tables.count
end

Copy link
Author

Choose a reason for hiding this comment

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

Here is my added test.

where(primary_key => id).get(:rowid)
end

def fetch_row(rowid)
Copy link
Author

Choose a reason for hiding this comment

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

👇 the rest of the changes in this file are rubocop formatting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant