Skip to content
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

feature/ using searchQuery object as a search layer + GraphQl support #1616

Merged
merged 80 commits into from
Sep 12, 2018

Conversation

yuriboyko
Copy link
Contributor

@yuriboyko yuriboyko commented Aug 21, 2018

Related issues

no related issues

Related pull requests

vuestorefront/vue-storefront-api#94

Short description and why it's useful

This update applies 2 big features:

  1. New search library which uses searchQuery object as a search layer with ability to apply filters and search text instead of creating Elasticsearch request from the beginning. This will allow VSF be more flexible in terms of using different search engines

  2. New search adapter uses graphQl as an API

Screenshots of visual changes before/after (if there are any)

Visually no changes

Screenshot of passed e2e tests (if you are using our standard setup as a backend)

add-to-cart
add-to-compare
basic-client-path
category-page
checkout-page
home-page
local-storage
login-path
product-page
register-path
search-result

Contribution and curently important rules acceptance

yuriboyko and others added 30 commits July 13, 2018 11:30
…update graphql query and searchQuery object methods clean up the code
…torefront into MTH-250

# Conflicts:
#	core/store/lib/search/search.js
# Conflicts:
#	core/store/modules/category/actions.ts
#	core/store/modules/product/actions.ts
# Conflicts:
#	core/components/blocks/SearchPanel/SearchPanel.js
#	core/store/lib/search.js
#	core/store/modules/product/actions.ts
…ssues with filtering by text fields with keyword subfields
… the code

# Conflicts:
#	core/store/helpers/index.ts
#	core/store/lib/search.ts
#	core/store/modules/attribute/actions.ts
#	core/store/modules/category/actions.ts
#	core/store/modules/product/actions.ts
#	core/store/modules/tax/actions.ts
#	src/themes/default/components/core/blocks/Product/Related.vue
#	src/themes/default/components/theme/blocks/Collection/Collection.vue
#	src/themes/default/pages/Home.vue
@pkarw
Copy link
Collaborator

pkarw commented Sep 9, 2018

@yuriboyko Please do resolve conflicts + let me know when it’s ready to be merged in. I’ll merge it in next week into develop

# Conflicts:
#	CONTRIBUTING.md
#	core/app.ts
#	core/pages/PageNotFound.js
#	core/store/lib/search.ts
#	core/store/modules/category/actions.ts
#	package.json
#	yarn.lock
@pkarw
Copy link
Collaborator

pkarw commented Sep 11, 2018

@yuriboyko - is it ready togo?

@yuriboyko
Copy link
Contributor Author

I think yes

core/store/helpers/index.ts Show resolved Hide resolved
query = query.aggregation('terms', attrToFilter.field + optionsPrfeix)
} else {
query = query.aggregation('terms', attrToFilter.field)
query.aggregation('range', 'price', {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it be final_price either - line 44 replacement removed - to just operate on the same field?

const queryBody = query.build()

return queryBody
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we should have a more general way to create full-text queries. I mean it would be great if the SearchQuery class can support constructing the text queries like this one to not have this logic centralized as it currently is

let queryVariables = {}
const filters = Query.searchQuery.getAppliedFilters()

switch (Query.type) {
Copy link
Collaborator

@pkarw pkarw Sep 12, 2018

Choose a reason for hiding this comment

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

This is important

We must add the way to extend this logick - I belive there should be a way to extend the graphQL queries support for new entities without modifying the core

Would be great if from the Extension or Theme level developer could do sth like:

const factory = new SearchAdapterFactory()
factory.registerEntityType('graphql', 'category', { 
 gql: require('./queries/categories.gql'),
 queryProcessor: (query) => {
  // function that can modify the query each time before it's being executed
  return query
 }
 resultPorcessor: (resp, start, size) =>  {
  const response = {
    items: map(resp.hits.hits, hit => {
      return Object.assign(hit._source, {
        _score: hit._score,
        slug: (hit._source.hasOwnProperty('url_key') && rootStore.state.config.products.useMagentoUrlKeys)
          ? hit._source.url_key
          : (hit._source.hasOwnProperty('name') ? slugify(hit._source.name) + '-' + hit._source.id : '')
      }) // TODO: assign slugs server side
    }), // TODO: add scoring information
    total: resp.hits.total,
    start: start,
    perPage: size,
    aggregations: resp.aggregations
  }

})

for the api adapter it could be sth like this:

const factory = new SearchAdapterFactory()
factory.registerEntityType('api', 'category', { 
 queryProcessor: (query) => {
  // function that can modify the query each time before it's being executed
  return query
 }
 resultPorcessor: (resp, start, size) =>  {
  const response = {
    items: map(resp.hits.hits, hit => {
      return Object.assign(hit._source, {
        _score: hit._score,
        slug: (hit._source.hasOwnProperty('url_key') && rootStore.state.config.products.useMagentoUrlKeys)
          ? hit._source.url_key
          : (hit._source.hasOwnProperty('name') ? slugify(hit._source.name) + '-' + hit._source.id : '')
      }) // TODO: assign slugs server side
    }), // TODO: add scoring information
    total: resp.hits.total,
    start: start,
    perPage: size,
    aggregations: resp.aggregations
  }

})

So we should have store the processor, queryProcessor and gql somewhere per entity type * driver. I think that good place is a kind of Vue.prototype.$searchContext OR rootStore.state - up to You

if (excludeFields) esQuery._sourceExclude = excludeFields
if (includeFields) esQuery._sourceInclude = includeFields
if (excludeFields) Request._sourceExclude = excludeFields
if (includeFields) Request._sourceInclude = includeFields
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is important

We must ensure that graphQL driver is taking care of _sourceInclude and _sourceExclude as they are key to optimize the bandwidth of data requests:

we’re running two queries in that case - here You have the result on Elastic (demo): (https://www.dropbox.com/s/acu4l0f868ovndf/Screenshot%202018-09-11%2017.31.23.png?dl=0)

and in graphql - the result is exactly the same in size because _excludeFields parameter is not being used (https://www.dropbox.com/s/znwfqrbvfn8llas/Screenshot%202018-09-11%2017.30.41.png?dl=0

In first case you see the difference: 123 vs 320kb - in second case the excludeFields are ignored

@pkarw pkarw merged commit 4cbf866 into vuestorefront:develop Sep 12, 2018
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