Skip to content
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

fix(mongodb): mongodb storage mget must return undefined when key not exist #6445

Merged
merged 4 commits into from
Sep 4, 2024

Conversation

rtyshyk
Copy link
Contributor

@rtyshyk rtyshyk commented Aug 7, 2024

Fixes #6441 (issue)

mget method in BaseStore must return values or undefined in the same order as the input keys.

PR is fixing MongoDB storage implementation.

Copy link

vercel bot commented Aug 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview Sep 4, 2024 10:37pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ⬜️ Ignored (Inspect) Visit Preview Sep 4, 2024 10:37pm

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. auto:bug Related to a bug, vulnerability, unexpected error with an existing feature labels Aug 7, 2024
);

return keys.map((key) => {
const prefixedKey = this._getPrefixedKey(key);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should just map over prefixedKeys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, done.

const prefixedKey = this._getPrefixedKey(key);
const value = valueMap.get(prefixedKey);

if (!value || !("value" in value)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

valueMap contains tuples now, so this would never be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, the second part of the condition was removed.

@jacoblee93
Copy link
Collaborator

Sorry for the delay! A few questions

@jacoblee93 jacoblee93 added the question Further information is requested label Aug 16, 2024
Copy link

vercel bot commented Aug 18, 2024

Someone is attempting to deploy a commit to the LangChain Team on Vercel.

A member of the Team first needs to authorize it.

@rtyshyk
Copy link
Contributor Author

rtyshyk commented Aug 18, 2024

Sorry for the delay! A few questions

not a problem. review updated. please have a look.

@rtyshyk
Copy link
Contributor Author

rtyshyk commented Sep 3, 2024

@jacoblee93 can you please merge it?

@jacoblee93
Copy link
Collaborator

Sorry about the delay, will ship shortly!

@dosubot dosubot bot added the lgtm PRs that are ready to be merged as-is label Sep 4, 2024
@jacoblee93 jacoblee93 changed the title fix: mongodb storage mget must return undefined when key not exist fix(mongodb): mongodb storage mget must return undefined when key not exist Sep 4, 2024
@jacoblee93 jacoblee93 merged commit 51c3ab2 into langchain-ai:main Sep 4, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:bug Related to a bug, vulnerability, unexpected error with an existing feature lgtm PRs that are ready to be merged as-is question Further information is requested size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MongoStore does not work as Cache for embeddings
2 participants