Skip to content

feat(cts): generate tests for helpers #2798

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

Merged
merged 27 commits into from
Jun 17, 2024
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 @@ -3,13 +3,14 @@
using System.Text.Json.Serialization;
using Algolia.Search.Http;
using Algolia.Search.Utils;
using System.Collections.Generic;

namespace Algolia.Search.Models.Search;

/// <summary>
/// Secured Api Key restrictions
/// </summary>
public partial class SecuredAPIKeyRestrictions
public partial class SecuredApiKeyRestrictions
{

/// <summary>
Expand All @@ -18,28 +19,26 @@ public partial class SecuredAPIKeyRestrictions
/// <returns></returns>
public string ToQueryString()
{
string restrictionQuery = null;
var restrictions = ToQueryMap(this, nameof(SearchParams));
if (SearchParams != null)
{
restrictionQuery = ToQueryString(SearchParams);
// merge SearchParams into restrictions
restrictions = restrictions.Concat(ToQueryMap(SearchParams)).ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
}

var restrictions = ToQueryString(this, nameof(SearchParams));
var array = new[] { restrictionQuery, restrictions };

return string.Join("&", array.Where(s => !string.IsNullOrEmpty(s)));
return QueryStringHelper.ToQueryString(restrictions.OrderBy(x => x.Key).ToDictionary(kvp => kvp.Key, kvp => kvp.Value));
}

/// <summary>
/// Transform a poco to a query string
/// Transform a poco to a map of query parameters
/// </summary>
/// <param name="value"></param>
/// <param name="ignoreList"></param>
/// <typeparam name="T"></typeparam>
/// <returns></returns>
private static string ToQueryString<T>(T value, params string[] ignoreList)
private static Dictionary<string, string> ToQueryMap<T>(T value, params string[] ignoreList)
{
var properties = typeof(T).GetTypeInfo()
return typeof(T).GetTypeInfo()
.DeclaredProperties.Where(p =>
p.GetValue(value, null) != null && !ignoreList.Contains(p.Name) &&
p.GetCustomAttribute<JsonPropertyNameAttribute>() != null)
Expand All @@ -48,8 +47,6 @@ private static string ToQueryString<T>(T value, params string[] ignoreList)
propsName = p.GetCustomAttribute<JsonPropertyNameAttribute>().Name,
value = QueryStringHelper.ParameterToString(p.GetValue(value, null))
}).ToDictionary(p => p.propsName, p => p.value);

return properties.ToQueryString();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ public IEnumerable<SynonymHit> BrowseSynonyms(string indexName, SearchSynonymsPa
/// <param name="parentApiKey">Parent API Key</param>
/// <param name="restriction">Restriction to add the key</param>
/// <returns></returns>
public string GenerateSecuredApiKey(string parentApiKey, SecuredAPIKeyRestrictions restriction)
public string GenerateSecuredApiKey(string parentApiKey, SecuredApiKeyRestrictions restriction)
{
var queryParams = restriction.ToQueryString();
var hash = HmacShaHelper.GetHash(parentApiKey, queryParams);
Expand All @@ -273,20 +273,20 @@ public string GenerateSecuredApiKey(string parentApiKey, SecuredAPIKeyRestrictio


/// <summary>
/// Get the remaining validity of a key generated by `GenerateSecuredApiKeys`.
/// Get the remaining validity of a key generated by `GenerateSecuredApiKey`.
/// </summary>
/// <param name="securedAPIKey">The secured API Key</param>
/// <param name="securedApiKey">The secured API Key</param>
/// <returns></returns>
/// <exception cref="ArgumentNullException"></exception>
/// <exception cref="AlgoliaException"></exception>
public TimeSpan GetSecuredApiKeyRemainingValidity(string securedAPIKey)
public TimeSpan GetSecuredApiKeyRemainingValidity(string securedApiKey)
{
if (string.IsNullOrWhiteSpace(securedAPIKey))
if (string.IsNullOrWhiteSpace(securedApiKey))
{
throw new ArgumentNullException(nameof(securedAPIKey));
throw new ArgumentNullException(nameof(securedApiKey));
}

var decodedKey = Encoding.UTF8.GetString(Convert.FromBase64String(securedAPIKey));
var decodedKey = Encoding.UTF8.GetString(Convert.FromBase64String(securedApiKey));

var regex = new Regex(@"validUntil=\d+");
var matches = regex.Matches(decodedKey);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,14 @@ export function serializeUrl(
}

export function serializeQueryParameters(parameters: QueryParameters): string {
const isObjectOrArray = (value: any): boolean =>
Object.prototype.toString.call(value) === '[object Object]' ||
Object.prototype.toString.call(value) === '[object Array]';

return Object.keys(parameters)
.filter((key) => parameters[key] !== undefined)
.sort()
.map(
(key) =>
`${key}=${encodeURIComponent(
isObjectOrArray(parameters[key])
? JSON.stringify(parameters[key])
Object.prototype.toString.call(parameters[key]) === '[object Array]'
? parameters[key].join(',')
Comment on lines +51 to +52
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this will break something

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it did break something, it wasn't working before with generateSecuredApiKey, but you agree with me that never ever do we have a query param that looks like a serialezed object right ? we only allow primitives and array to stringified

Copy link
Member

Choose a reason for hiding this comment

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

Yup but IIRC this implem was exactly the same as the legacy client but since I never found the reason why it had an object case :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay but we don't have any tests that expect that, and we don't have the same behavior in other languages, this was an outlier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can revert this if you want, but I think this is more correct

Copy link
Member

Choose a reason for hiding this comment

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

I also think so but since it worked like that for years I think there's an edge case we are not aware of :/ we can leave it and see if there's complains

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reverted in b9ff92c

Copy link
Collaborator Author

@millotp millotp Jun 14, 2024

Choose a reason for hiding this comment

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

ah bah it breaks the test because now it adds brackets to stringified array, thats why I had to put the .join(',')

Copy link
Member

Choose a reason for hiding this comment

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

I mostly wonder how it could've been wrong for 4 years in the current JS client, maybe the engine supports this format?

Your initial solution is indeed correct to me, we can go with it and if user complain we fix it (we can also for example try it on the Crawler migration PoC too)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good, the PR should be fine then

: parameters[key]
).replaceAll('+', '%20')}`
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -344,13 +344,13 @@ public suspend fun <T> SearchClient.replaceAllObjects(
/**
* Generate a virtual API Key without any call to the server.
*
* @param parentAPIKey API key to generate from.
* @param parentApiKey API key to generate from.
* @param restriction Restriction to add the key
* @throws Exception if an error occurs during the encoding
*/
public fun SearchClient.generateSecuredApiKey(parentAPIKey: String, restriction: SecuredAPIKeyRestrictions): String {
public fun SearchClient.generateSecuredApiKey(parentApiKey: String, restriction: SecuredApiKeyRestrictions): String {
val restrictionString = buildRestrictionString(restriction)
val hash = encodeKeySHA256(parentAPIKey, restrictionString)
val hash = encodeKeySHA256(parentApiKey, restrictionString)
return "$hash$restrictionString".encodeBase64()
}

Expand All @@ -359,7 +359,7 @@ public fun SearchClient.generateSecuredApiKey(parentAPIKey: String, restriction:
*
* @param apiKey The secured API Key to check.
* @return Duration left before the secured API key expires.
* @throws IllegalArgumentException if [apiKey] doesn't have a [SecuredAPIKeyRestrictions.validUntil].
* @throws IllegalArgumentException if [apiKey] doesn't have a [SecuredApiKeyRestrictions.validUntil].
*/
public fun securedApiKeyRemainingValidity(apiKey: String): Duration {
val decoded = apiKey.decodeBase64String()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@ package com.algolia.client.extensions.internal

import com.algolia.client.api.SearchClient
import com.algolia.client.model.search.SearchParamsObject
import com.algolia.client.model.search.SecuredAPIKeyRestrictions
import com.algolia.client.model.search.SecuredApiKeyRestrictions
import io.ktor.http.*
import kotlinx.serialization.json.JsonArray
import kotlinx.serialization.json.jsonObject
import kotlinx.serialization.json.jsonPrimitive

/**
* Builds a restriction string based on provided [SecuredAPIKeyRestrictions].
* Builds a restriction string based on provided [SecuredApiKeyRestrictions].
*/
internal fun SearchClient.buildRestrictionString(restriction: SecuredAPIKeyRestrictions): String {
internal fun SearchClient.buildRestrictionString(restriction: SecuredApiKeyRestrictions): String {
return Parameters.build {
restriction.searchParams?.let { searchParams ->
val json = options.json.encodeToJsonElement(SearchParamsObject.serializer(), searchParams).jsonObject
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package com.algolia.client

import com.algolia.client.api.SearchClient
import com.algolia.client.extensions.SecuredAPIKeyRestrictions
import com.algolia.client.extensions.SecuredApiKeyRestrictions
import com.algolia.client.extensions.generateSecuredApiKey
import com.algolia.client.extensions.securedApiKeyRemainingValidity
import com.algolia.client.model.search.SearchParamsObject
Expand All @@ -14,14 +14,14 @@ class TestSecureApiKey {

@Test
fun securedApiKey() {
val parentAPIKey = "SearchOnlyApiKeyKeptPrivate"
val restriction = SecuredAPIKeyRestrictions(
val parentApiKey = "SearchOnlyApiKeyKeptPrivate"
val restriction = SecuredApiKeyRestrictions(
query = SearchParamsObject(filters = "_tags:user_42"),
validUntil = Clock.System.now() + 2.days,
)

val client = SearchClient("appId", "apiKey")
val securedApiKey = client.generateSecuredApiKey(parentAPIKey, restriction)
val securedApiKey = client.generateSecuredApiKey(parentApiKey, restriction)
val validity = securedApiKeyRemainingValidity(securedApiKey)
assertTrue { validity > 1.days }
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from json import dumps
from typing import Any, Dict
from urllib.parse import urlencode

PRIMITIVE_TYPES = (float, bool, bytes, str, int)

Expand All @@ -12,21 +13,30 @@ class QueryParametersSerializer:
query_parameters: Dict[str, Any] = {}

def parse(self, value) -> Any:
if isinstance(value, dict):
return dumps(value)
elif isinstance(value, list):
if isinstance(value, list):
Copy link
Member

Choose a reason for hiding this comment

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

does list validates dict? that's why you changed the order?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same thing as for js, we never want to stringify a dict

return ",".join([self.parse(item) for item in value])
elif isinstance(value, dict):
return dumps(value)
elif isinstance(value, bool):
return "true" if value else "false"
else:
return str(value)

def encoded(self) -> str:
return urlencode(
dict(sorted(self.query_parameters.items(), key=lambda val: val[0]))
).replace("+", "%20")

def __init__(self, query_parameters: Dict[str, Any]) -> None:
self.query_parameters = {}
if query_parameters is None:
return
for key, value in query_parameters.items():
self.query_parameters[key] = self.parse(value)
if isinstance(value, dict):
for dkey, dvalue in value.items():
self.query_parameters[dkey] = self.parse(dvalue)
else:
self.query_parameters[key] = self.parse(value)


def bodySerializer(obj: Any) -> dict:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -526,18 +526,18 @@ public extension SearchClient {
/// - returns: String?
func generateSecuredApiKey(
parentApiKey: String,
with restriction: SecuredAPIKeyRestrictions = SecuredAPIKeyRestrictions()
with restriction: SecuredApiKeyRestrictions = SecuredApiKeyRestrictions()
) throws -> String? {
let queryParams = try restriction.toURLEncodedString()
let hash = queryParams.hmac256(withKey: parentApiKey)
return "\(hash)\(queryParams)".data(using: .utf8)?.base64EncodedString()
}

/// Get the remaining validity of a secured API key
/// - parameter securedAPIKey: The secured API key
/// - parameter securedApiKey: The secured API key
/// - returns: TimeInterval?
func getSecuredApiKeyRemainingValidity(for securedAPIKey: String) -> TimeInterval? {
guard let rawDecodedAPIKey = String(data: Data(base64Encoded: securedAPIKey) ?? Data(), encoding: .utf8),
func getSecuredApiKeyRemainingValidity(for securedApiKey: String) -> TimeInterval? {
guard let rawDecodedAPIKey = String(data: Data(base64Encoded: securedApiKey) ?? Data(), encoding: .utf8),
!rawDecodedAPIKey.isEmpty else {
return nil
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
#endif
import Foundation

public extension SecuredAPIKeyRestrictions {
public extension SecuredApiKeyRestrictions {
func toURLEncodedString() throws -> String {
var queryDictionary: [String: Any] = [:]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,22 @@ public void run(Map<String, CodegenModel> models, Map<String, CodegenOperation>
throw new CTSException("Cannot find operation for method: " + step.path, test.testName);
}
stepOut.put("stepTemplate", "tests/client/method.mustache");
stepOut.put("isMethod", true); // TODO: remove once dart and kotlin are converted
stepOut.put("isMethod", true); // TODO: remove once kotlin is converted
stepOut.put("hasOperationParams", ope.hasParams);

// set on testOut because we need to wrap everything for java.
testOut.put("isHelper", (boolean) ope.vendorExtensions.getOrDefault("x-helper", false));
testOut.put("isAsync", (boolean) ope.vendorExtensions.getOrDefault("x-asynchronous-helper", true)); // default to true because most api calls are asynchronous
}

stepOut.put("object", step.object);
stepOut.put("path", step.path);

Map<String, Object> requestOptions = new HashMap<>();
paramsType.enhanceParameters(step.requestOptions, requestOptions);
stepOut.put("requestOptions", requestOptions);
if (step.requestOptions != null) {
Map<String, Object> requestOptions = new HashMap<>();
paramsType.enhanceParameters(step.requestOptions, requestOptions);
stepOut.put("requestOptions", requestOptions);
}

if (step.path != null && CUSTOM_METHODS.contains(step.path)) {
stepOut.put("isCustom", true);
Expand Down Expand Up @@ -148,10 +155,11 @@ public void run(Map<String, CodegenModel> models, Map<String, CodegenOperation>
stepOut.put("expectedError", step.expected.error.replace(step.path, Helpers.toPascalCase(step.path)));
}
} else if (step.expected.match != null) {
if (step.expected.match instanceof Map) {
Map<String, Object> match = new HashMap<>();
paramsType.enhanceParameters((Map<String, Object>) step.expected.match, match);
stepOut.put("match", match);
Map<String, Object> matchMap = new HashMap<>();
if (step.expected.match instanceof Map match) {
paramsType.enhanceParameters(match, matchMap);
stepOut.put("match", matchMap);
stepOut.put("matchIsObject", true);
} else {
stepOut.put("match", step.expected.match);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public void run(Map<String, CodegenModel> models, Map<String, CodegenOperation>
boolean isHelper = (boolean) ope.vendorExtensions.getOrDefault("x-helper", false);
if (!cts.containsKey(operationId)) {
if (isHelper) {
continue;
continue; // some helpers don't have tests
}

throw new CTSException(
Expand Down
2 changes: 1 addition & 1 deletion playground/csharp/Playground/Playgrounds/Search.cs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ await PlaygroundHelper.Start("Deleting API Key", async () =>

Console.WriteLine("--- Generate Secured API Keys `GenerateSecuredApiKeys` ---");
var generateSecuredApiKeys = _client.GenerateSecuredApiKey(_configuration.SearchApiKey,
new SecuredAPIKeyRestrictions
new SecuredApiKeyRestrictions
{
RestrictIndices = [DefaultIndex],
});
Expand Down
2 changes: 1 addition & 1 deletion playground/swift/playground/playground/main.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ guard let apiKey = Bundle.main.infoDictionary?["ALGOLIA_ADMIN_KEY"] as? String e
}

guard applicationID != "" && apiKey != "" else {
fatalError("AppID and APIKey must be filled in your Info.plist file")
fatalError("AppID and ApiKey must be filled in your Info.plist file")
}

struct Contact: Codable {
Expand Down
4 changes: 2 additions & 2 deletions scripts/cts/runCts.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as fsp from 'fs/promises';

import { run, runComposerInstall, toAbsolutePath } from '../common.js';
import { isVerbose, run, runComposerInstall, toAbsolutePath } from '../common.js';
import { createSpinner } from '../spinners.js';
import type { Language } from '../types.js';

Expand All @@ -17,7 +17,7 @@ async function runCtsOne(language: string): Promise<void> {
await run('dart test', { cwd, language });
break;
case 'go':
await run('go test -race -count 1 ./...', {
await run(`go test -race -count 1 ${isVerbose() ? '-v' : ''} ./...`, {
cwd,
language,
});
Expand Down
7 changes: 4 additions & 3 deletions specs/search/helpers/generateSecuredApiKey.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
method:
get:
x-helper: true
x-asynchronous-helper: false
tags:
- Api Keys
operationId: generateSecuredApiKey
Expand All @@ -21,7 +22,7 @@ method:
The generated API key can have the same restrictions as the parent API key, or be more restrictive.
parameters:
- in: query
name: apiKey
name: parentApiKey
description: API key from which the secured API key will inherit its restrictions.
required: true
schema:
Expand All @@ -31,7 +32,7 @@ method:
description: Restrictions to add to the API key.
required: true
schema:
$ref: '#/securedAPIKeyRestrictions'
$ref: '#/securedApiKeyRestrictions'
responses:
'200':
description: OK
Expand All @@ -42,7 +43,7 @@ method:
'400':
$ref: '../../common/responses/IndexNotFound.yml'

securedAPIKeyRestrictions:
securedApiKeyRestrictions:
type: object
additionalProperties: false
properties:
Expand Down
2 changes: 1 addition & 1 deletion templates/csharp/tests/client/method.mustache
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{{^useEchoRequester}}var res = {{/useEchoRequester}}await client.{{#lambda.pascalcase}}{{#path}}.{{.}}{{/path}}{{/lambda.pascalcase}}Async{{#isGeneric}}<Object>{{/isGeneric}}({{#parametersWithDataType}}{{> tests/generateParams}}{{^-last}},{{/-last}}{{/parametersWithDataType}}{{#hasRequestOptions}}, new RequestOptions(){
{{^useEchoRequester}}var res = {{/useEchoRequester}}{{#isAsync}}await {{/isAsync}}client.{{#lambda.pascalcase}}{{#path}}.{{.}}{{/path}}{{/lambda.pascalcase}}{{#isAsync}}Async{{/isAsync}}{{#isGeneric}}<Object>{{/isGeneric}}({{#parametersWithDataType}}{{> tests/generateParams}}{{^-last}},{{/-last}}{{/parametersWithDataType}}{{#hasRequestOptions}}, new RequestOptions(){
{{#requestOptions.queryParameters}}
QueryParameters = new Dictionary<string, object>(){ {{#parametersWithDataType}} {"{{{key}}}", {{> tests/requests/requestOptionsParams}} } {{^-last}},{{/-last}}{{/parametersWithDataType}} },
{{/requestOptions.queryParameters}}
Expand Down
Loading
Loading