Skip to content

refactor: refactor core to simplify the code structure and API - #786

Merged
ahmad-moussawi merged 8 commits into
mainfrom
refactor_core
Oct 14, 2026
Merged

ahmad-moussawi merged 8 commits into
mainfrom
refactor_core

Conversation

@ahmad-moussawi

Copy link
Copy Markdown
Contributor

This pull request primarily refactors and improves the handling of condition clauses and their tests in the query builder. The most significant changes include simplifying generic types in condition classes, enhancing test coverage for complex "having" and "where" clause interactions, and cleaning up unused or redundant code.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Callback subqueries lose parent-variable resolution, and retained clause APIs contain cloning and replacement regressions.

4 open findings
What changed in this PR

Refactors query condition storage and compilation around dedicated condition groups while simplifying clause types and APIs.

Changes:

  • Separates where, having, and join clauses into dedicated containers.
  • Replaces reflective condition compilation with type-based dispatch.
  • Expands tests for grouped and interleaved conditions.
File Description
SqlKata.Execution/​XQuery.cs Uses centralized clause copying.
SqlKata.Execution/​QueryFactory.cs Copies dedicated clause containers.
QueryBuilder/​Query.Where.cs Adds Query-specific where API.
QueryBuilder/​Query.Join.cs Stores joins directly.
QueryBuilder/​Query.Having.cs Delegates having operations to condition groups.
QueryBuilder/​Query.From.cs Extracts Query-specific from API.
QueryBuilder/​Query.cs Introduces dedicated clause storage and cloning.
QueryBuilder/​Query.Aggregate.cs Modernizes collection construction.
QueryBuilder/​Join.cs Reworks joins around dedicated conditions and sources.
QueryBuilder/​IConditional.cs Defines the conditional-builder contract.
QueryBuilder/​IClauseContainer.cs Defines the clause-container contract.
QueryBuilder/​Extensions/​ConditionExtensions.cs Centralizes condition-building operations.
QueryBuilder/​ConditionGroup.cs Adds reusable condition groups.
QueryBuilder/​Compilers/​ConditionsCompilerProvider.cs Removes reflection-based dispatch.
QueryBuilder/​Compilers/​Compiler.cs Compiles dedicated join storage.
QueryBuilder/​Compilers/​Compiler.Conditions.cs Uses pattern-based condition compilation.
QueryBuilder/​Clauses/​ConditionClause.cs Simplifies generic condition types.
QueryBuilder/​BaseQuery.cs Removes the generic base query abstraction.
QueryBuilder/​Base.Where.cs Removes generic-base condition methods.
QueryBuilder.Tests/​WhereTests.cs Tests condition groups and flag interactions.
QueryBuilder.Tests/​OperatorWhitelistTests.cs Tightens expected exception types.
QueryBuilder.Tests/​Infrastructure/​TestCompiler.cs Removes reflection-testing helpers.
QueryBuilder.Tests/​GeneralTests.cs Removes obsolete reflection-cache tests.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread QueryBuilder/Join.cs Outdated
Comment thread QueryBuilder/Extensions/ConditionExtensions.cs
Comment thread QueryBuilder/Query.cs
Comment thread QueryBuilder/Query.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Clause metadata, engine-scoped replacement, and derived-query cloning contain behavioral regressions.

1 open finding
4 resolved since last review
Previously missed (3)

In code that hasn't changed since last review

Medium severity HAVING clauses are incorrectly stamped as where components

QueryBuilder/​Query.Having.cs:10

All simple HAVING APIs delegate to extensions that add the clause as component "where". This makes the compatibility Clauses property non-round-trippable: reading a query's clauses and assigning them to another query routes HAVING clauses into Wheres at lines 68-74. Ensure clauses inserted through Havings are stamped as "having" centrally.

Medium severity Null engine matching affects all scoped WHERE clauses

QueryBuilder/​Query.cs:219

When engineCode is null, this predicate matches every scoped WHERE clause rather than only the generic clause. A query containing generic and engine-specific conditions will therefore throw from SingleOrDefault() (or remove the sole engine-specific clause) when replacing the generic condition.

This issue also appears on line 225 of the same file.

Medium severity Join replacement broad-matches and loses engine/component metadata

QueryBuilder/​Query.cs:234

This branch both broad-matches every join when engineCode is null and adds the replacement without stamping its Engine/Component. Consequently an engine-specific join added through AddOrReplaceComponent becomes generic, and generic replacement can throw when scoped joins coexist.

🧠 Review effort: Balanced

Comment thread QueryBuilder/Join.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unscoped clause clearing breaks multi-engine queries, and the compatibility clause property can silently lose or misclassify clauses.

4 open findings
1 resolved since last review

🧠 Review effort: Balanced


this.ClearComponent("aggregate")
.AddComponent("aggregate", new AggregateClause
AggregateClauses.Clear();
Comment thread QueryBuilder/Query.Insert.cs Outdated
Method = "insert";

ClearComponent("insert").AddComponent("insert", new InsertClause
InsertClauses.Clear();
Comment thread QueryBuilder/Query.Update.cs Outdated
Method = "update";

ClearComponent("update").AddComponent("update", new InsertClause
UpdateClauses.Clear();
Comment thread QueryBuilder/Query.cs Outdated
{
foreach (var clause in value)
{
switch (clause.Component)
@ahmad-moussawi
ahmad-moussawi merged commit 6be5967 into main Oct 14, 2026
2 checks passed
@ahmad-moussawi
ahmad-moussawi deleted the refactor_core branch October 14, 2026 09:12
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.

2 participants