Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gerardosabetta
Copy link

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.

{"meta":{"alerts":[],"warnings":[],"precision":3,"page":{"current":1,"total_pages":1,"total_results":0,"size":0},"engine":{"name":"assessments-onprem","type":"default"},"request_id":"0c98a347-a054-49f2-b8dd-f433449791c7"},"results":[]}

@joemcelroy
Copy link
Member

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
@gerardosabetta
Copy link
Author

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: {
Copy link
Member

@joemcelroy joemcelroy Apr 11, 2023

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.

Copy link
Author

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.

Copy link
Author

Choose a reason for hiding this comment

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

The behavior doesn't reproduce by modifying the request as you suggested. It produces a different error:

image

Because Facet-Only request goes out to fetch the disjunctive "license" tag
image

Copy link
Author

Choose a reason for hiding this comment

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

@joemcelroy any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants