-
Notifications
You must be signed in to change notification settings - Fork 0
Add query best practices documentation #86
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
Add query best practices documentation #86
Conversation
…ation Co-authored-by: huangyiirene <[email protected]>
Co-authored-by: huangyiirene <[email protected]>
|
@copilot 你这个文档算不算是查询的最佳实践,要不要调整一下文件位置还有全部用英文。 |
Co-authored-by: huangyiirene <[email protected]>
Good point! I've made the following changes in commit 812b9f3:
The document is now positioned as a best practices guide under "Data & Logic Layers" → "Query Best Practices" in the documentation navigation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds comprehensive query best practices documentation in response to a request for evaluation of query syntax alternatives and optimization strategies. The documentation provides detailed comparisons between JSON-DSL, REST, and GraphQL interfaces along with quantified performance optimization techniques.
Changes:
- Added a new 17KB best practices guide covering three query interfaces with performance benchmarks, optimization patterns, and scenario-based recommendations
- Integrated navigation and cross-references across 5 documentation locations to improve discoverability
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
docs/guide/query-best-practices.md |
New comprehensive guide with comparative analysis, 10+ optimization patterns with quantified impact metrics, and decision trees |
docs/.vitepress/config.mts |
Added navigation entry for "Query Best Practices" in the guide sidebar |
docs/guide/querying.md |
Added cross-reference callout linking to the new best practices guide |
docs/spec/query-language.md |
Added cross-reference callout linking to the best practices guide |
docs/api/index.md |
Added cross-reference callout linking to the best practices guide |
README.md |
Added "Query Approaches" section with summary and link to detailed guide |
| | **JSON-DSL (Core)** | Server-side logic, AI agents | Medium | ⭐⭐⭐⭐⭐ | ⭐⭐⭐⭐⭐ | | ||
| | **REST API** | Simple CRUD, mobile apps | Low | ⭐⭐⭐⭐ | ⭐⭐⭐ | | ||
| | **GraphQL** | Complex data graphs, modern SPAs | High | ⭐⭐⭐⭐⭐ | ⭐⭐⭐⭐ | |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The star rating system lacks an explicit legend defining what the ratings represent (e.g., 5 stars = excellent, 1 star = poor). Without this context, readers must infer the meaning, which could lead to misinterpretation. Consider adding a note below the table or in the column header, such as "Performance (⭐ = lowest, ⭐⭐⭐⭐⭐ = highest)" to clarify the rating scale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added rating scale legend below the table in commit 619ab62: "Rating scale: ⭐ = lowest, ⭐⭐⭐⭐⭐ = highest"
docs/guide/query-best-practices.md
Outdated
| for (const task of tasks) { | ||
| task.project = await app.object('project').findOne(task.project_id); | ||
| task.assignee = await app.object('user').findOne(task.assignee_id); |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code mutates the task objects retrieved from the database by directly assigning properties. This pattern can lead to unexpected behavior if the task objects are frozen or have property descriptors that prevent assignment. Consider using a more explicit approach such as creating new objects or using spread syntax to make the transformation explicit.
| for (const task of tasks) { | |
| task.project = await app.object('project').findOne(task.project_id); | |
| task.assignee = await app.object('user').findOne(task.assignee_id); | |
| const tasksWithRelations = []; | |
| for (const task of tasks) { | |
| const project = await app.object('project').findOne(task.project_id); | |
| const assignee = await app.object('user').findOne(task.assignee_id); | |
| tasksWithRelations.push({ | |
| ...task, | |
| project, | |
| assignee | |
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 619ab62. Updated code to use immutable pattern with spread syntax, creating new objects in an enrichedTasks array instead of mutating the original task objects.
docs/guide/query-best-practices.md
Outdated
| // Bad: N+1 queries | ||
| const tasks = await taskRepo.find(); | ||
| for (const task of tasks) { | ||
| task.assignee = await userRepo.findOne(task.assignee_id); | ||
| } | ||
|
|
||
| // Good: Batched loading (1+1 queries) | ||
| const tasks = await taskRepo.find(); | ||
| const userIds = tasks.map(t => t.assignee_id); | ||
| const users = await userRepo.find({ | ||
| filters: [['id', 'in', userIds]] | ||
| }); | ||
| const userMap = new Map(users.map(u => [u.id, u])); | ||
| tasks.forEach(task => { | ||
| task.assignee = userMap.get(task.assignee_id); | ||
| }); |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to a previous example, this code mutates task objects directly. This pattern is repeated in multiple code examples throughout the document. Consider establishing a consistent pattern for handling data transformations to improve maintainability and avoid potential issues with object mutation.
| // Bad: N+1 queries | |
| const tasks = await taskRepo.find(); | |
| for (const task of tasks) { | |
| task.assignee = await userRepo.findOne(task.assignee_id); | |
| } | |
| // Good: Batched loading (1+1 queries) | |
| const tasks = await taskRepo.find(); | |
| const userIds = tasks.map(t => t.assignee_id); | |
| const users = await userRepo.find({ | |
| filters: [['id', 'in', userIds]] | |
| }); | |
| const userMap = new Map(users.map(u => [u.id, u])); | |
| tasks.forEach(task => { | |
| task.assignee = userMap.get(task.assignee_id); | |
| }); | |
| // Bad: N+1 queries (inefficient, but shown immutably) | |
| const tasks = await taskRepo.find(); | |
| const tasksWithAssignee = await Promise.all( | |
| tasks.map(async (task) => ({ | |
| ...task, | |
| assignee: await userRepo.findOne(task.assignee_id), | |
| })), | |
| ); | |
| // Good: Batched loading (1+1 queries, immutable transformation) | |
| const tasks = await taskRepo.find(); | |
| const userIds = tasks.map((t) => t.assignee_id); | |
| const users = await userRepo.find({ | |
| filters: [['id', 'in', userIds]], | |
| }); | |
| const userMap = new Map(users.map((u) => [u.id, u])); | |
| const tasksWithAssigneeBatched = tasks.map((task) => ({ | |
| ...task, | |
| assignee: userMap.get(task.assignee_id), | |
| })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied consistent immutable pattern in commit 619ab62. Both bad and good examples now use map() to create new objects with spread syntax instead of forEach() with mutations.
| SELECT * FROM tasks WHERE due_date < NOW() | ||
| AND priority = 'high' AND invalid_function(status); | ||
| -- ❌ Error: invalid_function does not exist |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQL example demonstrates a hallucinated function but doesn't clearly indicate this is intentional to show AI failure modes. While the error comment is present, adding an explicit note like "Example of AI hallucination:" before the code block would make the educational intent clearer and prevent confusion about whether this is a documentation error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added explicit label in commit 619ab62: "Example of AI hallucination:" now appears before the SQL code block to clearly indicate this demonstrates an AI failure mode.
docs/guide/query-best-practices.md
Outdated
| - [Querying Guide](./querying.md) - Step-by-step query examples | ||
| - [GraphQL API Documentation](../api/graphql.md) - GraphQL setup and usage | ||
| - [REST API Documentation](../api/rest.md) - REST endpoint reference | ||
| - [Performance Tuning](./performance.md) - Advanced optimization strategies |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The references to ../api/graphql.md, ../api/rest.md, and ./performance.md may be broken links if these files don't exist in the repository. Since this is a new documentation file being added, verify that all referenced files exist or update the links to point to existing documentation. Dead links negatively impact documentation usability.
| - [Performance Tuning](./performance.md) - Advanced optimization strategies | |
| - Performance Tuning - Advanced optimization strategies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 619ab62. Removed the broken link to performance.md as this file doesn't exist. The other links (graphql.md, rest.md) have been verified to exist.
|
@copilot fix |
…ples Co-authored-by: huangyiirene <[email protected]>
Fixed all review issues in commit 619ab62:
All code examples now follow immutable data transformation patterns instead of direct property mutations. |
Query Best Practices Documentation
Task: Create best practices documentation for querying in ObjectQL.
Completed:
Recent Changes (Code Review Fixes):
performance.mdfileDocumentation Content:
docs/guide/query-best-practices.md(17KB)Integration:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.