-
Notifications
You must be signed in to change notification settings - Fork 14
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(specs): search w/ hits & facets responses #1774
Conversation
✅ Deploy Preview for api-clients-automation ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
3b06ef2
to
e77baee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks super cool as a workaround!
it's sad we can't une the discriminator field, is it because we need to specify the type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed, the usual approach is to examine the value of the field, and from there, deduce its type. Unfortunately, we can't do that in this case. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the type was on its one and we pass it with a ref? would it be possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give more detail? by pass in it with a ref, you mean in the specs ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but I guess here it's a matter of generation instead
x-discriminator-fields: | ||
- facetHits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering our use of this field, can't we rely on the built-in one?
discriminator:
propertyName: facetHits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regrettably, we can't rely on the built-in one in this context. The notion of a discriminator is to include type information within the object, such as type: SearchForFacetsResponse
, but that's not present in our responses. In this case, we're checking for the existence of certain field(s), not their types. Furthermore, in anticipation of a day when proper type information will be included in the responses, it would be best to keep the semantics separate and reserve the discriminator for when the relevant fields become available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:( oki as long as it works let's go with that and try to implement it in other clients too if possible!
the generator refactor might help with unifying all of that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome :D
🧭 What and Why
Multi search endpoint support conducting both 'hits' and 'facets' search operations within the same call. Although this is already outlined in the specifications for requests, it is not yet included for responses.
Changes included:
Update the endpoint to support both 'hits' and 'facets' for requests as well as responses.