fix: prevents runtime error on undefined facets#48
fix: prevents runtime error on undefined facets#48gerardosabetta wants to merge 1 commit intoelastic:masterfrom
facets#48Conversation
|
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.
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.
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.


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
facetsin the response. I'm able to reproduce when our product is deployed to a new environment and the document count is zero.