Skip to content

Commit

Permalink
[spruce] Fix index targeting via VectorOperationsProvider (#179)
Browse files Browse the repository at this point in the history
## Problem
Back when I implemented the new `IndexHostSingleton` there was a bug
introduced in `VectorOperationsProvider`. Since we're assigning to
`this.config.indeHostUrl` inside `provide()`, it's updating the `config`
reference meaning once you initially target an index, it holds onto that
URL and reuses for all dataplane requests, regardless of which index
you're now trying to target.


https://github.com/pinecone-io/pinecone-ts-client/blob/41389d4a101cea996d17848c49f22d2d3991b34a/src/data/vectorOperationsProvider.ts#L39

## Solution

- Remove the `IndexConfiguration` type. Originally I had added
`IndexConfiguration` as an extension of `PineconeConfiguration`, but
this feels unnecessary as it was only used within
`VectorOperationsProvider`, and it proved to be more confusing than
anything.
- Add `indexHostUrl` as a private field to `VectorOperationsProvider`.
This field will be set immediately if an `indexHostUrl` has been passed
as part of the `Index` constructor, otherwise it's resolved via the
`IndexHostSingleton`.
- `namespace` targeting in the `Index` class was not passing
`indexHostUrl` as we'd expect, updated this will I was making changes.

## Type of Change
- [X] Bug fix (non-breaking change which fixes an issue)

## Test Plan
You'll need multiple indexes so you can chain calls to one or the other
to validate targeting.

```
$ npm run repl
$ await init

$ await client.index('index-1').describeIndexStats()
// verify the correct stats for index-1 are returned

$ await client.index('index-2').describeIndexStats()
// verify the correct stats for index-2 are returned
```

Previously, once you'd targeted an index all subsequent dataplane calls
would be made to the original host URL.
  • Loading branch information
austin-denoble authored Jan 3, 2024
1 parent 41389d4 commit f7d49d8
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 29 deletions.
5 changes: 1 addition & 4 deletions src/data/__tests__/vectorOperationsProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,6 @@ describe('VectorOperationsProvider', () => {
await provider.provide();

expect(IndexHostSingleton.getHostUrl).not.toHaveBeenCalled();
expect(provider.buildVectorOperationsConfig).toHaveBeenCalledWith({
...config,
hostUrl: indexHostUrl,
});
expect(provider.buildVectorOperationsConfig).toHaveBeenCalled();
});
});
7 changes: 6 additions & 1 deletion src/data/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,12 @@ export class Index<T extends RecordMetadata = RecordMetadata> {
* This `namespace()` method will inherit custom metadata types if you are chaining the call off an { @link Index } client instance that is typed with a user-specified metadata type. See { @link Pinecone.index } for more info.
*/
namespace(namespace: string): Index<T> {
return new Index<T>(this.target.index, this.config, namespace);
return new Index<T>(
this.target.index,
this.config,
namespace,
this.target.indexHostUrl
);
}

/**
Expand Down
8 changes: 0 additions & 8 deletions src/data/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,6 @@ export type PineconeConfiguration = {
additionalHeaders?: HTTPHeaders;
};

/** Configuration for a single Pinecone Index */
export type IndexConfiguration = PineconeConfiguration & {
/**
* The host URL for the Index.
*/
hostUrl?: string;
};

export const RecordIdSchema = Type.String({ minLength: 1 });
export const RecordValuesSchema = Type.Array(Type.Number());
export const RecordSparseValuesSchema = Type.Object(
Expand Down
30 changes: 14 additions & 16 deletions src/data/vectorOperationsProvider.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { IndexConfiguration, PineconeConfiguration } from './types';
import type { PineconeConfiguration } from './types';
import {
Configuration,
ConfigurationParameters,
Expand All @@ -9,8 +9,9 @@ import { IndexHostSingleton } from './indexHostSingleton';
import { middleware } from '../utils/middleware';

export class VectorOperationsProvider {
private config: IndexConfiguration;
private config: PineconeConfiguration;
private indexName: string;
private indexHostUrl?: string;
private vectorOperations?: VectorOperationsApi;

constructor(
Expand All @@ -20,42 +21,39 @@ export class VectorOperationsProvider {
) {
this.config = config;
this.indexName = indexName;

// If an indexHostUrl has been passed set it, otherwise keep
// it undefined so that hostUrl is properly resolved
if (indexHostUrl) {
this.config.hostUrl = indexHostUrl;
}
this.indexHostUrl = indexHostUrl;
}

async provide() {
if (this.vectorOperations) {
return this.vectorOperations;
}

if (this.config.hostUrl) {
this.vectorOperations = this.buildVectorOperationsConfig(this.config);
// If an indexHostUrl has been manually passed we use that,
// otherwise we rely on resolving the host from the IndexHostSingleton
if (this.indexHostUrl) {
this.vectorOperations = this.buildVectorOperationsConfig();
} else {
this.config.hostUrl = await IndexHostSingleton.getHostUrl(
this.indexHostUrl = await IndexHostSingleton.getHostUrl(
this.config,
this.indexName
);

this.vectorOperations = this.buildVectorOperationsConfig(this.config);
this.vectorOperations = this.buildVectorOperationsConfig();
}

return this.vectorOperations;
}

buildVectorOperationsConfig(config: IndexConfiguration) {
buildVectorOperationsConfig() {
const indexConfigurationParameters: ConfigurationParameters = {
basePath: config.hostUrl,
apiKey: config.apiKey,
basePath: this.indexHostUrl,
apiKey: this.config.apiKey,
queryParamsStringify,
headers: {
'User-Agent': buildUserAgent(false),
},
fetchApi: getFetch(config),
fetchApi: getFetch(this.config),
middleware,
};

Expand Down

0 comments on commit f7d49d8

Please sign in to comment.