Problem/Motivation

Elastic provides the ability for business users to build rule sets in Kibana. These rule sets can be added to the processing of a query using the retriever api.

GET movies/_search		
{
  "retriever": {
    "rule": {
      "match_criteria": {
        "query_string": "harry potter"
      },
      "ruleset_ids": [
        "my-ruleset"
      ],
      "retriever": {
        "standard": {
          "query": {
            "query_string": {
              "query": "harry potter"
            }
          }
        }
      }
    }
  }
}

The standard query is wrapped in a standard retriever, which is again wrapped in a rule retriever.

The implementation challenges for us at this level are that the keys for the key/value pairs in the rule section are user defined, and that data must be duplicated from the inputs in the standard query into the value for these arbitrary keys.

The mapping of views filter values and any static values to user-defined keys can be captured in a configuration entity.

Proposed resolution

  1. Create a configuration entity that collects:
    • The elastic rule set ID.
    • View ID and display ID.
    • Key value pairs for for any search template parameters with static values.
    • Key value pairs for user input in which keys are the template parameter key and values are a filter identifier to use as a data source.
  2. Use hook_views_query_alter to add the search template entity id to the query as an option
  3. Detect the presence of the option in our query process and divert the request to a template api request.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork elasticsearch_connector-3574791

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

fathershawn created an issue. See original summary.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Issue summary: View changes
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Issue summary: View changes
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Initial work will be on this MVP version. Eventually I'd like to incorporate the ability to create and maintain the template itself via Drupal.

fathershawn opened merge request !176

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Active » Needs review
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Tests passing - phstan is not picking up the class alias for Yaml class change in D11 but that's the only issue.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Well I need to add a functional test for the form properly saving and reloading. I tried to "improve" my code to use #config_target which some sources stated to work with config entities, and tried to revert back and missed something. My key-value pairs are not saving.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

False alarm: needed a cache rebuild - Still should probably add a form test :)

mparker17 made their first commit to this issue’s fork.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

c04525d0 - In D11, \Drupal\Core\Serialization\Yaml is an alias of \Drupal\Component\Serialization\Yaml.

Are we supporting D11 only? Asking because in D10 the decode method is only in `\Drupal\Core\Serialization\Yaml` but it's moved into the other class in D11.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

Huh, didn't know that! (aside: then why didn't my change fix phpstan and break phpstan (previous major)??)

AFAIK we want to support both Drupal 10 and Drupal 11.

I guess we need to use \Drupal\Component\Utility\DeprecationHelper::backwardsCompatibleCall() to call the correct class based on which version of Core we're working with.

More information in Changelog 3379306: DeprecationHelper helps modules support multiple versions of core

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Are we supporting D11 only? Asking because in D10 the decode method is only in `\Drupal\Core\Serialization\Yaml` but it's moved into the other class in D11.

I was wrong! The `decode` method in the Core class became an override of the Component class in 10.3 and is removed in 11. Your change was correct!

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

All tests and quality checks passing

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

I'm rebasing and will update tomorrow

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Fixed some minor errors found testing in my site

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Looks like we need more nuanced handling of the default _search body produced by QueryParamBuilder::buildQueryParams to get things where they need to go in a search template.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

I amended the template param builder to put the query key from the standard body params into a drupal_query parameter, and placing an empty match_all query when there is no top level query. This happens if the fulltext box is empty for instance.

I am putting everything else in a drupal_search_other parameter as at least when using retrievers those values need to be first in the template.

fathershawn closed merge request !176

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Title: Support for search templates » Support for rule retriever queries.
Issue summary: View changes
Status: Needs review » Needs work

fathershawn opened merge request !188

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Needs work » Needs review

All tests passing

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

All tests passing and queries behaving as expected in elastic! I'd like to find the best way to integrate these mappings in the views UI and put the retriever config as third-party settings on the view.

Suggestions please!

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Fixed un-selected view data

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Fixed bad data after second form save

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

We need "rank_window_size" >= "from" + "size" since the "rank_window_size" is constraining the # of docs to return.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Adjusted the test to check 'rank_window_size'.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Added a condition for facets

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

I'm in the early stages of reviewing this, so I may have additional feedback, but off the top of my head, I've noticed two potential issues.

First, I noticed that when I switched to the branch, the status report showed...

[Error] Entity/field definitions

Mismatched entity and/or field definitions

The following changes were detected in the entity type and field definitions.

Elastic Rule Retriever

  • The Elastic Rule Retriever entity type needs to be installed.

... running drush -y updatedb did not fix it.

AFAIK this means that the merge request will need an update hook that calls...

if (\is_null(\Drupal::service('entity.definition_update_manager')->getEntityType('elastic_rule_retriever')) {
  \Drupal::service('entity.definition_update_manager')->installEntityType(\Drupal::service('entity_type.manager')->getDefinition('elastic_rule_retriever'));
}

(you could also do this in a a post-update hook)

The other thing I noticed is that there are several references to either elastic_rule_retriever or elastic-rule-retriever.

Regrettably, I would prefer these to be named elasticsearch_connector_retriever / elasticsearch-connector-retriever, in order to keep them properly namespaced (unless there is some technical limitation why they can't be named that (e.g.: because of some internal length limit - but if that's the case, please mention that in the code!)).

I apologize for the verbosity. 🫤

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

The entity type issue may be another good reason to put this in a submodule? Maybe the first retriever in an elasticsearch_connector_retrievers module since there are other retrievers that elastic supports and that would give an expansion point?

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Needs review » Needs work

As discussed in Slack with @fathershawn, the code in this issue relies on a feature that's only available with an Elastic Cloud subscription. This means we cannot test it properly in the main module because we can only spin up an elasticsearch open-source copy in Drupal's CI environment.

Mainly because we cannot test it, @fathershawn and I agreed that this feature would be better in a narrowly-focused submodule. #3554964: Add an event to modify query parameters created by QueryParamBuilder makes that submodule possible.

But now that #3554964: Add an event to modify query parameters created by QueryParamBuilder is in, I'm going to move this back to "Needs work".

Thanks!