CFSQL-1473: fix D1 exec throwing TypeError on invalid SQL queries#6029
CFSQL-1473: fix D1 exec throwing TypeError on invalid SQL queries#6029FlorentCollin wants to merge 4 commits intomainfrom
Conversation
We were trying to call `addAggregatedD1MetaToSpan` before checking if any of the results are errors in a `.exec` call. When an error is returned the meta property is not set. This commit is now checking the errors first and then call the `addAggregatedD1MetaToSpan` if there is no error.
| } | ||
| ); | ||
| } else { | ||
| addAggregatedD1MetaToSpan( |
There was a problem hiding this comment.
This is good. We should also make the function addAggregatedD1MetaToSpan defensive in my opinion on its own, so that it doesn't crash even if an invalid object is passed in. It should only process things that exist on the given meta span.
There was a problem hiding this comment.
Good idea. I added defensive checks to both the addAggregatedD1MetaToSpan and aggregateD1Meta. It should be good now.
There was a problem hiding this comment.
It turns out that eslint is annoying
785:32 error Unnecessary conditional, expected left-hand side of `??` operator to be possibly null or undefined @typescript-eslint/no-unnecessary-condition
For now I added a bunch of eslint-disable-next-line but it's starting to get a bit ugly even though it's good to have these defensive checks.
Merging this PR will not alter performance
Comparing 
Footnotes
|
FlorentCollin
force-pushed
the
fcollin/CFSQL-1473-fix-db-exec-type-error
branch
2 times, most recently
from
a4cf765 to
d9b21f5
Compare
FlorentCollin
force-pushed
the
fcollin/CFSQL-1473-fix-db-exec-type-error
branch
from
d9b21f5 to
47e7695
Compare
| aggregatedMeta.last_row_id = meta.last_row_id; | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| aggregatedMeta.size_after = meta.size_after ?? 0; | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition |
There was a problem hiding this comment.
If you have to add ??, then the typescript types are wrong. Rather than ignoring eslint, can we fix the types please?
| } | ||
|
|
||
| function addAggregatedD1MetaToSpan(span: Span, metas: D1Meta[]): void { | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition |
There was a problem hiding this comment.
This would be unnecessary if you changed the param types.
|
|
||
| for (const meta of metas) { | ||
| aggregatedMeta.duration += meta.duration; | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition |
There was a problem hiding this comment.
Again, this is unnecessary if the type is correct. If it's not correct, then let's update the types.
3 participants
We were trying to call
addAggregatedD1MetaToSpanbefore checking if any of the results are errors in a.execcall. When an error is returned the meta property is not set.This commit is now checking the errors first and then call the
addAggregatedD1MetaToSpanif there is no error.