fix: disallow distinct for sqlite

[ymc9]

No description provided.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing Touches

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/sqlite-disable-distinct

---

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

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

• Visit our Status Page to check the current availability of CodeRabbit.
• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[claude]

Claude finished @ymc9's task —— View job

---

PR Review: SQLite DISTINCT Fix

Reviewing this pull request that disallows DISTINCT operations for SQLite.

Tasks

• Read and analyze all 7 changed files
• Understand the SQLite DISTINCT limitation being addressed
• Review implementation approach and architecture changes
• Assess code quality and potential issues
• Check test coverage adequacy
• Provide comprehensive review feedback

---

Summary

This PR implements proper SQLite DISTINCT operation handling by disallowing it where not supported and refactoring the dialect architecture for better provider-specific feature support.

Key Changes

1. Type System Updates (crud-types.ts)

• Introduced ProviderSupportsDistinct<Schema> conditional type
• Made distinct field optional in FindArgs based on provider support
• Clean provider-aware typing that prevents invalid operations at compile time

2. Architecture Refactoring (dialects/base.ts)

• Major improvement: Moved 100+ lines of shared functionality from PostgreSQL and SQLite dialects to base class
• Added abstract supportsDistinctOn property for provider-specific capabilities
• Centralized DISTINCT handling logic with proper error messaging
• Better separation of concerns between dialect-specific and common functionality

3. Provider-Specific Implementations

• PostgreSQL: supportsDistinctOn = true, maintains full DISTINCT ON functionality
• SQLite: supportsDistinctOn = false, throws clear error message when DISTINCT is attempted

4. Comprehensive Test Coverage (find.test.ts)

• Added dedicated test suite for DISTINCT functionality
• Proper provider-specific testing (SQLite expects error, PostgreSQL expects success)
• Tests various DISTINCT scenarios: single field, multiple fields, with relations, with select/include

5. Language Validation (datamodel-validator.ts)

• Minor change: More specific SQLite list type error message

6. Operation Cleanup (operations/base.ts)

• Removed 110+ lines of duplicate/dead code
• Better consolidation of shared functionality

Code Quality Assessment

✅ Strengths

1. Excellent Architecture: The refactoring to move shared functionality to the base class is a significant improvement
2. Type Safety: Conditional types properly prevent invalid operations at compile time
3. Clear Error Messages: Users get descriptive errors when attempting unsupported operations
4. Comprehensive Testing: Good coverage of various DISTINCT scenarios with provider-specific expectations
5. Clean Abstractions: The supportsDistinctOn property provides a clean way to handle provider differences

⚠️ Minor Concerns

1. Hardcoded Provider Check: The error throwing logic in base class checks this.schema.provider.type directly. Consider if this pattern should be more extensible.

if (this.supportsDistinctOn) {
    result = result.distinctOn(distinct.map((f) => sql.ref(`${model}.${f}`)));
} else {
    throw new QueryError(`"distinct" is not supported by "${this.schema.provider.type}" provider`);
}

2. Cast Safety: Some type assertions like (args as any).distinct could benefit from better typing, though this is likely due to the conditional type complexity.

Technical Review

SQLite DISTINCT Limitation

The PR correctly addresses a real limitation: SQLite doesn't support DISTINCT ON syntax that PostgreSQL provides. The solution appropriately:

• Prevents runtime errors by catching this at the API level
• Provides clear error messaging to developers
• Maintains type safety through conditional types

Code Consolidation

The movement of shared functionality from individual dialects to the base class is excellent. This reduces:

• Code duplication (194 deletions vs 160 additions)
• Maintenance overhead
• Potential inconsistencies between dialects

Test Strategy

The testing approach is solid:

• Provider-specific behavior testing
• Multiple DISTINCT scenarios covered
• Integration with other query features (relations, select, include)

Performance Considerations

✅ No negative performance impact identified

• Type-level checks don't affect runtime performance
• Error throwing is early and fast
• Consolidated code reduces bundle size

Security Review

✅ No security concerns

• No user input handling changes
• No new attack vectors introduced
• Maintains existing input validation

Recommendation

✅ APPROVE - This is a well-executed PR that:

1. Fixes a real issue: Prevents SQLite DISTINCT errors with clear messaging
2. Improves architecture: Significant code consolidation and better abstractions
3. Maintains type safety: Excellent use of conditional types
4. Has good test coverage: Comprehensive testing of various scenarios
5. No breaking changes: Existing code continues to work, new restrictions are provider-appropriate

The refactoring to consolidate shared functionality in the base dialect class is particularly commendable - it's the kind of cleanup that makes future maintenance much easier.

---

[Copilot]

Pull Request Overview

This PR fixes a limitation with the SQLite provider by disallowing the distinct operation, which is not supported by the SQLite dialect.

Key changes:

• Added type-level constraints to prevent distinct from being available for SQLite providers
• Updated test cases to handle SQLite-specific behavior and verify proper error handling
• Refactored database query building logic to consolidate filter, sort, and pagination operations

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/runtime/test/client-api/find.test.ts	Added SQLite-specific test handling and extended test coverage for distinct operations
packages/runtime/src/client/crud/operations/base.ts	Refactored query building logic and removed duplicate code
packages/runtime/src/client/crud/dialects/sqlite.ts	Simplified relation query building by using consolidated helper method
packages/runtime/src/client/crud/dialects/postgresql.ts	Simplified relation query building by using consolidated helper method
packages/runtime/src/client/crud/dialects/base.ts	Added consolidated query building method and moved cursor filter logic
packages/runtime/src/client/crud-types.ts	Added type-level constraints to conditionally allow distinct based on provider support
packages/language/src/validators/datamodel-validator.ts	Fixed non-null assertion operator usage

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.}

[claude]

Claude finished @ymc9's task —— View job

---

PR Review: SQLite DISTINCT Fix

Reviewing this pull request that disallows DISTINCT operations for SQLite.

Tasks

• Read and analyze all 7 changed files
• Understand the SQLite DISTINCT limitation being addressed
• Review implementation approach and architecture changes
• Assess code quality and potential issues
• Check test coverage adequacy
• Provide comprehensive review feedback

---

Summary

This PR implements proper SQLite DISTINCT operation handling by disallowing it where not supported and refactoring the dialect architecture for better provider-specific feature support.

Key Changes

1. Type System Updates (crud-types.ts)

• Introduced ProviderSupportsDistinct<Schema> conditional type (lines 1267-1269)
• Made distinct field conditionally available in FindArgs based on provider support (lines 633-643)
• Clean provider-aware typing that prevents invalid operations at compile time

2. Architecture Refactoring (dialects/base.ts)

• Major improvement: Moved 100+ lines of shared functionality from individual dialect files to base class
• Added abstract supportsDistinctOn property for provider-specific capabilities (line 1156)
• Centralized DISTINCT handling logic with proper error messaging (lines 94-102)
• Added comprehensive buildFilterSortTake method consolidating filter, sort, pagination, and distinct logic
• Better separation of concerns between dialect-specific and common functionality

3. Provider-Specific Implementations

• PostgreSQL: supportsDistinctOn = true (line 309), maintains full DISTINCT ON functionality
• SQLite: supportsDistinctOn = false (lines 254-256), throws clear error message when DISTINCT is attempted

4. Comprehensive Test Coverage (find.test.ts)

• Added dedicated test suite for DISTINCT functionality (lines 243-309)
• Proper provider-specific testing: SQLite expects error (line 265), PostgreSQL expects success
• Tests various DISTINCT scenarios: single field, multiple fields, with relations, with select/include

5. Minor Language Validation Update (datamodel-validator.ts)

• Changed from model.baseModel.ref to model.baseModel.ref! (line 446)
• More specific non-null assertion, though the reviewer from Copilot suggests this may be unnecessary

6. Operation Cleanup (operations/base.ts)

• Removed 110+ lines of duplicate/dead code through consolidation
• Better utilization of centralized functionality

Code Quality Assessment

✅ Strengths

1. Excellent Architecture: The refactoring to move shared functionality to the base class is a significant improvement
2. Type Safety: Conditional types properly prevent invalid operations at compile time
3. Clear Error Messages: Users get descriptive errors when attempting unsupported operations
4. Comprehensive Testing: Good coverage of various DISTINCT scenarios with provider-specific expectations
5. Clean Abstractions: The supportsDistinctOn property provides a clean way to handle provider differences
6. Code Consolidation: Significant reduction in duplicate code (194 deletions vs 160 additions)

⚠️ Minor Concerns

1. Hardcoded Provider Check: The error throwing logic in base class checks this.schema.provider.type directly (line 100):

   throw new QueryError(`"distinct" is not supported by "${this.schema.provider.type}" provider`);

   This is appropriate given the context, though extensibility could be enhanced.

2. Non-null Assertion: As noted by the Copilot reviewer, line 446 in datamodel-validator.ts adds a non-null assertion that may be unnecessary if the invariant at line 443 properly narrows the type.

3. Type Casting: Some type assertions like (args as any).distinct could benefit from better typing, though this is likely due to the conditional type complexity.

Technical Review

SQLite DISTINCT Limitation

The PR correctly addresses a real limitation: SQLite doesn't support PostgreSQL's DISTINCT ON syntax. The solution appropriately:

• Prevents runtime errors by catching this at the API level
• Provides clear error messaging to developers
• Maintains type safety through conditional types

Code Consolidation

The movement of shared functionality from individual dialects to the base class is excellent:

• Reduces code duplication significantly
• Improves maintainability
• Reduces potential inconsistencies between dialects
• Creates cleaner separation of concerns

Test Strategy

The testing approach is solid:

• Provider-specific behavior testing with appropriate expectations
• Multiple DISTINCT scenarios covered (single field, multiple fields, with relations)
• Integration with other query features (select, include, orderBy, pagination)

Performance & Security

✅ No negative performance impact identified

• Type-level checks don't affect runtime performance
• Error throwing is early and efficient
• Consolidated code reduces bundle size

✅ No security concerns

• No user input handling changes
• No new attack vectors introduced
• Maintains existing input validation patterns

Recommendation

✅ APPROVE - This is a well-executed PR that:

1. Fixes a real issue: Prevents SQLite DISTINCT errors with clear messaging
2. Improves architecture: Significant code consolidation and better abstractions
3. Maintains type safety: Excellent use of conditional types
4. Has comprehensive test coverage: Tests various scenarios appropriately
5. No breaking changes: Existing code continues to work, new restrictions are provider-appropriate

The refactoring to consolidate shared functionality in the base dialect class is particularly commendable - it's the kind of cleanup that significantly improves maintainability and reduces technical debt.

Minor Suggestions (Non-blocking)

• Consider removing the non-null assertion in datamodel-validator.ts:446 if the invariant properly narrows the type
• The architecture is extensible enough that future providers can easily declare their DISTINCT support capabilities

---

[claude]

Claude finished @ymc9's task —— View job

---

Claude finished reviewing this PR

---

PR Review: SQLite DISTINCT Fix

Comprehensive review of this pull request that disallows DISTINCT operations for SQLite.

Tasks

• Read and analyze all changed files
• Understand the SQLite DISTINCT limitation being addressed
• Review implementation approach and architecture changes
• Assess code quality and potential issues
• Check test coverage adequacy
• Provide comprehensive review feedback

---

Summary

This PR implements proper SQLite DISTINCT operation handling by disallowing it where not supported and refactoring the dialect architecture for better provider-specific feature support.

Key Changes

1. Type System Updates (crud-types.ts)

• Introduced ProviderSupportsDistinct<Schema> conditional type (lines 1267-1269)
• Made distinct field conditionally available in FindArgs based on provider support (lines 633-643)
• Clean provider-aware typing that prevents invalid operations at compile time

2. Architecture Refactoring (dialects/base.ts)

• Major improvement: Moved 100+ lines of shared functionality from individual dialect files to base class
• Added abstract supportsDistinctOn property for provider-specific capabilities (line 1156)
• Centralized DISTINCT handling logic with proper error messaging (lines 94-102)
• Added comprehensive buildFilterSortTake method consolidating filter, sort, pagination, and distinct logic
• Better separation of concerns between dialect-specific and common functionality

3. Provider-Specific Implementations

• PostgreSQL: supportsDistinctOn = true (line 309), maintains full DISTINCT ON functionality
• SQLite: supportsDistinctOn = false (lines 254-256), throws clear error message when DISTINCT is attempted

4. Comprehensive Test Coverage (find.test.ts)

• Added dedicated test suite for DISTINCT functionality (lines 243-309)
• Proper provider-specific testing: SQLite expects error (line 265), PostgreSQL expects success
• Tests various DISTINCT scenarios: single field, multiple fields, with relations, with select/include

5. Minor Language Validation Update (datamodel-validator.ts)

• Changed from model.baseModel.ref to model.baseModel.ref! (line 446)
• More specific non-null assertion, though the reviewer from Copilot suggests this may be unnecessary

6. Operation Cleanup (operations/base.ts)

• Removed 110+ lines of duplicate/dead code through consolidation
• Better utilization of centralized functionality

Code Quality Assessment

✅ Strengths

1. Excellent Architecture: The refactoring to move shared functionality to the base class is a significant improvement
2. Type Safety: Conditional types properly prevent invalid operations at compile time
3. Clear Error Messages: Users get descriptive errors when attempting unsupported operations
4. Comprehensive Testing: Good coverage of various DISTINCT scenarios with provider-specific expectations
5. Clean Abstractions: The supportsDistinctOn property provides a clean way to handle provider differences
6. Code Consolidation: Significant reduction in duplicate code (194 deletions vs 160 additions)

⚠️ Minor Concerns

1. Hardcoded Provider Check: The error throwing logic in base class checks this.schema.provider.type directly (line 100):

   throw new QueryError(`"distinct" is not supported by "${this.schema.provider.type}" provider`);

   This is appropriate given the context, though extensibility could be enhanced.

2. Non-null Assertion: As noted by the Copilot reviewer, line 446 in datamodel-validator.ts adds a non-null assertion that may be unnecessary if the invariant at line 443 properly narrows the type.

3. Type Casting: Some type assertions like (args as any).distinct could benefit from better typing, though this is likely due to the conditional type complexity.

Technical Review

SQLite DISTINCT Limitation

The PR correctly addresses a real limitation: SQLite doesn't support PostgreSQL's DISTINCT ON syntax. The solution appropriately:

• Prevents runtime errors by catching this at the API level
• Provides clear error messaging to developers
• Maintains type safety through conditional types

Code Consolidation

The movement of shared functionality from individual dialects to the base class is excellent:

• Reduces code duplication significantly
• Improves maintainability
• Reduces potential inconsistencies between dialects
• Creates cleaner separation of concerns

Test Strategy

The testing approach is solid:

• Provider-specific behavior testing with appropriate expectations
• Multiple DISTINCT scenarios covered (single field, multiple fields, with relations)
• Integration with other query features (select, include, orderBy, pagination)

Performance & Security

✅ No negative performance impact identified

• Type-level checks don't affect runtime performance
• Error throwing is early and efficient
• Consolidated code reduces bundle size

✅ No security concerns

• No user input handling changes
• No new attack vectors introduced
• Maintains existing input validation patterns

Recommendation

✅ APPROVE - This is a well-executed PR that:

1. Fixes a real issue: Prevents SQLite DISTINCT errors with clear messaging
2. Improves architecture: Significant code consolidation and better abstractions
3. Maintains type safety: Excellent use of conditional types
4. Has comprehensive test coverage: Tests various scenarios appropriately
5. No breaking changes: Existing code continues to work, new restrictions are provider-appropriate

The refactoring to consolidate shared functionality in the base dialect class is particularly commendable - it's the kind of cleanup that significantly improves maintainability and reduces technical debt.

Minor Suggestions (Non-blocking)

• Consider removing the non-null assertion in datamodel-validator.ts:446 if the invariant properly narrows the type
• The architecture is extensible enough that future providers can easily declare their DISTINCT support capabilities

---