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.

Sign up for GitHub

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
Open

fix: prevents runtime error on undefined facets #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
fix: prevents runtime error on undefined facets 
It is not safe to assume that facets is going to be in the response
  • Loading branch information
@gerardosabetta
gerardosabetta authored and Jerry (Gerardo Sabetta) committed Apr 4, 2023
commit 42f8b51ee1fca0ca8cc4d851eb72895a1643a009
29 changes: 29 additions & 0 deletions fixtures/host-2376rb.api.swiftype.com-443/disjunctive_license_returning_no_facets
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
POST /api/as/v1/engines/node-modules/search.json
authorization: Bearer api-hean6g8dmxnm2shqqiag757a
content-type: application/json
x-swiftype-client: elastic-app-search-javascript
x-swiftype-client-version: 8.7.0
accept: */*
accept-encoding: gzip,deflate
body: {\"query\":\"dog\",\"page\":{\"size\":0},\"filters\":{},\"facets\":{\"license\":[{\"type\":\"value\",\"size\":3}]},\"record_analytics\":false,\"analytics\":{\"tags\":[\"Facet-Only\"]}}

HTTP/1.1 200 OK
date: Thu, 27 Sep 2018 20:36:08 GMT
content-type: application/json; charset=utf-8
transfer-encoding: chunked
connection: close
vary: Accept-Encoding, Origin
status: 200 OK
x-frame-options: SAMEORIGIN
x-xss-protection: 1; mode=block
x-content-type-options: nosniff
etag: W/"d31aa222a68122cb8392577cf4a1a7f1"
cache-control: max-age=0, private, must-revalidate
x-request-id: 91197d45d104a3e2281a60eecec630ff
x-runtime: 0.270027
x-swiftype-frontend-datacenter: dal05
x-swiftype-frontend-node: web01.dal05
x-swiftype-edge-datacenter: dal05
x-swiftype-edge-node: web01.dal05

{"meta":{"warnings":[],"page":{"current":1,"total_pages":0,"total_results":0,"size":1},"request_id":"91197d45d104a3e2281a60eecec630ff"},"results":[]}
29 changes: 29 additions & 0 deletions ...rb.api.swiftype.com-443/search_filter_and_multi_facet_with_disjuntive_returning_no_facets
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
POST /api/as/v1/engines/node-modules/search.json
authorization: Bearer api-hean6g8dmxnm2shqqiag757a
content-type: application/json
x-swiftype-client: elastic-app-search-javascript
x-swiftype-client-version: 8.7.0
accept: */*
accept-encoding: gzip,deflate
body: {\"query\":\"dog\",\"page\":{\"size\":1},\"filters\":{\"license\":\"BSD\"},\"facets\":{\"license\":[{\"type\":\"value\",\"size\":3}],\"dependencies\":[{\"type\":\"value\",\"size\":3}]}}

HTTP/1.1 200 OK
date: Thu, 27 Sep 2018 20:53:56 GMT
content-type: application/json; charset=utf-8
transfer-encoding: chunked
connection: close
vary: Accept-Encoding, Origin
status: 200 OK
x-frame-options: SAMEORIGIN
x-xss-protection: 1; mode=block
x-content-type-options: nosniff
etag: W/"5010d52221547ddec5baceeb9f8a4931"
cache-control: max-age=0, private, must-revalidate
x-request-id: 8369bdda4d5c018ddbd85fc2859fbadb
x-runtime: 0.212519
x-swiftype-frontend-datacenter: dal05
x-swiftype-frontend-node: web01.dal05
x-swiftype-edge-datacenter: dal05
x-swiftype-edge-node: web01.dal05

{"meta":{"warnings":[],"page":{"current":1,"total_pages":0,"total_results":0,"size":10},"request_id":"8369bdda4d5c018ddbd85fc2859fbadb"},"results":[]}
17 changes: 11 additions & 6 deletions src/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,12 +189,17 @@ export default class Client {

return Promise.all([baseQueryPromise, ...disjunctiveQueriesPromises]).then(
([baseQueryResults, ...disjunctiveQueries]) => {
disjunctiveQueries.forEach(disjunctiveQueryResults => {
const [facetName, facetValue] = Object.entries(
disjunctiveQueryResults.facets
)[0];
baseQueryResults.facets[facetName] = facetValue;
});
if (
baseQueryResults.facets &&
Object.keys(baseQueryResults.facets).length
) {
disjunctiveQueries.forEach(disjunctiveQueryResults => {
const [facetName, facetValue] = Object.entries(
disjunctiveQueryResults.facets
)[0];
baseQueryResults.facets[facetName] = facetValue;
});
}
return baseQueryResults;
}
);
Expand Down
18 changes: 18 additions & 0 deletions tests/client.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,24 @@ describe("Client", () => {
});
});

// Fixture: disjunctive_license_returning_no_facets
// Fixture: search_filter_and_multi_facet_returning_no_facets
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 •
edited
Loading

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

@gerardosabetta gerardosabetta Apr 11, 2023

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

@gerardosabetta gerardosabetta Apr 11, 2023

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

@gerardosabetta gerardosabetta Apr 19, 2023

Choose a reason for hiding this comment

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

@joemcelroy any updates?

license: "BSD"
},
facets: {
license: [{ type: "value", size: 3 }],
dependencies: [{ type: "value", size: 3 }]
},
disjunctiveFacets: ["license", "dependencies"]
});

expect(result.info.facets).toBeUndefined();
});

// Fixture: disjunctive_license_with_override_tags
// Fixture: Fixture: search_filter_and_multi_facet_with_tags
it("will accept an alternative analytics tag for disjunctive queries", async () => {
Expand Down