fix: support using aggregations inside orderBy and having of groupBy

[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/agg-having-orderby

---

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: Support aggregations in orderBy and having of groupBy

Todo List

• Analyze core runtime changes
• Review type definitions and validation logic
• Check test coverage and quality
• Identify potential bugs or issues
• Assess performance and security considerations
• Provide comprehensive feedback

Summary

This PR successfully implements support for using aggregations inside orderBy and having clauses of groupBy operations. The implementation is well-structured and follows the existing codebase patterns.

🎯 Positive Aspects

Strong Type Safety

• Excellent TypeScript type definitions in crud-types.ts:1072-1087 with proper generic constraints
• WithAggregation parameter properly typed across OrderBy types
• Aggregation operators are well-defined in constants

Comprehensive Validation

• Robust input validation in validator.ts:1251-1312 with proper refinement rules
• Smart validation ensuring fields in having/orderBy are either in by clause or use aggregations only
• Good error messages for validation failures

Complete Test Coverage

• Excellent test coverage with practical test cases in group-by.test.ts:182-284
• Tests cover both having and orderBy with aggregations
• Edge cases like validation errors are properly tested

🔍 Issues Identified

1. Type Definition Inconsistency ⚠️

Location: crud-types.ts:1066-1068

type GroupByHaving<Schema extends SchemaDef, Model extends GetModels<Schema>> = Omit<
    WhereInput<Schema, Model, true, true>,
    '$expr'
>;

Issue: The having clause excludes $expr but validation logic in validator.ts:1334 still passes withAggregations=true to makeWhereSchema, which could allow invalid expressions to pass through.

Recommendation: Either allow $expr in having or ensure validation completely blocks it.

2. Validator Logic Gap ⚠️

Location: validator.ts:1288-1291

if (!this.onlyAggregationFields(val)) {
    return false;
}

Issue: The onlyAggregationFields method doesn't validate that aggregation fields are semantically valid for the field type (e.g., _avg on string fields should be invalid).

Recommendation: Add type-specific aggregation validation.

3. Potential SQL Injection Vector 🔴

Location: base.ts:509-521

.with(P.union(...AGGREGATE_OPERATORS), (op) => {
    const innerResult = this.buildStandardFilter(
        eb,
        type,
        value,
        aggregate(eb, lhs, op), // <-- Direct aggregation without validation
        getRhs,
        recurse,
        throwIfInvalid,
    );

Issue: The aggregate function processes operator strings without sufficient validation, potentially allowing injection if operators are user-controlled.

Recommendation: Add explicit operator whitelist validation before calling aggregate().

4. Missing Null Handling ⚠️

Location: group-by.ts:95-103

return fn(sql.ref(`$sub.${field}`)).as(`${key}.${field}`);

Issue: Aggregation functions don't handle null values consistently across different database backends.

Recommendation: Add explicit null handling or document null behavior.

🚀 Performance Considerations

Query Efficiency

• ✅ Good: Uses subqueries for proper filtering and pagination
• ✅ Good: Leverages database-native aggregation functions
• ⚠️ Concern: Complex having clauses with multiple aggregations could be expensive

Memory Usage

• ✅ Reasonable: Post-processing logic is efficient for typical group sizes
• ⚠️ Consider streaming for very large result sets

🔒 Security Assessment

Input Validation

• ✅ Good: Strong validation with Zod schemas
• ✅ Good: Proper sanitization of user inputs
• 🔴 Risk: Potential SQL injection in aggregation operators (see issue getting more policy test cases to pass #3 above)

Authorization

• ✅ Good: Leverages existing access control patterns
• ✅ Good: No bypassing of field-level permissions

📋 Minor Issues

1. Documentation: Missing JSDoc comments for new aggregation-related types
2. Error Messages: Some validation errors could be more specific about which aggregation failed
3. Code Organization: onlyAggregationFields method in validator.ts:1314-1331 could be extracted to a utility

🎯 Recommendations

High Priority

1. Fix SQL injection risk by adding operator validation in aggregate() function
2. Resolve type inconsistency between having clause definition and validation

Medium Priority

1. Add semantic validation for aggregation-field type compatibility
2. Improve null handling in aggregation functions
3. Add JSDoc documentation for new types

Low Priority

1. Consider performance optimizations for complex having clauses
2. Extract validation utilities for better code organization

✅ Overall Assessment

This is a high-quality implementation with excellent type safety, comprehensive validation, and good test coverage. The main security concern around potential SQL injection should be addressed, but overall the code follows good practices and integrates well with the existing architecture.

Recommendation: Approve after addressing the SQL injection risk

---

| Branch: fix/agg-having-orderby

[Copilot]

Pull Request Overview

This PR enhances the groupBy operation to support using aggregations inside the orderBy and having clauses, improving query flexibility for grouped data analysis.

• Added aggregation support in having and orderBy clauses for groupBy operations
• Enhanced type definitions to support aggregations in filter contexts
• Updated validation logic to properly handle aggregation operators

Reviewed Changes

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

Show a summary per file

File	Description
packages/runtime/test/schemas/basic/schema.zmodel	Added optional meta field to User model for testing
packages/runtime/test/schemas/basic/schema.ts	Updated schema definition to include meta field
packages/runtime/test/client-api/group-by.test.ts	Added comprehensive tests for aggregations in having and orderBy clauses
packages/runtime/src/client/query-utils.ts	Added aggregate utility function and moved extractFields
packages/runtime/src/client/crud/validator.ts	Enhanced validation to support aggregations in filters and having clauses
packages/runtime/src/client/crud/operations/group-by.ts	Updated groupBy implementation to properly handle aggregations
packages/runtime/src/client/crud/dialects/base.ts	Added aggregation support in filter building and order by logic
packages/runtime/src/client/crud-types.ts	Extended type definitions to support aggregations in filters
packages/runtime/src/client/contract.ts	Updated documentation for groupBy operation
packages/runtime/src/client/constants.ts	Added constants for logical combinators and aggregate operators
packages/runtime/src/client/client-impl.ts	Enabled post-processing for groupBy operations
packages/language/src/validators/datamodel-validator.ts	Updated error message terminology from "Array" to "List"

[claude]

Claude finished @ymc9's task —— View job

---

PR Review: Support aggregations in orderBy and having of groupBy

Todo List

• Analyze core runtime changes
• Review type definitions and validation logic
• Check test coverage and quality
• Identify potential bugs or issues
• Assess performance and security considerations
• Provide comprehensive feedback

Summary

This PR successfully implements support for using aggregations inside orderBy and having clauses of groupBy operations. The implementation is well-structured and follows the existing codebase patterns.

🎯 Positive Aspects

Strong Type Safety

• Excellent TypeScript type definitions in crud-types.ts:1072-1087 with proper generic constraints
• WithAggregation parameter properly typed across OrderBy types
• Aggregation operators are well-defined in constants

Comprehensive Validation

• Robust input validation in validator.ts:1251-1312 with proper refinement rules
• Smart validation ensuring fields in having/orderBy are either in by clause or use aggregations only
• Good error messages for validation failures

Complete Test Coverage

• Excellent test coverage with practical test cases in group-by.test.ts:182-284
• Tests cover both having and orderBy with aggregations
• Edge cases like validation errors are properly tested

🔍 Issues Identified

1. Critical Bug in Validator Logic 🔴

Location: validator.ts:1314-1331

private onlyAggregationFields(val: object) {
    let result = true;
    for (const [key, value] of Object.entries(val)) {
        if (AGGREGATE_OPERATORS.includes(key as any)) {
            continue;
        }
        if (LOGICAL_COMBINATORS.includes(key as any)) {
            if (enumerate(value).every((v) => this.onlyAggregationFields(v))) {
                continue;
            }
        }
        result = false;  // ⚠️ Sets false but continues loop
        break;           // This should return false immediately
    }
    return result;
}

Issue: Copilot correctly identified this bug. When a non-aggregation field is found, the function sets result = false but then hits break, which exits the loop and returns the result. However, the logic is flawed because it should return false immediately upon finding invalid fields.

Fix: Replace result = false; break; with return false; to immediately exit when invalid fields are found.

2. Type Definition Inconsistency ⚠️

Location: crud-types.ts:1065-1068

type GroupByHaving<Schema extends SchemaDef, Model extends GetModels<Schema>> = Omit<
    WhereInput<Schema, Model, true, true>,
    '$expr'
>;

Issue: The having clause excludes $expr but validation logic in validator.ts:1334 still passes withAggregations=true to makeWhereSchema, which could allow invalid expressions to pass through.

Recommendation: Either allow $expr in having or ensure validation completely blocks it.

3. Missing Null Handling ⚠️

Location: group-by.ts:95-103

return fn(sql.ref(`$sub.${field}`)).as(`${key}.${field}`);

Issue: Aggregation functions don't handle null values consistently across different database backends.

Recommendation: Add explicit null handling or document null behavior.

🚀 Performance Considerations

Query Efficiency

• ✅ Good: Uses subqueries for proper filtering and pagination
• ✅ Good: Leverages database-native aggregation functions
• ⚠️ Concern: Complex having clauses with multiple aggregations could be expensive

Memory Usage

• ✅ Reasonable: Post-processing logic is efficient for typical group sizes
• ⚠️ Consider streaming for very large result sets

🔒 Security Assessment

Input Validation

• ✅ Good: Strong validation with Zod schemas
• ✅ Good: Proper sanitization of user inputs
• ✅ Good: Aggregation operators are defined as constants and validated

Authorization

• ✅ Good: Leverages existing access control patterns
• ✅ Good: No bypassing of field-level permissions

📋 Minor Issues

1. Documentation: Missing JSDoc comments for new aggregation-related types
2. Error Messages: Some validation errors could be more specific about which aggregation failed
3. Code Organization: onlyAggregationFields method could be extracted to a utility

🎯 Recommendations

High Priority

1. Fix critical validator bug in onlyAggregationFields method - this could allow invalid queries through
2. Resolve type inconsistency between having clause definition and validation

Medium Priority

1. Add semantic validation for aggregation-field type compatibility (e.g., prevent _avg on string fields)
2. Improve null handling in aggregation functions
3. Add JSDoc documentation for new types

Low Priority

1. Consider performance optimizations for complex having clauses
2. Extract validation utilities for better code organization

✅ Overall Assessment

This is a high-quality implementation with excellent type safety, comprehensive validation, and good test coverage. The main issue is the validator bug which should be addressed before merging, but overall the code follows good practices and integrates well with the existing architecture.

Recommendation: Fix the critical validator bug, then approve

---

Branch: fix/agg-having-orderby | Job Run

[claude]

Claude finished @ymc9's task —— View job

---

PR Review: Support aggregations in orderBy and having of groupBy

Todo List

• Analyze core runtime changes
• Review type definitions and validation logic
• Check test coverage and quality
• Identify potential bugs or issues
• Assess performance and security considerations
• Provide comprehensive feedback

Summary

This PR successfully implements support for using aggregations inside orderBy and having clauses of groupBy operations. The implementation is well-structured and follows the existing codebase patterns.

🎯 Positive Aspects

Strong Type Safety

• Excellent TypeScript type definitions in crud-types.ts:1072-1087 with proper generic constraints
• WithAggregation parameter properly typed across OrderBy types
• Aggregation operators are well-defined in constants

Comprehensive Validation

• Robust input validation in validator.ts:1251-1312 with proper refinement rules
• Smart validation ensuring fields in having/orderBy are either in by clause or use aggregations only
• Good error messages for validation failures

Complete Test Coverage

• Excellent test coverage with practical test cases in group-by.test.ts:182-284
• Tests cover both having and orderBy with aggregations
• Edge cases like validation errors are properly tested

🔍 Issues Identified

1. Critical Bug in Validator Logic 🔴

Location: validator.ts:1314-1331

private onlyAggregationFields(val: object) {
    let result = true;
    for (const [key, value] of Object.entries(val)) {
        if (AGGREGATE_OPERATORS.includes(key as any)) {
            continue;
        }
        if (LOGICAL_COMBINATORS.includes(key as any)) {
            if (enumerate(value).every((v) => this.onlyAggregationFields(v))) {
                continue;
            }
        }
        result = false;  // ⚠️ Sets false but continues loop
        break;           // This should return false immediately
    }
    return result;
}

Issue: Copilot correctly identified this bug. When a non-aggregation field is found, the function sets result = false but then hits break, which exits the loop and returns the result. However, the logic is flawed because it should return false immediately upon finding invalid fields.

Fix: Replace result = false; break; with return false; to immediately exit when invalid fields are found.

2. Type Definition Inconsistency ⚠️

Location: crud-types.ts:1065-1068

type GroupByHaving<Schema extends SchemaDef, Model extends GetModels<Schema>> = Omit<
    WhereInput<Schema, Model, true, true>,
    '$expr'
>;

Issue: The having clause excludes $expr but validation logic in validator.ts:1334 still passes withAggregations=true to makeWhereSchema, which could allow invalid expressions to pass through.

Recommendation: Either allow $expr in having or ensure validation completely blocks it.

3. Missing Null Handling ⚠️

Location: group-by.ts:95-103

return fn(sql.ref(`$sub.${field}`)).as(`${key}.${field}`);

Issue: Aggregation functions don't handle null values consistently across different database backends.

Recommendation: Add explicit null handling or document null behavior.

🚀 Performance Considerations

Query Efficiency

• ✅ Good: Uses subqueries for proper filtering and pagination
• ✅ Good: Leverages database-native aggregation functions
• ⚠️ Concern: Complex having clauses with multiple aggregations could be expensive

Memory Usage

• ✅ Reasonable: Post-processing logic is efficient for typical group sizes
• ⚠️ Consider streaming for very large result sets

🔒 Security Assessment

Input Validation

• ✅ Good: Strong validation with Zod schemas
• ✅ Good: Proper sanitization of user inputs
• ✅ Good: Aggregation operators are defined as constants and validated

Authorization

• ✅ Good: Leverages existing access control patterns
• ✅ Good: No bypassing of field-level permissions

📋 Minor Issues

1. Documentation: Missing JSDoc comments for new aggregation-related types
2. Error Messages: Some validation errors could be more specific about which aggregation failed
3. Code Organization: onlyAggregationFields method could be extracted to a utility

🎯 Recommendations

High Priority

1. Fix critical validator bug in onlyAggregationFields method - this could allow invalid queries through
2. Resolve type inconsistency between having clause definition and validation

Medium Priority

1. Add semantic validation for aggregation-field type compatibility (e.g., prevent _avg on string fields)
2. Improve null handling in aggregation functions
3. Add JSDoc documentation for new types

Low Priority

1. Consider performance optimizations for complex having clauses
2. Extract validation utilities for better code organization

✅ Overall Assessment

This is a high-quality implementation with excellent type safety, comprehensive validation, and good test coverage. The main issue is the validator bug which should be addressed before merging, but overall the code follows good practices and integrates well with the existing architecture.

Recommendation: Fix the critical validator bug, then approve

---

Branch: fix/agg-having-orderby | Job Run