-
Notifications
You must be signed in to change notification settings - Fork 57.5k
fix(Google Gemini Node): Filter out undefined parts when processing API responses #28567
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
Changes from 4 commits
cf3fd7e
f738159
f8e8635
b26163e
d0b8ba8
1ce538c
bfd7811
6cda7de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| import { getConnectedTools } from '@utils/helpers'; | ||
| import { | ||
| type IDataObject, | ||
| type IExecuteFunctions, | ||
|
|
@@ -11,13 +10,17 @@ import { | |
| } from 'n8n-workflow'; | ||
| import zodToJsonSchema from 'zod-to-json-schema'; | ||
|
|
||
| import { getConnectedTools } from '@utils/helpers'; | ||
|
|
||
| import type { | ||
| GenerateContentRequest, | ||
| GenerateContentResponse, | ||
| Content, | ||
| Tool, | ||
| GenerateContentGenerationConfig, | ||
| BuiltInTools, | ||
| FunctionCallPart, | ||
| TextPart, | ||
| } from '../../helpers/interfaces'; | ||
| import { apiRequest } from '../../transport'; | ||
| import { modelRLC } from '../descriptions'; | ||
|
|
@@ -341,8 +344,16 @@ const displayOptions = { | |
|
|
||
| export const description = updateDisplayOptions(displayOptions, properties); | ||
|
|
||
| function isFunctionCallPart(part: unknown): part is FunctionCallPart { | ||
| return !!part && typeof part === 'object' && 'functionCall' in part; | ||
| } | ||
|
|
||
| function isTextPart(part: unknown): part is TextPart { | ||
| return !!part && typeof part === 'object' && 'text' in part; | ||
| } | ||
|
|
||
| function getToolCalls(response: GenerateContentResponse) { | ||
| return response.candidates.flatMap((c) => c.content.parts).filter((p) => 'functionCall' in p); | ||
| return response.candidates.flatMap((c) => c?.content?.parts ?? []).filter(isFunctionCallPart); | ||
| } | ||
|
|
||
| export async function execute(this: IExecuteFunctions, i: number): Promise<INodeExecutionData[]> { | ||
|
|
@@ -538,7 +549,10 @@ export async function execute(this: IExecuteFunctions, i: number): Promise<INode | |
| break; | ||
| } | ||
|
|
||
| contents.push(...response.candidates.map((c) => c.content)); | ||
| contents.push.apply( | ||
| contents, | ||
| response.candidates.map((c) => c.content), | ||
| ); | ||
|
Comment on lines
+557
to
+560
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. claude: The old push.apply pattern came from pre-ES6 days when spread didn't exist. It was used as a workaround. Today, contents.push(...arr) is the idiomatic equivalent and compiles to the same bytecode in V8.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this got changed because I ran eslint autofix, and we have a lint rule to prefer The reasoning: if the array being spread is very big, it can cause stack overflow, because each array item is converted into a function argument, therefor it takes a lot of memory on the stack This doesn't apply here, because |
||
|
|
||
| for (const { functionCall } of toolCalls) { | ||
| let toolResponse; | ||
|
|
@@ -575,9 +589,9 @@ export async function execute(this: IExecuteFunctions, i: number): Promise<INode | |
| const candidates = options.includeMergedResponse | ||
| ? response.candidates.map((candidate) => ({ | ||
| ...candidate, | ||
| mergedResponse: candidate.content.parts | ||
| .filter((part) => 'text' in part) | ||
| .map((part) => (part as { text: string }).text) | ||
| mergedResponse: (candidate?.content?.parts ?? []) | ||
| .filter(isTextPart) | ||
| .map((part) => part.text) | ||
| .join(''), | ||
| })) | ||
| : response.candidates; | ||
|
|
||
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.
maybe better name
"should handle undefined entries in parts array"
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.
Agree, updated it