Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add querybuilder #320

Merged
merged 2 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions packages/sdk/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,11 @@ See [Conventional Commits](https://conventionalcommits.org) for commit guideline

**Note:** Version bump only for package @dojoengine/sdk





# [1.0.0-alpha.30](https://github.com/dojoengine/sdk/compare/v1.0.0-alpha.29...v1.0.0-alpha.30) (2024-11-09)


### Features

* docs ([1bf47fa](https://github.com/dojoengine/sdk/commit/1bf47fae3afef5ab7f02f9ed288141e735ab2788))




- docs ([1bf47fa](https://github.com/dojoengine/sdk/commit/1bf47fae3afef5ab7f02f9ed288141e735ab2788))

# [1.0.0-alpha.29](https://github.com/dojoengine/sdk/compare/v1.0.0-alpha.28...v1.0.0-alpha.29) (2024-11-08)

Expand Down
92 changes: 34 additions & 58 deletions packages/sdk/src/__example__/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// EXAMPLE FOR NOW

import * as torii from "@dojoengine/torii-client";
import { init } from "..";
import { init, QueryBuilder } from "..";
import { SchemaType } from "../types";

export interface PlayerModel {
Expand Down Expand Up @@ -111,73 +111,49 @@ async function exampleUsage() {
invalidMessage // TypeScript Error
);

db.subscribeEntityQuery(
{
world: {
player: {
$: {
where: {
name: { $is: "Alice" },
score: { $is: 10 },
},
},
},
},
},
(resp) => {
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)
);
}
});
Comment on lines +114 to +132
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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)
);
}
});

// Example usage of getEntities with where clause
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)
);
}
}
);
});
Comment on lines +134 to +156
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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)
);
}
});

console.log("Queried entities:", entities);
} catch (error) {
console.error("Error querying entities:", error);
Expand Down
37 changes: 37 additions & 0 deletions packages/sdk/src/__tests__/queryBuilder.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { describe, expect, it } from "vitest";
import { QueryBuilder } from "../queryBuilder";
import { MockSchemaType } from "../__example__/index";

describe("QueryBuilder", () => {
it("should be implemented", () => {
const query = new QueryBuilder().build();
expect(query).toStrictEqual({});
});
Comment on lines +6 to +9
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance basic test coverage

The current test case "should be implemented" could be improved:

  1. Rename to something more descriptive like "should create empty query when no parameters are provided"
  2. 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();
});


it("should work with example", () => {
const query = new QueryBuilder<MockSchemaType>()
.namespace("world", (n) =>
n.entity("player", (e) => e.eq("id", "1").eq("name", "Alice"))
)
.namespace("universe", (n) =>
n.entity("galaxy", (e) => e.is("name", "Milky Way"))
)
.build();
const expected = {
world: {
player: {
$: { where: { id: { $eq: "1" }, name: { $eq: "Alice" } } },
},
},
universe: {
galaxy: {
$: {
where: { name: { $is: "Milky Way" } },
},
},
},
};

expect(query).toStrictEqual(expected);
});
});
1 change: 1 addition & 0 deletions packages/sdk/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { SchemaType, SDK, SDKConfig, UnionOfModelData } from "./types";

export * from "./types";
export * from "./state";
export * from "./queryBuilder";

/**
* Creates a new Torii client instance.
Expand Down
162 changes: 162 additions & 0 deletions packages/sdk/src/queryBuilder.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
import { QueryType, SchemaType, SubscriptionQueryType } from "./types";

type NestedKeyOf<ObjectType extends object> = {
[Key in keyof ObjectType &
(string | number)]: ObjectType[Key] extends object
? `${Key}` | `${Key}.${NestedKeyOf<ObjectType[Key]>}`
: `${Key}`;
}[keyof ObjectType & (string | number)];

export class QueryBuilder<T extends SchemaType> {
namespaces: Map<string, Namespace<T>>;

constructor() {
this.namespaces = new Map<string, Namespace<T>>();
}

public namespace(
name: keyof T,
cb: (ns: Namespace<T>) => void
): Namespace<T> {
const ns = new Namespace(this);
this.namespaces.set(name as string, ns);
Comment on lines +21 to +22
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

cb(ns);
return ns;
}
Comment on lines +17 to +25
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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;
}


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>;
}
Comment on lines +27 to +55
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

}

class Namespace<T extends SchemaType> {
entities: Map<string, QueryEntity<T>>;

constructor(private parent: QueryBuilder<T>) {
this.entities = new Map<string, QueryEntity<T>>();
}
Comment on lines +58 to +63
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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>>();
}


public entity(
name: NestedKeyOf<T>,
cb: (entity: QueryEntity<T>) => void
): QueryEntity<T> {
const entity = new QueryEntity(this);
this.entities.set(name, entity);
cb(entity);
return entity;
}

public namespace(ns: string, cb: (ns: Namespace<T>) => void): Namespace<T> {
return this.parent.namespace(ns, cb);
}
Comment on lines +75 to +77
Copy link

Choose a reason for hiding this comment

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

🛠️ 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 build(): SubscriptionQueryType<T> {
return this.parent.build();
}
}

class QueryEntity<T extends SchemaType> {
constraints: Map<string, Constraint>;

constructor(private parent: Namespace<T>) {
this.constraints = new Map<string, Constraint>();
}
public entity(
name: NestedKeyOf<T>,
cb: (entity: QueryEntity<T>) => void
): QueryEntity<T> {
return this.parent.entity(name, cb);
}

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);
}
Comment on lines +97 to +123
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add input validation for constraint values

The constraint methods accept any value without validation, which could lead to injection vulnerabilities.

Consider adding:

  1. Type validation for values based on field types
  2. Sanitization for string values
  3. 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;
 }


private addConstraint(
field: string,
value: any,
op: Operator
): QueryEntity<T> {
this.constraints.set(field, new Constraint(op, value));
return this;
}
Comment on lines +125 to +132
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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(): QueryType<T> {
return this.parent.build();
}
}

class Constraint {
constructor(
private _operator: Operator,
private _value: any
) {}

get operator(): string {
return this._operator.toString();
}

get value(): any {
return this._value;
}
}
Comment on lines +139 to +152
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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;
}
}


enum Operator {
is = "$is",
eq = "$eq",
neq = "$neq",
gt = "$gt",
gte = "$gte",
lt = "$lt",
lte = "$lte",
}