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

fix: search index spec #8

Merged
merged 15 commits into from
Nov 18, 2021
Merged

fix: search index spec #8

merged 15 commits into from
Nov 18, 2021

Conversation

bodinsamuel
Copy link
Contributor

@bodinsamuel bodinsamuel commented Nov 15, 2021

Spec for /1/indexes/{indexName}/query

todo:

  • Search Params are not correct, curl does not work right now, you can use https://editor.swagger.io/ to test
    Mostly because SearchParams are incorrect
  • Decide if we reflect all HTTP 400 or just BadRequest

@bodinsamuel bodinsamuel self-assigned this Nov 15, 2021
@bodinsamuel
Copy link
Contributor Author

Error I got, you got to love an API that only output one error without context

{
  "message": "Unknown parameter: userData",
  "status": 400
}

{
  "message": "Unknown response field `string`",
  "status": 400
}
{
  "message": "Unknown parameter: attributeForDistinct",
  "status": 400
}

{
  "message": "Value too small for \"minimumAroundRadius\" parameter, expected integer between 1 and 9223372036854775807",
  "status": 400
}
{
  "message": "Invalid value for \"aroundRadius\" parameter, expected integer > 0",
  "status": 400
}

@bodinsamuel
Copy link
Contributor Author

@millotp @damcou @shortcuts not sure I will have time tomorrow (tuesday), do not hesitate to continue this PR.

@shortcuts
Copy link
Member

Decide if we reflect all HTTP 400 or just BadRequest

Do you mean forwarding the message sent by the engine but changing the status code?

@bodinsamuel
Copy link
Contributor Author

Do you mean forwarding the message sent by the engine but changing the status code?

It was more about writing all possible messages and do a oneOf, up to the user to handle them accordingly. But that might be hard for us and users to manage

app.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,85 @@
allowCompressionOfIntegerArray:
Copy link
Member

Choose a reason for hiding this comment

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

Since we already have indexSettings params, 1/indexes/{indexName}/settings should be pretty easy

@bodinsamuel bodinsamuel marked this pull request as ready for review November 17, 2021 09:30
Copy link
Contributor Author

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

I can not approve since I'm the owner but LGTM

openapi_spec/paths/indexes/search.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 work !

openapi_spec/paths/indexes/search.yml Outdated Show resolved Hide resolved
openapi_spec/paths/indexes/search.yml Outdated Show resolved Hide resolved
openapi_spec/schemas/Record.yml Outdated Show resolved Hide resolved
openapi_spec/schemas/Record.yml Outdated Show resolved Hide resolved
openapi_spec/schemas/Record.yml Outdated Show resolved Hide resolved
/**
* A single record
*/
export class Record extends null<String, object> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get where this extends null is coming from but it's breaking the js, do you know how it got here ?

Copy link
Member

Choose a reason for hiding this comment

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

Good question 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't forget to commit the build if you have found the solution

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't find where this is coming from, it's just every other model but it adds a parent to this one

Copy link
Member

Choose a reason for hiding this comment

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

additionalProperties: true adds this weird part, I did not find a solution yet 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Actually, modelError also have this type thing https://github.com/algolia/api-client-automation-experiment/blob/main/output/model/modelError.ts so it might be related to the generator template itself

@shortcuts shortcuts requested a review from millotp November 17, 2021 15:52
@shortcuts
Copy link
Member

Updated a bunch of things, might need an other review @damcou @bodinsamuel @millotp

@shortcuts shortcuts requested a review from damcou November 17, 2021 16:17
Comment on lines +10 to +17
_highlightResult:
$ref: '#/highlightResult'
_snippetResult:
$ref: '#/snippetResult'
_rankingInfo:
$ref: '#/rankingInfo'
_distinctSeqID:
type: number
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, we should investigate if we can influence this response object with request param.
e.g: _rankingInfo is only return when getRankingInfo is true

Copy link
Member

Choose a reason for hiding this comment

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

True, it was implemented as all optional for now but we should indeed make it clearer.

Should we consider it in this PR?

Copy link
Contributor Author

@bodinsamuel bodinsamuel Nov 17, 2021

Choose a reason for hiding this comment

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

no but we can note it in next investigation ☺️

/**
* A single record
*/
export class Record extends null<String, object> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't forget to commit the build if you have found the solution

output/model/searchResponse.ts Show resolved Hide resolved
@shortcuts shortcuts self-assigned this Nov 18, 2021
Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

Seems to be working now that main was merged.
If everything is good on your side, we can merge.

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.

4 participants