print tablename in unique() error message #108
Conversation
| @@ -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}: | |||
There was a problem hiding this comment.
@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}"
There was a problem hiding this comment.
@djbalin what do you think about printing indexName here instead of tableName? It'll give more information.
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
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
|
thx for the idea! This looks reasonable - just want to check in with tom. |
|
Side note, if there's anywhere that developers see this error without a stack trace that's something we should fix, this is a good improvements to make as well but making sure there's a stack trace is top pri |
@thomasballinger I haven't experienced missing stack trace, pasted is a log example. As also outlined in my other comment, I suppose that a developer who runs into 
|
| constructor(query: SerializedQuery) { | ||
| this.state = { type: "preparing", query }; | ||
| if (query.source.type === "FullTableScan") { |
There was a problem hiding this comment.
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
|
Let's do it, thanks @djbalin! This is great, I think we have other error messages that coudl be better like this. |
|
Awesome :) there is also an implementation of |
Improves usefulness of the error thrown when
.unique()returns more than 1 result by now also printing the table name.Improves quality of life: currently, it can be cumbersome to identify the actual table in which these non-unique elements belong.
This solution is a bit work-around-y, but it's the only solution I could come up with. Let me know if there is a better way to achieve this!
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.