feat: add querybuilder #320
Conversation
WalkthroughThe changes introduce a new Changes
Assessment against linked issues
Warning Rate limit exceeded@MartianGreed has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
MartianGreed
force-pushed
the
feat/query-builder
branch
2 times, most recently
from
a28e949
to
f9b283a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
packages/sdk/src/__tests__/queryBuilder.test.ts (1)
10-35: Expand test coverage for complex scenariosWhile the current example is good, consider adding tests for:
- Different condition types (gt, lt, contains, etc.)
- Array conditions
- Multiple conditions combined with AND/OR
- Deeply nested structures
Also, consider adding JSDoc comments to document the expected query structure and usage patterns:
/** * Example of building a complex query with multiple namespaces and conditions. * The resulting query structure follows the pattern: * { * namespace: { * entity: { * $: { * where: { field: { $operator: value } } * } * } * } * } */ it("should work with example", () => {packages/sdk/src/queryBuilder.ts (5)
9-17: Error handling for the callback functions innamespacemethod.Consider adding error handling around the callback
cbin thenamespacemethod to catch potential exceptions that might occur during namespace configuration. This can prevent unexpected crashes and make debugging easier.Example:
public namespace( name: string, cb: (ns: Namespace<T>) => void ): Namespace<T> { const ns = new Namespace(this, name); this.namespaces.set(name, ns); - cb(ns); + try { + cb(ns); + } catch (error) { + console.error(`Error configuring namespace ${name}:`, error); + } return ns; }
19-47: Refactor thebuildmethod for better readability and efficiency.The
buildmethod is quite complex due to deeply nested loops and object constructions. Refactoring it can improve readability and maintainability. Consider extracting parts of the logic into helper methods or simplifying the object merging.Example:
+private buildConstraints(entityObj: QueryEntity<T>): Record<string, any> { + const constraints: Record<string, Record<string, any>> = {}; + for (const [field, constraint] of entityObj.constraints) { + constraints[field] = { + [constraint.operator]: constraint.value, + }; + } + return constraints; +} public build(): QueryType<T> { const qt: Record< string, Record< string, { $: { where: Record<string, Record<string, any>> } } > > = {}; for (const [ns, namespace] of this.namespaces) { qt[ns] = {}; for (const [entity, entityObj] of namespace.entities) { - const constraints: Record<string, Record<string, any>> = {}; - for (const [field, constraint] of entityObj.constraints) { - constraints[field] = { - [`${constraint.operator}`]: constraint.value, - }; - } + const constraints = this.buildConstraints(entityObj); qt[ns][entity] = { $: { where: { ...(qt[ns]?.[entity]?.$?.where ?? {}), ...constraints, }, }, }; } } return qt as QueryType<T>; }
136-138: Unnecessary use of.toString()inoperatorgetter.Since the
Operatorenum values are strings, calling.toString()is redundant. You can returnthis._operatordirectly.Apply this diff:
get operator(): string { - return this._operator.toString(); + return this._operator; }
145-153: Consider extending theOperatorenum for additional query capabilities.Depending on the requirements, you might want to include additional operators like
$in,$nin,$exists, or$regexto support more complex queries.Example:
enum Operator { is = "$is", eq = "$eq", neq = "$neq", gt = "$gt", gte = "$gte", lt = "$lt", lte = "$lte", + in = "$in", + nin = "$nin", + exists = "$exists", + regex = "$regex", }And add corresponding methods in
QueryEntity:class QueryEntity<T extends SchemaType> { // Existing methods... + public in(field: string, values: any[]): QueryEntity<T> { + this.constraints.set(field, new Constraint(Operator.in, values)); + return this; + } + + public nin(field: string, values: any[]): QueryEntity<T> { + this.constraints.set(field, new Constraint(Operator.nin, values)); + return this; + } + + public exists(field: string, value: boolean): QueryEntity<T> { + this.constraints.set(field, new Constraint(Operator.exists, value)); + return this; + } + + public regex(field: string, pattern: string): QueryEntity<T> { + this.constraints.set(field, new Constraint(Operator.regex, pattern)); + return this; + } }
89-122: Potential duplication in constraint methods inQueryEntity.The methods
is,eq,neq,gt,gte,lt,lteare very similar. Consider refactoring them to reduce code duplication.Example:
+private addConstraint(field: string, operator: Operator, value: any): QueryEntity<T> { + this.constraints.set(field, new Constraint(operator, value)); + return this; +} public is(field: string, value: any): QueryEntity<T> { - this.constraints.set(field, new Constraint(Operator.is, value)); - return this; + return this.addConstraint(field, Operator.is, value); } // Repeat for other methods... public eq(field: string, value: any): QueryEntity<T> { - this.constraints.set(field, new Constraint(Operator.eq, value)); - return this; + return this.addConstraint(field, Operator.eq, value); } // And so on for neq, gt, gte, lt, lte
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/sdk/src/__tests__/queryBuilder.test.ts(1 hunks)packages/sdk/src/index.ts(1 hunks)packages/sdk/src/queryBuilder.ts(1 hunks)
🔇 Additional comments (4)
packages/sdk/src/__tests__/queryBuilder.test.ts (2)
1-2: LGTM! Imports are clean and appropriate.
1-36: Consider implementing a comprehensive test strategy
To ensure robust testing of the QueryBuilder feature:
- Create separate test suites for different aspects (basic operations, error handling, complex queries)
- Add integration tests with actual SDK usage
- Consider property-based testing for query structure validation
- Add performance tests for large query structures
This will help ensure the reliability of this core SDK feature.
Would you like assistance in generating a comprehensive test suite structure?
packages/sdk/src/index.ts (1)
12-12: LGTM! Verify documentation updates.
The export statement is correctly placed and aligns with the module's export pattern. Since this exposes new functionality to SDK users, ensure that:
- The SDK's public API documentation is updated
- Examples demonstrating the QueryBuilder usage are added
- The changelog reflects this addition
packages/sdk/src/queryBuilder.ts (1)
2-7: QueryBuilder class is well-structured and initialized properly.
The QueryBuilder class is correctly defined with a proper initialization of the namespaces map. The use of generics with T extends SchemaType ensures type safety throughout the query building process.
| it("should be implemented", () => { | ||
| const query = new QueryBuilder().build(); | ||
| expect(query).toStrictEqual({}); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance basic test coverage
The current test case "should be implemented" could be improved:
- Rename to something more descriptive like "should create empty query when no parameters are provided"
- Add test cases for edge cases and error scenarios
Consider adding these test cases:
it("should throw error when namespace name is empty", () => {
expect(() => new QueryBuilder().namespace("", () => {})).toThrow();
});
it("should throw error when entity name is empty", () => {
expect(() =>
new QueryBuilder().namespace("test", n => n.entity("", () => {}))
).toThrow();
});
packages/sdk/src/queryBuilder.ts
Outdated
| constructor( | ||
| private parent: Namespace<T>, | ||
| private name: string | ||
| ) { | ||
| this.constraints = new Map<string, Constraint<T>>(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused parameter name in QueryEntity constructor.
The name parameter in the QueryEntity constructor is not stored or used within the class. Store it as a property if it's intended for future use or remove it if unnecessary.
Apply this diff to store the name property:
class QueryEntity<T extends SchemaType> {
constraints: Map<string, Constraint<T>>;
+ private name: string;
constructor(
private parent: Namespace<T>,
- private name: string
+ name: string
) {
+ this.name = name;
this.constraints = new Map<string, Constraint<T>>();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor( | |
| private parent: Namespace<T>, | |
| private name: string | |
| ) { | |
| this.constraints = new Map<string, Constraint<T>>(); | |
| } | |
| constructor( | |
| private parent: Namespace<T>, | |
| name: string | |
| ) { | |
| this.name = name; | |
| this.constraints = new Map<string, Constraint<T>>(); | |
| } |
packages/sdk/src/queryBuilder.ts
Outdated
| class Constraint<T extends SchemaType> { | ||
| constructor( | ||
| private parent: QueryEntity<T>, | ||
| private _operator: Operator, | ||
| private _value: any | ||
| ) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused parameter parent in Constraint constructor.
The parent parameter in the Constraint constructor is not used within the class. Removing it simplifies the code and avoids potential confusion.
Apply this diff:
class Constraint<T extends SchemaType> {
constructor(
- private parent: QueryEntity<T>,
private _operator: Operator,
private _value: any
) {}
}Also, update the instantiation of Constraint objects in QueryEntity methods:
public is(field: string, value: any): QueryEntity<T> {
- this.constraints.set(field, new Constraint(this, Operator.is, value));
+ this.constraints.set(field, new Constraint(Operator.is, value));
return this;
}Committable suggestion skipped: line range outside the PR's diff.
packages/sdk/src/queryBuilder.ts
Outdated
| constructor( | ||
| private parent: QueryBuilder<T>, | ||
| private name: string | ||
| ) { | ||
| this.entities = new Map<string, QueryEntity<T>>(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused parameter name in Namespace constructor.
The name parameter in the Namespace constructor is not stored or used within the class. If the namespace name is needed later, consider storing it as a class property. Otherwise, remove it to avoid confusion.
Apply this diff to store the name property:
class Namespace<T extends SchemaType> {
entities: Map<string, QueryEntity<T>>;
+ private name: string;
constructor(
private parent: QueryBuilder<T>,
- private name: string
+ name: string
) {
+ this.name = name;
this.entities = new Map<string, QueryEntity<T>>();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor( | |
| private parent: QueryBuilder<T>, | |
| private name: string | |
| ) { | |
| this.entities = new Map<string, QueryEntity<T>>(); | |
| } | |
| constructor( | |
| private parent: QueryBuilder<T>, | |
| name: string | |
| ) { | |
| this.name = name; | |
| this.entities = new Map<string, QueryEntity<T>>(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
packages/sdk/src/__example__/index.ts (1)
Line range hint
1-156: Consider restructuring the examples for better clarityWhile the example demonstrates the QueryBuilder functionality, consider the following improvements:
- Split the examples into separate files for different use cases (e.g.,
basic-queries.ts,complex-queries.ts)- Move the schema definitions to a separate
schema.tsfile- Add examples demonstrating more QueryBuilder features like:
- Different comparison operators
- Combining conditions with AND/OR
- Pagination/limiting results
- Sorting options
This would make the examples more maintainable and educational.
Would you like me to help create a PR for these improvements?
packages/sdk/src/queryBuilder.ts (2)
28-34: Simplify complex type definitionConsider extracting the nested type definition to improve readability:
type QueryConstraint = Record<string, Record<string, any>>; type QueryStructure = Record<string, Record<string, { $: { where: QueryConstraint } }>>
152-160: Add documentation for operatorsThe Operator enum would benefit from JSDoc comments explaining the behavior of each operator:
enum Operator { + /** Exact match comparison */ is = "$is", + /** Equality comparison */ eq = "$eq", + /** Not equal comparison */ neq = "$neq", + /** Greater than comparison */ gt = "$gt", + /** Greater than or equal comparison */ gte = "$gte", + /** Less than comparison */ lt = "$lt", + /** Less than or equal comparison */ lte = "$lte", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/sdk/CHANGELOG.md(1 hunks)packages/sdk/src/__example__/index.ts(2 hunks)packages/sdk/src/__tests__/queryBuilder.test.ts(1 hunks)packages/sdk/src/queryBuilder.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/sdk/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/src/tests/queryBuilder.test.ts
🔇 Additional comments (2)
packages/sdk/src/__example__/index.ts (1)
4-4: LGTM: QueryBuilder import added correctly
The QueryBuilder import is appropriately placed alongside the existing init import.
packages/sdk/src/queryBuilder.ts (1)
3-8: Well-designed type-safe nested key access!
The NestedKeyOf type elegantly handles both direct and nested property access, ensuring type safety when accessing nested fields in the schema.
| const query = new QueryBuilder<MockSchemaType>().namespace("world", (n) => | ||
| n.entity("player", (e) => e.eq("name", "Alice")) | ||
| ); | ||
|
|
||
| db.subscribeEntityQuery(query.build(), (resp) => { | ||
| if (resp.error) { | ||
| console.error( | ||
| "Error querying todos and goals:", | ||
| resp.error.message | ||
| ); | ||
| return; | ||
| } | ||
| if (resp.data) { | ||
| console.log( | ||
| "Queried todos and goals:", | ||
| resp.data.map((a) => a.models.world) | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fix misleading error and log messages
The error and log messages seem to be copied from a different example (mentioning "todos and goals" while the code is actually querying players).
Apply this diff to fix the messages and improve response handling:
db.subscribeEntityQuery(query.build(), (resp) => {
if (resp.error) {
console.error(
- "Error querying todos and goals:",
+ "Error querying players:",
resp.error.message
);
return;
}
if (resp.data) {
console.log(
- "Queried todos and goals:",
+ "Queried players:",
- resp.data.map((a) => a.models.world)
+ resp.data.map((a) => a.models.world.player)
);
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const query = new QueryBuilder<MockSchemaType>().namespace("world", (n) => | |
| n.entity("player", (e) => e.eq("name", "Alice")) | |
| ); | |
| db.subscribeEntityQuery(query.build(), (resp) => { | |
| if (resp.error) { | |
| console.error( | |
| "Error querying todos and goals:", | |
| resp.error.message | |
| ); | |
| return; | |
| } | |
| if (resp.data) { | |
| console.log( | |
| "Queried todos and goals:", | |
| resp.data.map((a) => a.models.world) | |
| ); | |
| } | |
| }); | |
| const query = new QueryBuilder<MockSchemaType>().namespace("world", (n) => | |
| n.entity("player", (e) => e.eq("name", "Alice")) | |
| ); | |
| db.subscribeEntityQuery(query.build(), (resp) => { | |
| if (resp.error) { | |
| console.error( | |
| "Error querying players:", | |
| resp.error.message | |
| ); | |
| return; | |
| } | |
| if (resp.data) { | |
| console.log( | |
| "Queried players:", | |
| resp.data.map((a) => a.models.world.player) | |
| ); | |
| } | |
| }); |
| try { | ||
| const eq = new QueryBuilder<MockSchemaType>().namespace("world", (n) => | ||
| n | ||
| .entity("item", (e) => | ||
| e.eq("type", "sword").lt("durability", 5) | ||
| ) | ||
| .entity("game", (e) => e.eq("status", "completed")) | ||
| ); | ||
| const entities = await db.getEntities(eq.build(), (resp) => { | ||
| if (resp.error) { | ||
| console.error( | ||
| "Error querying todos and goals:", | ||
| "Error querying completed important todos:", | ||
| resp.error.message | ||
| ); | ||
| return; | ||
| } | ||
| if (resp.data) { | ||
| console.log( | ||
| "Queried todos and goals:", | ||
| resp.data.map((a) => a.models.world) | ||
| "Completed important todos:", | ||
| resp.data.map((a) => a.models) | ||
| ); | ||
| } | ||
| } | ||
| ); | ||
| // Example usage of getEntities with where clause | ||
| try { | ||
| const entities = await db.getEntities( | ||
| { | ||
| world: { | ||
| item: { | ||
| $: { | ||
| where: { | ||
| type: { $eq: "sword" }, | ||
| durability: { $lt: 5 }, | ||
| }, | ||
| }, | ||
| }, | ||
| game: { | ||
| $: { | ||
| where: { | ||
| status: { $eq: "completed" }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| (resp) => { | ||
| if (resp.error) { | ||
| console.error( | ||
| "Error querying completed important todos:", | ||
| resp.error.message | ||
| ); | ||
| return; | ||
| } | ||
| if (resp.data) { | ||
| console.log( | ||
| "Completed important todos:", | ||
| resp.data.map((a) => a.models) | ||
| ); | ||
| } | ||
| } | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Fix misleading messages and improve type safety
The error and log messages reference "todos" instead of the actual entities being queried (items and games). Additionally, the response handling could be more specific.
Apply these changes to improve the code:
try {
const eq = new QueryBuilder<MockSchemaType>().namespace("world", (n) =>
n
.entity("item", (e) =>
e.eq("type", "sword").lt("durability", 5)
)
.entity("game", (e) => e.eq("status", "completed"))
);
- const entities = await db.getEntities(eq.build(), (resp) => {
+ const entities: { world: { item: ItemModel[], game: GameModel[] } } = await db.getEntities(eq.build(), (resp) => {
if (resp.error) {
console.error(
- "Error querying completed important todos:",
+ "Error querying items and games:",
resp.error.message
);
return;
}
if (resp.data) {
console.log(
- "Completed important todos:",
+ "Query results:",
+ "Low durability items:",
+ resp.data.map((a) => a.models.world.item),
+ "Completed games:",
+ resp.data.map((a) => a.models.world.game)
- resp.data.map((a) => a.models)
);
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const eq = new QueryBuilder<MockSchemaType>().namespace("world", (n) => | |
| n | |
| .entity("item", (e) => | |
| e.eq("type", "sword").lt("durability", 5) | |
| ) | |
| .entity("game", (e) => e.eq("status", "completed")) | |
| ); | |
| const entities = await db.getEntities(eq.build(), (resp) => { | |
| if (resp.error) { | |
| console.error( | |
| "Error querying todos and goals:", | |
| "Error querying completed important todos:", | |
| resp.error.message | |
| ); | |
| return; | |
| } | |
| if (resp.data) { | |
| console.log( | |
| "Queried todos and goals:", | |
| resp.data.map((a) => a.models.world) | |
| "Completed important todos:", | |
| resp.data.map((a) => a.models) | |
| ); | |
| } | |
| } | |
| ); | |
| // Example usage of getEntities with where clause | |
| try { | |
| const entities = await db.getEntities( | |
| { | |
| world: { | |
| item: { | |
| $: { | |
| where: { | |
| type: { $eq: "sword" }, | |
| durability: { $lt: 5 }, | |
| }, | |
| }, | |
| }, | |
| game: { | |
| $: { | |
| where: { | |
| status: { $eq: "completed" }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| (resp) => { | |
| if (resp.error) { | |
| console.error( | |
| "Error querying completed important todos:", | |
| resp.error.message | |
| ); | |
| return; | |
| } | |
| if (resp.data) { | |
| console.log( | |
| "Completed important todos:", | |
| resp.data.map((a) => a.models) | |
| ); | |
| } | |
| } | |
| ); | |
| }); | |
| try { | |
| const eq = new QueryBuilder<MockSchemaType>().namespace("world", (n) => | |
| n | |
| .entity("item", (e) => | |
| e.eq("type", "sword").lt("durability", 5) | |
| ) | |
| .entity("game", (e) => e.eq("status", "completed")) | |
| ); | |
| const entities: { world: { item: ItemModel[], game: GameModel[] } } = await db.getEntities(eq.build(), (resp) => { | |
| if (resp.error) { | |
| console.error( | |
| "Error querying items and games:", | |
| resp.error.message | |
| ); | |
| return; | |
| } | |
| if (resp.data) { | |
| console.log( | |
| "Query results:", | |
| "Low durability items:", | |
| resp.data.map((a) => a.models.world.item), | |
| "Completed games:", | |
| resp.data.map((a) => a.models.world.game) | |
| ); | |
| } | |
| }); |
| const ns = new Namespace(this); | ||
| this.namespaces.set(name as string, ns); |
There was a problem hiding this comment.
Pass namespace name to constructor
The namespace name is not passed to the Namespace constructor, which was flagged in a previous review. This is needed for proper namespace hierarchy.
-const ns = new Namespace(this);
+const ns = new Namespace(this, name as string);Committable suggestion skipped: line range outside the PR's diff.
| class Constraint { | ||
| constructor( | ||
| private _operator: Operator, | ||
| private _value: any | ||
| ) {} | ||
|
|
||
| get operator(): string { | ||
| return this._operator.toString(); | ||
| } | ||
|
|
||
| get value(): any { | ||
| return this._value; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety for constraint values
The Constraint class accepts any value type. Consider adding generic type parameter for better type safety:
-class Constraint {
+class Constraint<T> {
constructor(
private _operator: Operator,
- private _value: any
+ private _value: T
) {}
get operator(): string {
return this._operator.toString();
}
- get value(): any {
+ get value(): T {
return this._value;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class Constraint { | |
| constructor( | |
| private _operator: Operator, | |
| private _value: any | |
| ) {} | |
| get operator(): string { | |
| return this._operator.toString(); | |
| } | |
| get value(): any { | |
| return this._value; | |
| } | |
| } | |
| class Constraint<T> { | |
| constructor( | |
| private _operator: Operator, | |
| private _value: T | |
| ) {} | |
| get operator(): string { | |
| return this._operator.toString(); | |
| } | |
| get value(): T { | |
| return this._value; | |
| } | |
| } |
packages/sdk/src/queryBuilder.ts
Outdated
| public is(field: string, value: any): QueryEntity<T> { | ||
| this.constraints.set(field, new Constraint(Operator.is, value)); | ||
| return this; | ||
| } | ||
|
|
||
| public eq(field: string, value: any): QueryEntity<T> { | ||
| this.constraints.set(field, new Constraint(Operator.eq, value)); | ||
| return this; | ||
| } | ||
|
|
||
| public neq(field: string, value: any): QueryEntity<T> { | ||
| this.constraints.set(field, new Constraint(Operator.neq, value)); | ||
| return this; | ||
| } | ||
|
|
||
| public gt(field: string, value: any): QueryEntity<T> { | ||
| this.constraints.set(field, new Constraint(Operator.gt, value)); | ||
| return this; | ||
| } | ||
|
|
||
| public gte(field: string, value: any): QueryEntity<T> { | ||
| this.constraints.set(field, new Constraint(Operator.gte, value)); | ||
| return this; | ||
| } | ||
|
|
||
| public lt(field: string, value: any): QueryEntity<T> { | ||
| this.constraints.set(field, new Constraint(Operator.lt, value)); | ||
| return this; | ||
| } | ||
|
|
||
| public lte(field: string, value: any): QueryEntity<T> { | ||
| this.constraints.set(field, new Constraint(Operator.lte, value)); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in operator methods
The operator methods (is, eq, neq, gt, gte, lt, lte) all follow the same pattern. Consider refactoring to reduce duplication:
+private addConstraint(field: string, operator: Operator, value: any): QueryEntity<T> {
+ this.constraints.set(field, new Constraint(operator, value));
+ return this;
+}
public is(field: string, value: any): QueryEntity<T> {
- this.constraints.set(field, new Constraint(Operator.is, value));
- return this;
+ return this.addConstraint(field, Operator.is, value);
}
public eq(field: string, value: any): QueryEntity<T> {
- this.constraints.set(field, new Constraint(Operator.eq, value));
- return this;
+ return this.addConstraint(field, Operator.eq, value);
}
// Apply similar changes to other operator methodsCommittable suggestion skipped: line range outside the PR's diff.
MartianGreed
force-pushed
the
feat/query-builder
branch
from
2438663
to
6a98b79
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/sdk/src/__example__/index.ts (1)
114-156: Consider expanding examples for common query patternsWhile the current examples effectively demonstrate basic and advanced QueryBuilder usage, consider adding examples for other common query patterns:
- Sorting and pagination
- Complex logical conditions (AND/OR combinations)
- Aggregation queries (if supported)
- Error handling best practices
This would make the example file more valuable as a reference for SDK users.
packages/sdk/src/queryBuilder.ts (1)
27-55: Consider improving type safety in build methodThe build method uses generic Record types which could be made more specific.
Consider using a more specific type for the query structure:
interface QueryStructure<T> { [namespace: string]: { [entity: string]: { $: { where: Record<string, Record<string, any>> } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/sdk/CHANGELOG.md(1 hunks)packages/sdk/src/__example__/index.ts(2 hunks)packages/sdk/src/__tests__/queryBuilder.test.ts(1 hunks)packages/sdk/src/queryBuilder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/sdk/CHANGELOG.md
- packages/sdk/src/tests/queryBuilder.test.ts
🔇 Additional comments (8)
packages/sdk/src/__example__/index.ts (5)
4-4: LGTM!
The import statement correctly includes the new QueryBuilder alongside the existing init import, aligning with the PR's objective of introducing the query builder pattern.
114-116: LGTM! Clean QueryBuilder implementation
The QueryBuilder usage demonstrates a clear and type-safe way to construct queries, which is a significant improvement over the previous query pattern. The namespace and entity structure is well-defined and easy to understand.
118-132: Previous review comment about misleading messages is still valid
The error and log messages still reference "todos and goals" instead of "players".
135-141: LGTM! Excellent demonstration of advanced QueryBuilder features
The code effectively demonstrates advanced QueryBuilder capabilities:
- Multiple entity queries in a single builder
- Chained conditions (eq + lt)
- Clear and readable query structure
142-156: Previous review comment about misleading messages and type safety is still valid
The error and log messages still reference "todos" instead of the actual entities being queried (items and games).
packages/sdk/src/queryBuilder.ts (3)
3-8: LGTM! Well-structured type definition for nested object paths.
The NestedKeyOf type effectively handles nested object paths with proper type safety.
154-162: LGTM! Well-defined operator enum
The operators are well-defined and follow common query operator conventions with proper $ prefix.
75-77:
Fix namespace hierarchy implementation
The current implementation creates sibling namespaces instead of nested ones.
Apply this diff:
public namespace(ns: string, cb: (ns: Namespace<T>) => void): Namespace<T> {
- return this.parent.namespace(ns, cb);
+ const nestedNamespace = new Namespace(this.parent, `${this.name}.${ns}`);
+ this.entities.set(ns, nestedNamespace);
+ cb(nestedNamespace);
+ return nestedNamespace;
}Likely invalid or redundant comment.
| public is(field: string, value: any): QueryEntity<T> { | ||
| return this.addConstraint(field, value, Operator.is); | ||
| } | ||
|
|
||
| public eq(field: string, value: any): QueryEntity<T> { | ||
| return this.addConstraint(field, value, Operator.eq); | ||
| } | ||
|
|
||
| public neq(field: string, value: any): QueryEntity<T> { | ||
| return this.addConstraint(field, value, Operator.neq); | ||
| } | ||
|
|
||
| public gt(field: string, value: any): QueryEntity<T> { | ||
| return this.addConstraint(field, value, Operator.gt); | ||
| } | ||
|
|
||
| public gte(field: string, value: any): QueryEntity<T> { | ||
| return this.addConstraint(field, value, Operator.gte); | ||
| } | ||
|
|
||
| public lt(field: string, value: any): QueryEntity<T> { | ||
| return this.addConstraint(field, value, Operator.lt); | ||
| } | ||
|
|
||
| public lte(field: string, value: any): QueryEntity<T> { | ||
| return this.addConstraint(field, value, Operator.lte); | ||
| } |
There was a problem hiding this comment.
Add input validation for constraint values
The constraint methods accept any value without validation, which could lead to injection vulnerabilities.
Consider adding:
- Type validation for values based on field types
- Sanitization for string values
- Range validation for numeric values
Example implementation:
private validateConstraintValue(field: string, value: any): void {
// Add validation based on field type
if (typeof value === 'string') {
// Add string sanitization
if (value.length > MAX_STRING_LENGTH) {
throw new Error(`Value for field "${field}" exceeds maximum length`);
}
}
// Add more validation as needed
}Then update the addConstraint method:
private addConstraint(field: string, value: any, op: Operator): QueryEntity<T> {
+ this.validateConstraintValue(field, value);
this.constraints.set(field, new Constraint(op, value));
return this;
}| class Namespace<T extends SchemaType> { | ||
| entities: Map<string, QueryEntity<T>>; | ||
|
|
||
| constructor(private parent: QueryBuilder<T>) { | ||
| this.entities = new Map<string, QueryEntity<T>>(); | ||
| } |
There was a problem hiding this comment.
Add name property to Namespace class
The name property is missing but required for proper namespace hierarchy.
Apply this diff:
class Namespace<T extends SchemaType> {
entities: Map<string, QueryEntity<T>>;
+ private name: string;
- constructor(private parent: QueryBuilder<T>) {
+ constructor(private parent: QueryBuilder<T>, name: string) {
+ this.name = name;
this.entities = new Map<string, QueryEntity<T>>();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class Namespace<T extends SchemaType> { | |
| entities: Map<string, QueryEntity<T>>; | |
| constructor(private parent: QueryBuilder<T>) { | |
| this.entities = new Map<string, QueryEntity<T>>(); | |
| } | |
| class Namespace<T extends SchemaType> { | |
| entities: Map<string, QueryEntity<T>>; | |
| private name: string; | |
| constructor(private parent: QueryBuilder<T>, name: string) { | |
| this.name = name; | |
| this.entities = new Map<string, QueryEntity<T>>(); | |
| } |
feat: add basic querybuilder
MartianGreed
force-pushed
the
feat/query-builder
branch
from
6a98b79
to
738beda
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/sdk/src/__example__/index.ts (1)
Line range hint
1-162: Consider enhancing the example file's educational valueWhile this example effectively demonstrates the QueryBuilder functionality, consider these improvements to make it more educational:
- Add a section header comment explaining the purpose of each example
- Include comments showing the equivalent raw query structure that the QueryBuilder replaces
- Add examples of other QueryBuilder features mentioned in the PR objectives
Example structure:
// Basic QueryBuilder Example // Demonstrates simple entity filtering // Raw query equivalent: // { world: { player: { name: { $eq: "Alice" } } } } const simpleQuery = new QueryBuilder()... // Advanced QueryBuilder Example // Demonstrates multiple entity queries with conditions // Raw query equivalent: // { world: { // item: { type: { $eq: "sword" }, durability: { $lt: 5 } }, // game: { status: { $eq: "completed" } } // } } const advancedQuery = new QueryBuilder()...packages/sdk/src/queryBuilder.ts (1)
10-56: Add JSDoc documentation for public API.The QueryBuilder class and its public methods lack documentation:
+/** + * A builder for creating type-safe subscription queries. + * @template T The schema type that defines the structure of available entities + */ export class QueryBuilder<T extends SchemaType> { // ... + /** + * Creates a new namespace for grouping related entities. + * @param name The name of the namespace + * @param cb A callback function for configuring the namespace + * @returns The created namespace + */ public namespace( name: keyof T, cb: (ns: Namespace<T>) => void ): Namespace<T> { // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/sdk/CHANGELOG.md(1 hunks)packages/sdk/src/__example__/index.ts(2 hunks)packages/sdk/src/__tests__/queryBuilder.test.ts(1 hunks)packages/sdk/src/index.ts(1 hunks)packages/sdk/src/queryBuilder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/sdk/CHANGELOG.md
- packages/sdk/src/tests/queryBuilder.test.ts
- packages/sdk/src/index.ts
🔇 Additional comments (5)
packages/sdk/src/__example__/index.ts (3)
4-4: LGTM!
The import statement correctly includes the new QueryBuilder class alongside the existing init import.
114-132: QueryBuilder implementation looks good, but error messages need updating
The QueryBuilder implementation correctly demonstrates the new query pattern for filtering players. However, the error messages still need to be updated.
The previous review comment about fixing the misleading messages is still valid. Please refer to the existing comment for the suggested changes to error messages and response handling.
134-156: Good QueryBuilder demonstration, needs message updates and type safety
The implementation effectively demonstrates advanced QueryBuilder features:
- Multiple entity queries (items and games)
- Condition chaining (.eq() and .lt())
- Namespace and entity nesting
However, there are still some issues to address:
The previous review comment about fixing misleading messages and improving type safety is still valid. Please refer to the existing comment for the suggested improvements.
Additionally, consider adding a code comment explaining this complex query pattern for documentation purposes:
// Example: Query low durability swords and completed games
// This demonstrates how to combine multiple entity queries within a namespacepackages/sdk/src/queryBuilder.ts (2)
1-8: LGTM! Well-designed type definition for nested object keys.
The NestedKeyOf type correctly implements recursive type generation for nested object properties, enabling type-safe dot-notation paths.
84-89: 🛠️ Refactor suggestion
Improve type safety in QueryEntity.
The constraints Map uses a non-generic Constraint type:
class QueryEntity<T extends SchemaType> {
- constraints: Map<string, Constraint>;
+ constraints: Map<string, Constraint<unknown>>;Likely invalid or redundant comment.
| private addConstraint( | ||
| field: string, | ||
| value: any, | ||
| op: Operator | ||
| ): QueryEntity<T> { | ||
| this.constraints.set(field, new Constraint(op, value)); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Add input validation in addConstraint method.
The method accepts any value without validation:
private addConstraint(
field: string,
value: any,
op: Operator
): QueryEntity<T> {
+ if (value === undefined || value === null) {
+ throw new Error(`Invalid value for field "${field}": value cannot be null or undefined`);
+ }
+ if (typeof field !== 'string' || field.trim() === '') {
+ throw new Error('Field name must be a non-empty string');
+ }
this.constraints.set(field, new Constraint(op, value));
return this;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private addConstraint( | |
| field: string, | |
| value: any, | |
| op: Operator | |
| ): QueryEntity<T> { | |
| this.constraints.set(field, new Constraint(op, value)); | |
| return this; | |
| } | |
| private addConstraint( | |
| field: string, | |
| value: any, | |
| op: Operator | |
| ): QueryEntity<T> { | |
| if (value === undefined || value === null) { | |
| throw new Error(`Invalid value for field "${field}": value cannot be null or undefined`); | |
| } | |
| if (typeof field !== 'string' || field.trim() === '') { | |
| throw new Error('Field name must be a non-empty string'); | |
| } | |
| this.constraints.set(field, new Constraint(op, value)); | |
| return this; | |
| } |
| public build(): SubscriptionQueryType<T> { | ||
| const qt: Record< | ||
| string, | ||
| Record< | ||
| string, | ||
| { $: { where: Record<string, Record<string, any>> } } | ||
| > | ||
| > = {}; | ||
| for (const [ns, namespace] of this.namespaces) { | ||
| qt[ns] = {}; | ||
| for (const [entity, entityObj] of namespace.entities) { | ||
| const constraints: Record<string, Record<string, any>> = {}; | ||
| for (const [field, constraint] of entityObj.constraints) { | ||
| constraints[field] = { | ||
| [`${constraint.operator}`]: constraint.value, | ||
| }; | ||
| } | ||
| qt[ns][entity] = { | ||
| $: { | ||
| where: { | ||
| ...(qt[ns]?.[entity]?.$?.where ?? {}), | ||
| ...constraints, | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
| } | ||
| return qt as SubscriptionQueryType<T>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in build method.
The build method uses loose typing with Record<string, any>:
public build(): SubscriptionQueryType<T> {
- const qt: Record<
- string,
- Record<
- string,
- { $: { where: Record<string, Record<string, any>> } }
- >
- > = {};
+ const qt: {
+ [K in keyof T]?: {
+ [E in NestedKeyOf<T[K]>]?: {
+ $: {
+ where: Record<string, Record<string, unknown>>
+ }
+ }
+ }
+ } = {};
// ... rest of the method
}Committable suggestion skipped: line range outside the PR's diff.
| public namespace(ns: string, cb: (ns: Namespace<T>) => void): Namespace<T> { | ||
| return this.parent.namespace(ns, cb); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve namespace hierarchy implementation.
The current implementation creates sibling namespaces instead of nested ones:
public namespace(ns: string, cb: (ns: Namespace<T>) => void): Namespace<T> {
- return this.parent.namespace(ns, cb);
+ const fullName = `${this.name}.${ns}`;
+ return this.parent.namespace(fullName as keyof T, cb);
}Committable suggestion skipped: line range outside the PR's diff.
| public namespace( | ||
| name: keyof T, | ||
| cb: (ns: Namespace<T>) => void | ||
| ): Namespace<T> { | ||
| const ns = new Namespace(this); | ||
| this.namespaces.set(name as string, ns); | ||
| cb(ns); | ||
| return ns; | ||
| } |
There was a problem hiding this comment.
Add validation and proper namespace initialization.
The namespace method needs validation and proper initialization:
public namespace(
name: keyof T,
cb: (ns: Namespace<T>) => void
): Namespace<T> {
+ const nameStr = String(name);
+ if (this.namespaces.has(nameStr)) {
+ throw new Error(`Namespace "${nameStr}" already exists`);
+ }
- const ns = new Namespace(this);
+ const ns = new Namespace(this, nameStr);
this.namespaces.set(name as string, ns);
cb(ns);
return ns;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public namespace( | |
| name: keyof T, | |
| cb: (ns: Namespace<T>) => void | |
| ): Namespace<T> { | |
| const ns = new Namespace(this); | |
| this.namespaces.set(name as string, ns); | |
| cb(ns); | |
| return ns; | |
| } | |
| public namespace( | |
| name: keyof T, | |
| cb: (ns: Namespace<T>) => void | |
| ): Namespace<T> { | |
| const nameStr = String(name); | |
| if (this.namespaces.has(nameStr)) { | |
| throw new Error(`Namespace "${nameStr}" already exists`); | |
| } | |
| const ns = new Namespace(this, nameStr); | |
| this.namespaces.set(name as string, ns); | |
| cb(ns); | |
| return ns; | |
| } |
Closes #296
Introduced changes
Checklist
Summary by CodeRabbit
New Features
QueryBuildermodule for constructing complex queries with nested namespaces and entities.queryBuildermodule for easier access through the SDK's main entry point.Tests
QueryBuilder, verifying instantiation and query construction functionality.Chores
1.0.0-alpha.30to1.0.0.