-
Notifications
You must be signed in to change notification settings - Fork 153
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
ECP-469: Finalize new search schema #368
Conversation
- Added stats bucket & range filter
- fixed range type to input - added support for custom sorting
- search direction is mandatory
- Added suggestions
Can I ask you to describe scenario that will be covered by this API? |
- Added attribute name to aggregations
- Fixed underscores
input ProductSearchSortInput | ||
{ | ||
attribute: String! | ||
direction: SortEnum! |
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.
why should we have direction as a mandatory attribute?
Most of RDBMS just use a default value of ASC / DESC
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.
Here is official elastic search documentation:
The order defaults to desc when sorting on the _score, and defaults to asc when sorting on anything else.
I don't like this uncertainty
ProductSearchSortInput
is optional so we can apply some default order, but if developer asks for custom ordering I think it's better to ask for direction as well.
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 like @maghamed's idea of having a default. If something isn't critical for an operation to succeed, (I think) it should be optional. Otherwise querying becomes tedious and verbose.
I don't like this uncertainty
I don't see much uncertainty from that quote, if we're only talking about ES.
But, are we planning to only support Elastic Search? If we're ever going to swap search providers, I think it would make sense for the application to define the default so it's consistent.
ProductSearchSortInput is optional so we can apply some default order
I think an application-defined default sort order would be great 👍
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 think there is no need for any changes. We have the optional ProductSearchSortInput
, so we can apply default sort. However, the user needs to specify direction for every custom sort. Does it work?
Would also like to see this, with a few example queries showing how the front-end would cover the scenarios |
- Removed attribute field duplicate from aggregations
- Various fixes
- Added suggestions
- Various fixes
- Removed multi-search
currentPage: Int = 1, | ||
filter: [SearchClauseInput], | ||
sort: [ProductSearchSortInput] | ||
): ProductSearchResponse! |
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.
productSearch
should be nullable, or all other operations under Query
in a request will fail. See #372
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 table with old-to-new query mapping says we preserve products
query with filtering by ID. In the example below, the query is called product(id)
.
} | ||
|
||
input SearchClauseInput { | ||
attribute_code: String! |
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.
Inconsistent field name, attribute_code
here vs. attribute
on other type.
|
||
interface Bucket { | ||
#Human readable bucket title | ||
title: String! |
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.
All fields under the interface (e.g. Bucket) must be listed under any type that implements the interface. e.g. I don't see title
listed under StatsBucket, ScalarBucket and RangeBucket.
type Highlight { | ||
attribute: String! | ||
value: String! | ||
matched_words: [String]! |
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.
It is a common convention to use camelCase for field name (e.g. matchedWords instead of matched_words). Not sure if this is Magento convention or else.
count: Int! | ||
} | ||
|
||
interface Aggregation { |
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.
Is this supposed to be a type
instead of an interface
?
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 think we should add title
field for storing aggregation friendly name. This is so the client (e.g. UI) can use it for display, instead of displaying attribute code.
type ProductSearchResponse { | ||
items: [ProductSearchItem] | ||
facets: [Aggregation] | ||
facets_values: [Aggregation] |
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.
As discussed with @kokoc verbally, I think facets
is sufficient for storing facets aggregation and I am not clear the use case for facet_values
.
Posted the following question earlier in Slack:
And @kokoc responded to me that this should be covered by this new search api and gave me the possibility to actually check it out! 🎉 So I've been trying out the proof-of-concept graphql endpoint (https://search-api-dev.magento-ds.com/graphql) and have some feedback / questions:
|
Oh, I forgot something, Shouldn't ScalarBucket be have additional implementations like ImageBucket, ColorBucket and TextBucket to account for swatch information? |
Overview
This PR contains graphql schema changes required to introduce new search functionality such as
The main challenge is to find a proper place in existing/new queries for new fields.
Inject into one of the existing types/queries
products
queryproducts
query covers two main use cases: search by search phrase and catalog browsing with filtering by category.This query is the most obvious choice for search-related customizations.
However, this query returns a
Products
type withProductInterface
inside. TheProductInterface
is designed torepresent a product entity with complex product types support.
Adding fields to
ProductInterface
orProducts
willmake those fields optional and useless in "catalog browsing" scenario. Other words, our query is handling two different
use cases with two different results. Mixing those results in very common
ProductInterface
is messy,inconvenient and just strange.
Products
type is a little bit better place for new fields. We can put request-specific fields here, but we also need to introduce one more items conainer forsearch results(will contain highlights + products). This approach is still messy, but at least it doesn't affect other product usecases (shopping cart item, wishlist item, pdp, etc).
Example for this option:
other queries with product results
There are two more queries which return products and could be considered as a potential candidate for new fields:
category and categoryList. Both return
CategoryInterface
interface withproducts
field inside:This interface also returns
ProductInterface
and also those queries do not receive a search phrase. Thus it's they are not a good candidates for new search functionality.Proper place
In order to find the proper place we may take a look on the use cases we have and want to support:
I think the following table is the desired assigment of queries per use cases:
category
categoryList
Add filters to product sub-request
products
queryProbably can be omitted
You can find a possible shape of new query in PR content.
From the table above we can see that
products
query will be used mostly for retrieving the product by their ids, other functionality will be transferred toproductSearch
query.Thus we may also deprecate entire
products
query entirely and create a new query designed to get products by ids.Here is a possible shape of the api if we decide to deprecated old query:
Here are few possible examples per use case:
Category Listing
Search by phrase
Product details page
Category navigation