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

[Meta] Admin UI Searching #2692

Closed
7 tasks
jesstelford opened this issue Apr 9, 2020 · 10 comments
Closed
7 tasks

[Meta] Admin UI Searching #2692

jesstelford opened this issue Apr 9, 2020 · 10 comments

Comments

@jesstelford
Copy link
Contributor

jesstelford commented Apr 9, 2020

There are a number of suggested improvements / open issues for searching in the Admin UI. This issue to collate & organise them.

@jesstelford
Copy link
Contributor Author

jesstelford commented Apr 9, 2020

I think the first step is to expand #640 so searching will include more than just the name field (see my comment: #640 (comment)):

search across all fields that have text-ish values and combine them in an OR query:

query relationshipSearch($query: String) {
  Users(where: { OR: [
    { id: $query },
    { name_contains_i: $query },
    { bio_contains_i: $query },
    { username_contains_i: $query },
  ] }) {
    id
    _label_
  }
}

That same logic could act as a poor-man's solution to #319 until we have proper full text searching.

Then next we can implement customisable fields (#343) on both the list-level search and the relationship autocomplete search.

Then we can expand upon those things to add searching through nested relationships (#352).

Finally, we can implement full text searching (#319) (the most complex).

Thoughts?

@jesstelford
Copy link
Contributor Author

Here's a mock-up of a solution for #343 for the Relationship Autocomplete case:

Screen Shot 2020-04-09 at 12 48 46 pm

@jesstelford
Copy link
Contributor Author

From @JedWatson

I’ve seen (with keystone 4) valid use cases in our clients where the right answer to searching is “match the ID”

We'd need to ensure we allow a search for ID in relationship autocomplete's too.

@JedWatson
Copy link
Member

😍 love your concept mockup @jesstelford

(to be explicit we'd definitely not want to actually overload the select that much 😅 putting the field pills below the select would work though)

I'd like to propose a first solution here, based on what's worked in Keystone Classic for years:

(prerequisite opinion: we should never default the search to all text-like fields because in certain schemas this is a serious perf foot-gun; the default should lean to less intensive queries until the author explicitly expands the field set being searched)

The search input defaults to

  • the name field if it is defined, and is a Text(-type) field
  • the id field as an exact match otherwise

You can provide a searchFields config on the List which contains one or more paths of Text(-type) fields or false to turn the feature off entirely (this would need support in the Admin UI, and can happen separately)

So if I had the following list:

keystone.createList('User', {
  searchFields: ['name', 'email'],
  fields: {
    name: { type: Text },
    email: { type: Email },
    bio: { type: Text },
  },
});

The search would be have similar to what @jesstelford suggested above:

query relationshipSearch($query: String) {
  Users(where: { OR: [
    { id: $query },
    { name_contains_i: $query },
    { email_contains_i: $query },
  ] }) {
    id
    _label_
  }
}

Given case-sensitive and -insensitive queries can have significantly different performance characteristics we could expand the syntax to allow specifying sensitive/insensitive clauses, but I'd treat that as an enhancement.

When the list is constructed, the searchFields would be validated against the defined fields and we'd throw an error if any of the field paths provided are invalid or not text type fields.

note: I'm not sure we have any way at the moment of easily identifying that a field is compatible with a specific type; which is why I've said "Text(-type)" fields above. Ideally for, e.g an Email field, we would have a way of saying "this is functionally compatible with a Text field". OR some way of knowing which generic input fields are supported in the gqlQueryInputFields implementation, would achieve the same effect here.

@jesstelford
Copy link
Contributor Author

I like it!

I'm not sure we have any way at the moment of easily identifying that a field is compatible with a specific type

In the Admin UI, we could push that to the field itself, allowing it to dictate how to construct the query?

@jesstelford
Copy link
Contributor Author

jesstelford commented Apr 9, 2020

From @timleslie

{ id: $query }, won’t work with a String argument. We’ll have to think about how to manage those, but I’m sure it’s solvable.

Taking my idea of "push that to the field itself", perhaps we could have something like this:

class TextView extends View {
  getGqlSearchInput() {
    return '$textValue: String';
  }

  getGqlSearchVariables(value) {
    return {
      textValue: value
    }
  }

  getGqlSearchWhereClause() {
    return `{ OR: [
      { ${this.path}_startsWith_i: $textValue }
      { ${this.path}_endsWith_i: $textValue }
     ] }`;
  }
}

class IdView extends View {
  getGqlSearchInput() {
    return '$idValue: ID';
  }

  getGqlSearchVariables(value) {
    return {
      idValue: value
    }
  }

  getGqlSearchWhereClause() {
    return `{ ${this.path}: $idValue }`;
  }
}

Then in the AdminUI, it would execute something like:

const uniqueFields = uniq(fields, field => field.type);
const inputs = uniqueFields.map(field => field.getGqlSearchInput()).join(', ');

const where = `{
  OR: [ ${fields.map(field => field.getGqlSearchWhereClause()).join(', ')} ]
}`;

const data = await query(
  `
    query search(${inputs)) {
      all${listName}s(where: ${where}) {
        id
      }
    }
  `,
  variables: Object.assign({}, ...uniqueFields.map(field => field.getGqlSearchVariables(searchTerm))),
);

Which would be the equivalent of:

const data = await query(
  `
    query search($textValue: String, $idValue: ID) {
      allUsers(where: { OR: [
        { id: $idValue }
        { OR: [
          { name_startsWith_i: $textValue }
          { name_endsWith_i: $textValue }
         ] }
      ] }) {
        id
      }
    }
  `,
  variables: { idValue: searchTerm, textValue: searchTerm },
);

@gautamsi
Copy link
Member

gautamsi commented Apr 9, 2020

What about finalising hooks (#2469 and related) which will also let someone create exact search behaviour based on their need.

my customer wants to be able to write free text which would search 4-5 fields and filter result. I have need to generate query which would span multiple fields (indexed) for same text. I had to fork the admin-ui to do that. if hooks support was there people would refactor component they need and substitute.

@MadeByMike had some idea and showed me some extra names he thought about. I believe this is stuck due to not able to finalise hook names.

@JedWatson
Copy link
Member

That does look like a viable solution @jesstelford 👍

In particular, implemented the right way we'd be able to understand when search filters would clash with specific field filters and handle that gracefully in the Admin UI, which at the moment isn't possible.

Another idea I'd like to discuss... having the client intelligent enough to build a search query makes some amount of sense, but I can imagine scenarios where someone with a deep understanding of the back end would want to expose a "blessed" search query method for items in a list.

When originally designing the search argument for Lists, that was part of the thinking; i.e given a specific indexing setup configuration, the API could provide a specific queries for the front-end to use, that canonically answers "this is how you search for things in this list".

Sometimes, encoding the details of that query in the front-end is specifically not what you want.

@jesstelford @timleslie @molomby do you have any particular views on how this would best be achieved in Keystone today? i.e we now support custom queries, and we could potentially also allow List authors to implement custom filter arguments, etc (independently of how we solve Admin UI searching)

I'm pretty convinced that a fair bit of the time it's good for the API to abstract "here's the answer to how you search for things in this list". Having front-end devs needing to know how to set up performant filters in GraphQL queries is a footgun (ref our goal to lead people into the pit of success - doing the right thing is possible, but doing the wrong thing is also very easy) almost to the point where (thinking about it now) it may become important down the track to white-list filters that can be applied on a public API.

Whatever our answer for this is, it doesn't need to be the same as how we implement searching in the Admin UI, but it's something to consider.

@molomby
Copy link
Member

molomby commented May 18, 2020

TL:DR...

  • Search and filtering are very different, please be clear on this
  • We should focus on building good default filtering functionality (using full-text indexes, not chained ORs)
  • We should basically ignore "search"; it's too big/hard/custom for Keystone to do well
  • If someone builds their own solution for "search" it should be easy to use those indexes in place of the default filter functionality in the Admin UI/GraphQL (for specific lists)

I've found it very helpful to make a clear distinction between two different use cases:

The behaviour you've described in this issue is filtering. @jesstelford, I strongly suggest aligning the naming around that term to avoid confusion.

Note, this is about the behaviour you're after and is tangental to the technology used under the hood. Ie. both use cases can (and should!) make use of full-text indexes.

It's also worth reviewing my comments in #319, specifically around how chaining together multiple "or" conditions scales very badly as either fields, items or search terms are added. This approach also isn't indexable on Mongo so can quickly become an outage-causing foot-gun for some systems.

@JedWatson writes:

.. I can imagine scenarios where someone with a deep understanding of the back end would want to expose a "blessed" search query method for items in a list.

Yes and no. Again, the distinction between search and filtering is critical.

Filtering ain't so hard. If we..

  • Take a list of fields
  • Creating a full-text index on them
  • Exposing them as a filter in GraphQL (ie. via the ${listKey}WhereInput type)

Then I think we have the vast majority of filtering use cases covered.

Search on the other hand will probably always be custom. It's such a complex area.. the inclusion of relevance for search means you're dealing with ordering differently, different GraphQL output types, config describing weighting for different fields, etc. Plus, if you're in the kind of system that needs to take search seriously, you're probably dealing with stemming, synonyms, "exact phrases", boolean logic, translations 😱. Whole businesses have sprung up to solve those problems (Algolia, Google, things like Lucene, etc.).

So I agree with Jed in an sense.. If your system needs a proper indexing and full-text search system you're going to need to build or integrate it yourself. But, if you have enough content to justify it and have gone to all the trouble, it'd be a waste to not also use those indexes to power your Admin UI filtering as well.

@stale
Copy link

stale bot commented Sep 15, 2020

It looks like there hasn't been any activity here in over 6 months. Sorry about that! We've flagged this issue for special attention. It wil be manually reviewed by maintainers, not automatically closed. If you have any additional information please leave us a comment. It really helps! Thank you for you contribution. :)

@stale stale bot added the needs-review label Sep 15, 2020
@bladey bladey closed this as completed Apr 8, 2021
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

No branches or pull requests

6 participants