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(clients): make POST body related to the endpoint, assert body in tests #849

Merged
merged 4 commits into from
Jul 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -239,12 +239,17 @@ public RequestBody serialize(Object obj) throws AlgoliaRuntimeException {

if (obj != null) {
try {
content = json.writeValueAsString(obj);
if (obj.getClass().getName().equals("java.lang.Object")) {
content = "{}";
} else {
content = json.writeValueAsString(obj);
}
} catch (JsonProcessingException e) {
throw new AlgoliaRuntimeException(e);
}
} else {
content = null;
// We can't send a null body with okhttp, so we default it to an empty string
content = "";
}

return RequestBody.create(content, MediaType.parse(this.contentType));
Expand Down Expand Up @@ -343,16 +348,10 @@ public Request buildRequest(
processHeaderParams(headerParams, hasRequestOptions ? requestOptions.getExtraHeaders() : null, reqBuilder);

RequestBody reqBody;
if (!HttpMethod.permitsRequestBody(method)) {
// We rely on `permitsRequestBody` to tell us if we should provide a body
// but also set it for DELETE methods
if (!HttpMethod.permitsRequestBody(method) || (method.equals("DELETE") && body == null)) {
reqBody = null;
} else if (body == null) {
if ("DELETE".equals(method)) {
// allow calling DELETE without sending a request body
reqBody = null;
} else {
// use an empty request body (for POST, PUT and PATCH)
reqBody = RequestBody.create("", MediaType.parse(this.contentType));
}
} else {
reqBody = serialize(body);
}
Expand Down
12 changes: 6 additions & 6 deletions clients/algoliasearch-client-javascript/bundlesize.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
"files": [
{
"path": "packages/algoliasearch/dist/algoliasearch.umd.js",
"maxSize": "8.35KB"
"maxSize": "8.30KB"
},
{
"path": "packages/algoliasearch/dist/lite/lite.umd.js",
"maxSize": "3.90KB"
"maxSize": "3.85KB"
},
{
"path": "packages/client-abtesting/dist/client-abtesting.umd.js",
Expand All @@ -18,15 +18,15 @@
},
{
"path": "packages/client-insights/dist/client-insights.umd.js",
"maxSize": "3.85KB"
"maxSize": "3.80KB"
},
{
"path": "packages/client-personalization/dist/client-personalization.umd.js",
"maxSize": "4.00KB"
"maxSize": "3.95KB"
},
{
"path": "packages/client-query-suggestions/dist/client-query-suggestions.umd.js",
"maxSize": "4.05KB"
"maxSize": "4.00KB"
},
{
"path": "packages/client-search/dist/client-search.umd.js",
Expand All @@ -38,7 +38,7 @@
},
{
"path": "packages/predict/dist/predict.umd.js",
"maxSize": "3.90KB"
"maxSize": "3.85KB"
},
{
"path": "packages/recommend/dist/recommend.umd.js",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { EchoResponse, EndRequest, Request, Response } from './types';
import type { EchoResponse, EndRequest, Requester, Response } from './types';

export type EchoRequesterParams = {
getURL: (url: string) => URL;
Expand All @@ -8,7 +8,8 @@ export type EchoRequesterParams = {
function getUrlParams({
host,
searchParams: urlSearchParams,
}: URL): Pick<EchoResponse, 'algoliaAgent' | 'host' | 'searchParams'> {
pathname,
}: URL): Pick<EchoResponse, 'algoliaAgent' | 'host' | 'path' | 'searchParams'> {
const algoliaAgent = urlSearchParams.get('x-algolia-agent') || '';
const searchParams = {};

Expand All @@ -25,39 +26,34 @@ function getUrlParams({
algoliaAgent,
searchParams:
Object.keys(searchParams).length === 0 ? undefined : searchParams,
path: pathname,
};
}

// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
export function createEchoRequester({
getURL,
status = 200,
}: EchoRequesterParams) {
function send(
{ headers, url, connectTimeout, responseTimeout }: EndRequest,
{ data, ...originalRequest }: Request
): Promise<Response> {
const { host, searchParams, algoliaAgent } = getUrlParams(getURL(url));
const originalData =
data && Object.keys(data).length > 0 ? data : undefined;
}: EchoRequesterParams): Requester {
function send(request: EndRequest): Promise<Response> {
const { host, searchParams, algoliaAgent, path } = getUrlParams(
getURL(request.url)
);

const content: EchoResponse = {
...request,
data: request.data ? JSON.parse(request.data) : undefined,
path,
host,
algoliaAgent: encodeURI(algoliaAgent),
searchParams,
};

return Promise.resolve({
content: JSON.stringify({
...originalRequest,
host,
headers,
connectTimeout,
responseTimeout,
algoliaAgent: encodeURI(algoliaAgent),
searchParams,
data: originalData,
}),
content: JSON.stringify(content),
isTimedOut: false,
status,
});
}

return { send };
}

export type EchoRequester = ReturnType<typeof createEchoRequester>;
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,20 @@ export function createTransporter({

async function retryableRequest<TResponse>(
request: Request,
requestOptions: RequestOptions
requestOptions: RequestOptions,
isRead = true
): Promise<TResponse> {
const stackTrace: StackFrame[] = [];
const isRead = request.useReadTransporter || request.method === 'GET';

/**
* First we prepare the payload that do not depend from hosts.
*/
const data = serializeData(request, requestOptions);
const headers = serializeHeaders(baseHeaders, requestOptions);
const method = request.method;
const headers = serializeHeaders(
baseHeaders,
request.headers,
requestOptions.headers
);

// On `GET`, the data is proxied to query parameters.
const dataQueryParameters: QueryParameters =
Expand All @@ -109,6 +112,7 @@ export function createTransporter({
const queryParameters: QueryParameters = {
'x-algolia-agent': algoliaAgent.value,
...baseQueryParameters,
...request.queryParameters,
...dataQueryParameters,
};

Expand Down Expand Up @@ -152,7 +156,7 @@ export function createTransporter({
const payload: EndRequest = {
data,
headers,
method,
method: request.method,
url: serializeUrl(host, request.path, queryParameters),
connectTimeout: getTimeout(timeoutsCount, timeouts.connect),
responseTimeout: getTimeout(timeoutsCount, responseTimeout),
Expand Down Expand Up @@ -236,41 +240,20 @@ export function createTransporter({
}

function createRequest<TResponse>(
baseRequest: Request,
baseRequestOptions: RequestOptions = {}
request: Request,
requestOptions: RequestOptions = {}
): Promise<TResponse> {
const mergedData: Request['data'] = Array.isArray(baseRequest.data)
? baseRequest.data
: {
...baseRequest.data,
...baseRequestOptions.data,
};
const request: Request = {
...baseRequest,
data: mergedData,
};
const requestOptions: RequestOptions = {
cacheable: baseRequestOptions.cacheable,
timeout: baseRequestOptions.timeout,
queryParameters: {
...baseRequest.queryParameters,
...baseRequestOptions.queryParameters,
},
headers: {
Accept: 'application/json',
...baseRequest.headers,
...baseRequestOptions.headers,
},
};

/**
* A read request is either a `GET` request, or a request that we make
* via the `read` transporter (e.g. `search`).
*/
const isRead = request.useReadTransporter || request.method === 'GET';

if (!isRead) {
/**
* On write requests, no cache mechanisms are applied, and we
* proxy the request immediately to the requester.
*/
return retryableRequest<TResponse>(request, requestOptions);
return retryableRequest<TResponse>(request, requestOptions, isRead);
}

const createRetryableRequest = (): Promise<TResponse> => {
Expand All @@ -287,7 +270,7 @@ export function createTransporter({
* request is "cacheable" - should be cached. Note that, once again,
* the user can force this option.
*/
const cacheable = Boolean(requestOptions.cacheable || request.cacheable);
const cacheable = requestOptions.cacheable || request.cacheable;

/**
* If is not "cacheable", we immediately trigger the retryable request, no
Expand All @@ -306,8 +289,8 @@ export function createTransporter({
request,
requestOptions,
transporter: {
queryParameters: requestOptions.queryParameters,
headers: requestOptions.headers,
queryParameters: baseQueryParameters,
headers: baseHeaders,
},
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,14 @@ export function serializeData(

export function serializeHeaders(
baseHeaders: Headers,
requestOptions: RequestOptions
requestHeaders: Headers,
requestOptionsHeaders?: Headers
): Headers {
const headers: Headers = {
Accept: 'application/json',
...baseHeaders,
...requestOptions.headers,
...requestHeaders,
...requestOptionsHeaders,
};
const serializedHeaders: Headers = {};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,9 @@ export type Requester = {
send: (request: EndRequest, originalRequest: Request) => Promise<Response>;
};

export type EchoResponse = Request & {
connectTimeout: number;
host: string;
headers: Headers;
responseTimeout: number;
algoliaAgent: string;
searchParams?: Record<string, string>;
};
export type EchoResponse = Omit<EndRequest, 'data'> &
Pick<Request, 'data' | 'path'> & {
host: string;
algoliaAgent: string;
searchParams?: Record<string, string>;
};
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { createEchoRequester } from '@algolia/client-common';
import type { EchoRequester } from '@algolia/client-common';
import type { Requester } from '@algolia/client-common';

export function echoRequester(status: number = 200): EchoRequester {
export function echoRequester(status: number = 200): Requester {
return createEchoRequester({ getURL: (url: string) => new URL(url), status });
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { URL } from 'url';

import { createEchoRequester } from '@algolia/client-common';
import type { EchoRequester } from '@algolia/client-common';
import type { Requester } from '@algolia/client-common';

export function echoRequester(status: number = 200): EchoRequester {
export function echoRequester(status: number = 200): Requester {
return createEchoRequester({ getURL: (url: string) => new URL(url), status });
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ private function normalize($options)
'headers' => [
'x-algolia-application-id' => $this->config->getAppId(),
'x-algolia-api-key' => $this->config->getAlgoliaApiKey(),
'User-Agent' => $this->config->getAlgoliaAgent() !== null
'User-Agent' =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the php changes can be moved to another PR until @damcou is back

Copy link
Member Author

Choose a reason for hiding this comment

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

The changes for the PHP client here still fixes the POST methods that were not working previously (e.g. browse). However, I've removed the transporter/CTS changes in the meantime

$this->config->getAlgoliaAgent() !== null
? $this->config->getAlgoliaAgent()
: AlgoliaAgent::get($this->config->getClientName()),
'Content-Type' => 'application/json',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,10 @@ public function sendRequest(
*/
$isRead = $useReadTransporter || 'GET' === mb_strtoupper($method);

if ($isRead) {
if ($isRead || 'DELETE' === mb_strtoupper($method)) {
$requestOptions = $this->requestOptionsFactory->createBodyLess(
$requestOptions
);
} elseif ('DELETE' === mb_strtoupper($method)) {
$requestOptions = $this->requestOptionsFactory->createBodyLess(
$requestOptions
);
$data = [];
} else {
$requestOptions = $this->requestOptionsFactory->create(
$requestOptions
Expand Down Expand Up @@ -139,7 +134,9 @@ private function request(
->withQuery($requestOptions->getBuiltQueryParameters())
->withScheme('https');

$body = array_merge($data, $requestOptions->getBody());
$body = isset($data)
? array_merge($data, $requestOptions->getBody())
: $data;

$logParams = [
'body' => $body,
Expand Down Expand Up @@ -277,10 +274,9 @@ private function createRequest(
$protocolVersion = '1.1'
) {
if (is_array($body)) {
// Send an empty body instead of "[]" in case there are
// no content/params to send
// Send an empty valid JSON object
if (empty($body)) {
$body = '';
$body = '{}';
} else {
$body = \json_encode($body, $this->jsonOptions);
if (JSON_ERROR_NONE !== json_last_error()) {
Expand Down
Loading