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

hotfix: only send required parameters for search #37

Merged

Conversation

eskombro
Copy link
Member

@eskombro eskombro commented Jun 4, 2020

As for now, search requests are sending every possible parameter, even those that are unused. For example, searching with a query "term" and a limit of 10 build the following request:

GET /indexes/:indexUid/search?attributesToCrop=&attributesToHighlight=&attributesToRetrieve=&cropLength=0&filters=&limit=10&matches=false&offset=0&q=term

This PR modifies this behavior with a hotfix, as this will be broken with the v0.11. That request would be built as:

GET /indexes/:indexUid/search?limit=10&q=term

I call it hotfix because the goal of this PR is to make it compatible as fast as possible, but we might want to find a better and cleaner solution for this

@eskombro eskombro changed the base branch from master to bump-meilisearch-v0.11 June 4, 2020 12:26
@eskombro eskombro requested review from alexisvisco and curquiza June 4, 2020 12:28
@eskombro eskombro force-pushed the hotfix_send_only_required_search_params branch from 5cc1fc0 to f468478 Compare June 4, 2020 12:32
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

LGTM! No missing query parameter!

@eskombro eskombro force-pushed the hotfix_send_only_required_search_params branch from f468478 to af86a99 Compare June 4, 2020 12:40
@eskombro eskombro merged commit 0b49c2e into bump-meilisearch-v0.11 Jun 4, 2020
@eskombro eskombro deleted the hotfix_send_only_required_search_params branch June 4, 2020 12:41
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.

2 participants