Skip to content

print tablename in unique() error message #108

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

print tablename in unique() error message #108

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 8 additions & 2 deletions npm-packages/convex/src/server/impl/query_impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,15 @@ export class QueryImpl implements Query<GenericTableInfo> {
| { type: "executing"; queryId: number }
| { type: "closed" }
| { type: "consumed" };

private tableName: string;

constructor(query: SerializedQuery) {
this.state = { type: "preparing", query };
if (query.source.type === "FullTableScan") {
Copy link
Contributor

Choose a reason for hiding this comment

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

just to avoid having multiple sources of truth, let's add a getter for this instead of stashing it here in the constructor
Not so big deal since it's private, but TypeScript private doesn't stop users from riting JavaScript that expects this.tableName to be there

this.tableName = query.source.tableName;
} else {
this.tableName = query.source.indexName.split(".")[0];
}
}

private takeQuery(): SerializedQuery {
Expand Down Expand Up @@ -333,7 +339,7 @@ export class QueryImpl implements Query<GenericTableInfo> {
return null;
}
if (first_two_array.length === 2) {
throw new Error(`unique() query returned more than one result:
throw new Error(`unique() query returned more than one result from table ${this.tableName}:
Copy link
Collaborator

Choose a reason for hiding this comment

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

@thomasballinger what do you think?
I almost feel like it would be more useful to say "unique() returned more than one result from query on ${this.indexName}"

Copy link
Collaborator

Choose a reason for hiding this comment

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

@djbalin what do you think about printing indexName here instead of tableName? It'll give more information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nipunn1313 with indexName being the string "table.index", like "messages.by_userId"? Yes that would work for me, but simply because I would only use the table name which is the important part as far as I am concerned. In my daily routine, I use this error message to go to the dashboard and look up the non-unique documents to find out why something I expected to be unique is not, and most likely to manually fix this from the dashboard.
With the log printing both _id and tableName of the documents, I can do this directly in one step from the error log :)
I can't off the top of my head see how indexName would be useful for debugging purposes, but can't hurt either I guess!

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me, it's table name that's immediately actionable and suggests the broken invariant, and nothing can that can be unique with an index that isn't unique on a table

[${first_two_array[0]._id}, ${first_two_array[1]._id}, ...]`);
}
return first_two_array[0];
Expand Down