Build out index tags tests a bit more #319
Conversation
| }); | ||
|
|
||
| await pinecone.configureIndex(serverlessIndexName, { | ||
| tags: { project: '' }, | ||
| }); | ||
| const description2 = await pinecone.describeIndex(serverlessIndexName); | ||
| expect(description2.tags).toBeUndefined(); | ||
| if (description2.tags != null) { | ||
| expect(description2.tags['project']).toBeUndefined(); |
There was a problem hiding this comment.
This is interesting. I'm sure this has already been discussed, but I'd expect to have to explicitly pass null for this to happen and for empty strings to be the case, which is how a few other SDKs we use would do it.
There was a problem hiding this comment.
So the tag itself (the key value) is still present, but the value can be an empty string. If it's an empty string, then it's undefined. Does that make sense?
There was a problem hiding this comment.
This is definitely more on the implementation side and agnostic to the SDK, but if I understand correctly that json responses don't really encode undefined, ie. the key is just omitted altogether, providing project: '' in this case would remove the key altogether?
so i'd expect something like
input: `{ "project": null }`, output: `{}`
input: `{ "project": "" }`, output: `{ "project": "" }`
whereas this test tells me
input: `{ "project": "" }`, output: `{ }`
There was a problem hiding this comment.
well, so ' ' removes the key, but the user can't pass null because of type safety in typescript (their IDE will tell them that the passed value in a tag must be a string)
There was a problem hiding this comment.
(Same thing happpens here when you sub undefined for null:)
| }); | ||
|
|
||
| await pinecone.configureIndex(serverlessIndexName, { | ||
| tags: { project: '' }, | ||
| }); | ||
| const description2 = await pinecone.describeIndex(serverlessIndexName); | ||
| expect(description2.tags).toBeUndefined(); | ||
| if (description2.tags != null) { |
There was a problem hiding this comment.
If undefined != null, does this block still run?
Separately, are tags always returned, even if they're empty? I guess that would be my expectation as a user of the SDK.
There was a problem hiding this comment.
Question 1: undefined does equal null.
Question2 : Yes!
There was a problem hiding this comment.
Ah ok, so the conflation of null and undefined are intentional here because tags should always be {} regardless of whether they've been added to the response or not? That would make sense to me!
| }); | ||
|
|
||
| await pinecone.configureIndex(serverlessIndexName, { | ||
| tags: { project: '' }, | ||
| }); | ||
| const description2 = await pinecone.describeIndex(serverlessIndexName); | ||
| expect(description2.tags).toBeUndefined(); | ||
| if (description2.tags != null) { | ||
| expect(description2.tags['project']).toBeUndefined(); |
There was a problem hiding this comment.
This is definitely more on the implementation side and agnostic to the SDK, but if I understand correctly that json responses don't really encode undefined, ie. the key is just omitted altogether, providing project: '' in this case would remove the key altogether?
so i'd expect something like
input: `{ "project": null }`, output: `{}`
input: `{ "project": "" }`, output: `{ "project": "" }`
whereas this test tells me
input: `{ "project": "" }`, output: `{ }`
| }); | ||
|
|
||
| await pinecone.configureIndex(serverlessIndexName, { | ||
| tags: { project: '' }, | ||
| }); | ||
| const description2 = await pinecone.describeIndex(serverlessIndexName); | ||
| expect(description2.tags).toBeUndefined(); | ||
| if (description2.tags != null) { |
There was a problem hiding this comment.
Ah ok, so the conflation of null and undefined are intentional here because tags should always be {} regardless of whether they've been added to the response or not? That would make sense to me!
Problem
I just wanted to ensure that adding, deleting, and updating index tags on the same index gives us the results we expect, so added some integration tests.
Test Plan
CI passes