-
Notifications
You must be signed in to change notification settings - Fork 21
fix: prevents runtime error on undefined facets
#48
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
base: master
Are you sure you want to change the base?
Conversation
Thank you! Will need some unit tests to validate the change. Also added a comment |
It is not safe to assume that facets is going to be in the response
Any updates on this? |
it("will handle not having `facets` in the backend response without throwing", async () => { | ||
const result = await client.search("dog", { | ||
...config, | ||
filters: { |
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.
The change looks good but the testcase is incorrect. What you want in the testcase is to validate that the request doesn't have the presence of facets object. I still see in the request fixture, the facets object thats populated with a facet configuration (which is expected with this testcase).
I think you should see this behavior (without the fix) with the following configuration:
const result = await client.search("dog", {
...config,
filters: {
license: "BSD"
},
facets: {
dependencies: [{ type: "value", size: 3 }]
},
disjunctiveFacets: ["license"]
});
The other way is to filter disjunctiveFacets array that are only present in facets objects too.
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.
I don't think that's correct; I don't want to validate the request not having the presence of the facet object. It's the response what I really care about. That's why I do the assertion down below. This is really similar to the original request that produced the issue for me if you look back to the original slack thread.
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.
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.
@joemcelroy any updates?
It is not safe to assume that facets is going to be in the response
As discussed in this slack thread https://elasticstack.slack.com/archives/C018C7B9T5F/p1679935623878799
The backend for app search sometimes does not include a
facets
in the response. I'm able to reproduce when our product is deployed to a new environment and the document count is zero.