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

feat: add multiclusters endpoints for the search client #34

Merged
merged 36 commits into from
Dec 16, 2021

Conversation

MaxenceMaire
Copy link
Contributor

Added multiclusters endpoints for the search client:

  • POST /1/clusters/mapping
  • GET /1/clusters/mapping
  • DELETE /1/clusters/mapping
  • POST /1/clusters/mapping/batch
  • GET /1/clusters/mapping/top
  • GET /1/clusters/mapping/{userID}
  • GET /1/clusters
  • POST /1/clusters/mapping/search
  • GET /1/clusters/mapping/pending

Related doc :

@shortcuts
Copy link
Member

shortcuts commented Dec 13, 2021

Could you merge main and do yarn generate and push the result? It should run the linter

@MaxenceMaire MaxenceMaire changed the title Feat/apic 195/multiclusters endpoints feat: add multiclusters endpoints for the search client Dec 13, 2021
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

(First pass)

Great job! That's a lot :D

Overall there's a few missing description fields and response properties should be required since the engine does not return empty responses.

specs/common/parameters.yml Outdated Show resolved Hide resolved
specs/search/paths/objects/objects.yml Show resolved Hide resolved
specs/search/paths/multiclusters/userIds.yml Outdated Show resolved Hide resolved
specs/search/paths/multiclusters/userIds.yml Outdated Show resolved Hide resolved
specs/search/paths/multiclusters/searchUserIds.yml Outdated Show resolved Hide resolved
specs/search/paths/multiclusters/schemas.yml Outdated Show resolved Hide resolved
specs/search/paths/multiclusters/schemas.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Nice job ! Thanks for your contribution :)

specs/search/common/parameters.yml Outdated Show resolved Hide resolved
specs/search/paths/multiclusters/getTopUserIds.yml Outdated Show resolved Hide resolved
@MaxenceMaire MaxenceMaire requested a review from millotp December 14, 2021 14:08
millotp
millotp previously approved these changes Dec 14, 2021
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

All good for me !

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

that's a lot, gg!

specs/search/paths/search/search.yml Outdated Show resolved Hide resolved
Comment on lines 4 to 5
userID?: HighlightResult;
clusterName?: HighlightResult;
Copy link
Member

Choose a reason for hiding this comment

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

Missing description or regen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we can add a description since they are defined like so:

properties:
  userID:
    $ref: '../../common/schemas/Record.yml#/highlightResult'
  clusterName:
    $ref: '../../common/schemas/Record.yml#/highlightResult'

Sibling elements of $ref are ignored: https://swagger.io/docs/specification/using-ref/#sibling

Also the original documentation doesn't seem to have a description for those: https://www.algolia.com/doc/api-reference/api-methods/search-user-id/#method-response-_highlightresult-object

What do you think?

* List of user object matching the query.
*/
hits: SearchUserIdsResponseHits[];
nbHits: Record<string, any>;
Copy link
Member

Choose a reason for hiding this comment

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

Missing descriptions

Copy link
Contributor Author

@MaxenceMaire MaxenceMaire Dec 16, 2021

Choose a reason for hiding this comment

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

Same as above, sibling elements of $ref are ignored so I'm not sure how to add a description here?

@@ -1,3 +0,0 @@
export type SearchParamsAsString = {
Copy link
Member

Choose a reason for hiding this comment

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

This should not be deleted

Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

I think we can call this one good

@millotp millotp requested a review from shortcuts December 16, 2021 14:27
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.

3 participants