Skip to content

Conversation

danmichaeljones
Copy link
Contributor

@danmichaeljones danmichaeljones commented Aug 20, 2025

This adds the new search-only mode to the Query Agent client:

  • Adds search method to QueryAgent to retrieve the first page of search results
  • The next() method on the response is used to paginate the next sets of results consistently (i.e., same results set)

Copy link

@orca-security-eu orca-security-eu bot left a comment

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca

@danmichaeljones danmichaeljones marked this pull request as ready for review August 20, 2025 15:56
@danmichaeljones danmichaeljones marked this pull request as draft August 21, 2025 14:37
@danmichaeljones danmichaeljones marked this pull request as ready for review August 22, 2025 15:35
*/
async search<T = undefined>(
query: string,
{ limit, collections }: QueryAgentSearchOnlyOptions = {},

Choose a reason for hiding this comment

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

IMO, having this be options?: QueryAgentSearchOnlyOptions would be clearer for a user on the exact nature of this functional argument. Having the object keys explicit and it having a default of {} is quite different to the rest of the TS client so might be a point of confusion for users

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense, but doesn't match the existing QueryAgent methods (which I don't watch to touch now). I'll CC @augustas1 to see what he thinks once he's back, but for now I'd prefer to be consistent with the existing run / stream methods.

I've modified the signature a little though, to try and make the default argument values clearer 👍

Comment on lines 99 to 100
* @param options.limit - The maximum number of results to return. Defaults to 20 if not specified.
* @param options.offset - The offset to start from. If not specified, retrieval begins from the first object.

Choose a reason for hiding this comment

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

Optional args can be signified in JSdoc using [] like so: [options.limit]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool - done! 👍

* @param options.offset - The offset to start from. If not specified, retrieval begins from the first object.
* @returns A SearchModeResponse object containing the results, usage, and underlying searches performed.
*/
async run(options: SearchExecutionOptions): Promise<SearchModeResponse<T>> {

Choose a reason for hiding this comment

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

Should a user be able to just call await search.run()? If so, options needs to be optional here: options?: SearchExecutionOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mostly a method the end user won't touch (they should be interacting with it via the next() method on the SearchModeResponse), but I've updated to have default values (following the same convention as above) so you can now call plain await search.run().

}
return {
...mappedResponse,
next: async ({ limit: nextLimit = 20, offset: nextOffset = 0 } = {}) =>

Choose a reason for hiding this comment

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

The function args here are different to how they are in the run outer function, might that be confusing from a UX PoV? Can the same SearchExecutionOptions type be reused here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's nicer - I've moved the SearchExecutionOptions around and they're reused here.

Comment on lines 275 to 278
next: (options?: {
limit?: number;
offset?: number;
}) => Promise<SearchModeResponse<T>>;
Copy link

@tsmith023 tsmith023 Aug 27, 2025

Choose a reason for hiding this comment

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

Likewise here as above wrt to the SearchExecutionOptions type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, now uses SearchExecutionOptions 👍

searches?: ApiSearchResult[];
usage: ApiUsage;
total_time: number;
search_results: WeaviateReturn<T>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sadly results also probably are returned from API as underscore_case and not camelCase, so we'd have to map it as well to make it developer friendly 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, good catch! I've adding mapping from the api snake_case to camelCase 👍 I've also removed all the generics on the types to make this work (we can add them back later if we want, but they were for typing properties but our search results are potentially multi-collection, so maybe didn't make sense anyway 🤷 ).

The types of the search result objects are also an extension of the Weaviate types, to add a collection field (which was missing from the original type).

@@ -260,3 +262,23 @@ export type StreamedTokens = {
outputType: "streamedTokens";
delta: string;
};

export type MappedSearchModeResponse<T> = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks like internal type and everything in this file is exported to user (see index.ts file).
So Maybe move it to response-mapping.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense 👍 Moving to response-mapping.ts would end up with circular dependencies, so I've just removed this type and am using Omit<SearchModeResponse, "next"> in it's place (since it's internal anyway)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This works as well :) fyi circular dependencies are supported in JS/TS (especially fine with for types)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, TIL!

}
return {
...mappedResponse,
next: async (options: SearchExecutionOptions = {}) => this.run(options),
Copy link
Collaborator

Choose a reason for hiding this comment

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

In SearchModeResponse options are declared as required, so probably = {} shouldn't be here?
On that note, if it's required, we probably expect user to always pass offset on next, so maybe offset need to be required as well (in comparison to run)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even better, I see run is being used only internally and we explicitly pass offset: 0, so maybe we can just make offset required on whole SearchExecutionOptions?

Copy link
Contributor Author

@danmichaeljones danmichaeljones Sep 3, 2025

Choose a reason for hiding this comment

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

The SearchExecutionOptions are also used on the SearchModeResponse.next method, so I don't want to make offset required there. I've removed the = {} default value though.

Copy link
Collaborator

@augustas1 augustas1 Sep 4, 2025

Choose a reason for hiding this comment

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

isn't the whole point of next that offset should be passed? Otherwise it'll return the first page again (returned on initial search)?
Just think that users might think next is smart and will magically return next page somehow and don't pass offset if we don't require it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just think that users might think next is smart and will magically return next page somehow and don't pass offset if we don't require it.

Yeah, that's true.. I've updated the SearchExecutionOptions so that offset is required (and left limit optional). That's not consistent with what's in the Python version, but we can discuss whether that should be changed 👍


return {
properties: object.properties,
metadata: metadata,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick can omit variable when key has the same name. metadata,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch - fixed.

@danmichaeljones danmichaeljones merged commit 820e581 into main Sep 4, 2025
5 checks passed
@danmichaeljones danmichaeljones deleted the search_only branch September 4, 2025 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants