Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Define fields to search on at runtime #251

Merged
merged 9 commits into from
Jul 31, 2023

Conversation

ManyTheFish
Copy link
Member

Summary

Update specifications related to the Define fields to search on at runtime feature.


Changes

  • Add the new search parameter definition
  • Add related error messages
  • Update OpenAPI

@ManyTheFish ManyTheFish marked this pull request as ready for review July 3, 2023 09:41
@ManyTheFish ManyTheFish requested a review from macraig July 3, 2023 09:41
@github-actions
Copy link

github-actions bot commented Jul 3, 2023

🚨 Breaking API change detected:

Modified (3)

  • [Breaking] DELETE /indexes/{indexUid}/documents/{documentId}
    • [Breaking] Header removed: Content-Type
    • [Breaking] Body removed
  • POST /indexes/{indexUid}/search
    • Body modified
      • Attribute added: attributesToSearchOn
  • POST /multi-search
    • Body modified
      • Attribute modified: queries

Powered by Bump

@ManyTheFish ManyTheFish changed the title Update 0118-search-api.md Define fields to search on at runtime Jul 3, 2023
@ManyTheFish ManyTheFish added Implemented Feature specification has been implemented. OpenAPI Update OpenAPI specification. v1.3.0 labels Jul 3, 2023
@macraig macraig mentioned this pull request Jul 3, 2023
1 task
Copy link
Contributor

@macraig macraig left a comment

Choose a reason for hiding this comment

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

Can we add telemetry to know when a user is searching using attributesToSearchOn?


- If `attributesToSearchOn` is not set or set to `null`, then the query will search on all `searchableAttributes`.
- Sending the attributes in a different order than the order set in the settings `searchableAttributes` doesn't reorder the fields' rank for the `Attributes` ranking rule
- 🔴 Sending a value with a different type than `Array of String`(POST), `String`(GET) or `null` for `attributesToSearchOn` returns an [invalid_attributes_to_search_on](0061-error-format-and-definitions.md#invalid_search_attributes_to_search_on) error.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you send an empty array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Empty result

Copy link
Contributor

Choose a reason for hiding this comment

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

Should that be specified or is it common practice in the API?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that defining an empty attributesToSearchOn list means that you search in no attributes, which implies no results. Do you see any other behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess not, I was just wondering if it might be surprising for a user to leave it empty and then be confused about getting no results

Copy link
Member

Choose a reason for hiding this comment

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

I'm more on the side of the error if the array is sent with an empty form, so the developer can't break the search and is aware of it; By doing this, we can catch it before they have to understand what's wrong and fix it; I see no point in returning 0 results, am I wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could return the hits as if the query was empty?

Copy link
Member Author

@ManyTheFish ManyTheFish Jul 13, 2023

Choose a reason for hiding this comment

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

I'm more on the side of the error if the array is sent with an empty form, so the developer can't break the search and is aware of it; By doing this, we can catch it before they have to understand what's wrong and fix it; I see no point in returning 0 results, am I wrong?

I agree about the fact that would be useless to set [], however, it's a logical behavior from Meilisearch in my sense. Moreover, it's consistent with the searchableAttributes that accepts an empty array

Maybe we could return the hits as if the query was empty?

It doesn't seems logical to me to replace the search with a placeholder one when attributeToSearchOn is empty. 🤔

Copy link
Member

@gmourier gmourier Jul 13, 2023

Choose a reason for hiding this comment

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

You're right @ManyTheFish; accepting an empty array is an aspect of "consistency" that we could revisit with a v2 eventually because it doesn't seem great for DX (letting the developer put himself in a situation where he/she gets nothing out of the product)

I see it as an edge case personally, so the current implementation works for me, and I'm not against it

cc @macraig (if it helps!)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 @gmourier , let's be consistent and revisit this when we're ready for a v2

@gmourier
Copy link
Member

gmourier commented Jul 5, 2023

Don't we want a telemetry point to see if the feature is used?

@gmourier gmourier added the Ready For Review Feature specification must be reviewed. label Jul 5, 2023
@ManyTheFish
Copy link
Member Author

@gmourier @macraig about telemetry, what about something like:

"attributes_to_search_on": {
    "total_number_of_use": 60,
}

@macraig
Copy link
Contributor

macraig commented Jul 10, 2023

@gmourier @macraig about telemetry, what about something like:

"attributes_to_search_on": {
    "total_number_of_use": 60,
}

@ManyTheFish how does total_number_of_use work?

open-api.yaml Outdated Show resolved Hide resolved
meili-bors bot added a commit to meilisearch/meilisearch that referenced this pull request Jul 13, 2023
3907: Add telemetry for define field to search on at query time r=dureuill a=ManyTheFish

Add "attributes_to_search_on" telemetry usage counter:
```json
"attributes_to_search_on": {
   "total_number_of_use": 12,
},
```

This measures the number of search queries that the user uses `attributesToSearchOn` field.

related to meilisearch/specifications#251

## reviewers:

- `@macraig` for validating the telemetry's name
- `@dureuill` for validating the code

Co-authored-by: ManyTheFish <many@meilisearch.com>
@ManyTheFish ManyTheFish added the Telemetry Update the telemetry collect. label Jul 13, 2023
open-api.yaml Outdated Show resolved Hide resolved
text/0118-search-api.md Outdated Show resolved Hide resolved
text/0118-search-api.md Outdated Show resolved Hide resolved
text/0061-error-format-and-definitions.md Show resolved Hide resolved

- If `attributesToSearchOn` is not set or set to `null`, then the query will search on all `searchableAttributes`.
- Sending the attributes in a different order than the order set in the settings `searchableAttributes` doesn't reorder the fields' rank for the `Attributes` ranking rule
- 🔴 Sending a value with a different type than `Array of String`(POST), `String`(GET) or `null` for `attributesToSearchOn` returns an [invalid_attributes_to_search_on](0061-error-format-and-definitions.md#invalid_search_attributes_to_search_on) error.
Copy link
Member

@gmourier gmourier Jul 13, 2023

Choose a reason for hiding this comment

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

You're right @ManyTheFish; accepting an empty array is an aspect of "consistency" that we could revisit with a v2 eventually because it doesn't seem great for DX (letting the developer put himself in a situation where he/she gets nothing out of the product)

I see it as an edge case personally, so the current implementation works for me, and I'm not against it

cc @macraig (if it helps!)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Implemented Feature specification has been implemented. OpenAPI Update OpenAPI specification. Ready For Review Feature specification must be reviewed. Telemetry Update the telemetry collect. v1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants