-
Notifications
You must be signed in to change notification settings - Fork 10
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): allow POST
methods to send read
requests
#525
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 |
b20ebf4
to
b98e6e0
Compare
0bae151
to
7a8718c
Compare
if (useReadTransporter) { | ||
reqBuilder.tag(new UseReadTransporter()); | ||
} |
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 is the only difference, the rest is indenting sorry
@@ -401,18 +412,4 @@ public class ApiClient { | |||
} | |||
} | |||
} | |||
|
|||
/** |
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.
It was not used, could you confirm?
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 concur
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.
Ok my lord
return $this->api->sendRequest( | ||
$method, | ||
$resourcePath . ($query ? "?{$query}" : ''), | ||
$httpBody, | ||
$requestOptions, | ||
$useReadTransporter | ||
); |
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.
Since there was basically no difference between both, I've made the conditional stuff in the sendRequest
method, which reduces the duplication
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.
flawless !
@@ -401,18 +412,4 @@ public class ApiClient { | |||
} | |||
} | |||
} | |||
|
|||
/** |
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 concur
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.
On a coding POV, this looks fine (and the CI seems to approve that). I'm just a little bit confused with the fact that we should use GET
over POST
for these methods. Is this DSN thing new ?
My only issue is that it's not controlled on the CI for PHP and Java yet (client tests), so if you can please test via the plaground :D
Looking at the current clients, that's what we are already doing, we leverage the edit: Not all READ methods are concerned, just a few |
.../algoliasearch-client-javascript/packages/client-common/src/transporter/createTransporter.ts
Outdated
Show resolved
Hide resolved
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.
seems correct!
🧭 What and Why
🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-495
Changes included:
Some
POST
methods of the REST API are sent using theread
requester in the current clients.🧪 Test
CI :D