-
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
feat(java): browse objects/synonyms/rules #952
Conversation
✅ Deploy Preview for api-clients-automation canceled.
|
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
@@ -39,17 +39,6 @@ synonymHit: | |||
items: | |||
type: string | |||
description: List of query words that will match the token. | |||
_highlightResult: |
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.
shouldn't it still be there but a string?
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.
I'm afraid in some case it can be the object, as defined here and in that case it would be a mess.
I'm not sure what's best
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.
Maybe worth to go with what we know for now and assume that it can be an enum "synonym" or a free form object?
It should be enough until we have a proper use case to investigate, wdyt?
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.
Would it still be valid once the Java unknown parameter validation is removed?
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.
FAIL_ON_UNKNOWN_PROPERTIES
is already disabled for the client, but it will still fail when trying to deserialize a known property, maybe I could return null instead of throwing but then it's misleading.
Are you sure this property is ever used for synonym ?
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.
for rules we have the same issue, it's listed in the doc but not in our spec
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.
In the JS client, browseRules
and browseSynonyms
both delete the _highlightResult
key for each responses, but not in the searchRules
and searchSynonyms
methods. I'd be fine keeping the same logic until we have some context
maybe @Haroenv (off) @francoischalifour (?) knows more about those?
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.
I don't know, but I would guess that we prefer having consistent keys when using search endpoints to facilitate the data flow in the consumers.
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.
from usage it seems like _highlightResult
is pretty random (example) so we remove it for now
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.
good!
🧭 What and Why
🎟 JIRA Ticket: APIC-616
Implement browse for
object/synonyms/rules
.They are meant to be used with for loop like:
Changes included:
_highlightResult
only contains"synonym"
and never the expected object, so we decided to remove it from the spec.🧪 Test
Playground with the snippet above