Skip to content

client.search should take parameters as object, but send as string #528

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

Closed
Haroenv opened this issue May 20, 2022 · 9 comments · Fixed by #531
Closed

client.search should take parameters as object, but send as string #528

Haroenv opened this issue May 20, 2022 · 9 comments · Fixed by #531

Comments

@Haroenv
Copy link
Contributor

Haroenv commented May 20, 2022

Description

Steps to reproduce

  1. Use method searchClient.search
  2. With parameters { requests: [{ indexName: "abc", params: { query: 'something', hitsPerPage: 2 }}]}
  3. See error in browser (400 bad request)

Expected behavior

it should have turned the params into params: "query=something&hitsPerPage=2"

I think some search parameters are more than one level, but then don't get transformed further, just encoded and escaped

Environment

  • OS: [e.g. Windows / Linux / macOS / iOS / Android] /
  • Environment: [e.g. Browser / Node] /
  • Client: [e.g. All / Recommend / Search / Insights] Search
  • Language: [e.g. JavaScript / PHP / Java] JavaScript
  • Client version: [e.g. 5.0.0] 577293e
@shortcuts
Copy link
Member

Thanks!

Do you know if it's also the case with other clients?

@shortcuts
Copy link
Member

Also, it seems to only be for this endpoint, right? The searchSingleIndex method allows sending objects?

@shortcuts
Copy link
Member

Yep we do have that logic in place too but not at this level for this method

@Haroenv
Copy link
Contributor Author

Haroenv commented May 20, 2022

yes, singleIndexSearch doesn't have params, the options are json top-level (verified in https://codesandbox.io/s/gallant-hofstadter-v4ps0v?file=/src/app.js)

@Haroenv
Copy link
Contributor Author

Haroenv commented May 20, 2022

Actually that made me think, and the engine already accepts this (no scoping of params):

  const { results } = await client.search({
    requests: [
      {
        type: 'default',
        indexName: 'instant_search',
        query: 'hi',
        hitsPerPage: 1,
        analyticsTags: ['search!!'],
      },
      {
        type: 'facet',
        indexName: 'instant_search',
        facet: 'brand',
        facetQuery: 'app',
        hitsPerPage: 1,
        analyticsTags: ['facet'],
      },
    ],
  });

@Haroenv
Copy link
Contributor Author

Haroenv commented May 20, 2022

I don't know how to express that in specs, but the correct type is:

import type { SearchParamsObject } from './searchParamsObject';

export type SearchQueries = SearchParamsObject &
  (
    | {
        type: 'facet';
        indexName: string;
        facet: string;
        facetQuery?: string;
      }
    | {
        type?: 'default';
        indexName: string;
      }
  );

PS. this type should be called SearchQuery, not SearchQueries, as it's a single object

@shortcuts
Copy link
Member

Can you open a second issue for #528 (comment) 😇 ? So we can tackle both and make tickets :D

@Haroenv
Copy link
Contributor Author

Haroenv commented May 20, 2022

I think the original issue doesn't need to be solved if #528 (comment) or #529 approach is chosen. That way the parameters are

search({
  requests: [{
    indexName,
    query,
    hitsPerPage,
    params, // as string, if you really still want it
  }]
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants