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(spec): better filters type #413

Merged
merged 18 commits into from
Apr 26, 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
2 changes: 1 addition & 1 deletion .github/.cache_version
Original file line number Diff line number Diff line change
@@ -1 +1 @@
8.0.9.0.8
8.0.10.0.8
2 changes: 1 addition & 1 deletion .github/workflows/check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ jobs:
with:
path: |
${{ format('{0}/algoliasearch-core/src/com/algolia/api/{1}.java', matrix.client.path, matrix.client.api) }}
${{ format('{0}/{1}/**', matrix.client.path, matrix.client.capitalizedName) }}
${{ format('{0}/algoliasearch-core/src/com/algolia/model/{1}/**', matrix.client.path, matrix.client.camelizedName) }}
key: |
${{ env.CACHE_VERSION }}-${{
hashFiles(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public Map<String, Object> postProcessAllModels(Map<String, Object> objs) {
.get(0)
.get("model");
if (!model.oneOf.isEmpty()) {
List<HashMap<String, String>> listOneOf = new ArrayList();
List<HashMap<String, String>> oneOfList = new ArrayList();

for (String iterateModel : model.oneOf) {
HashMap<String, String> oneOfModel = new HashMap();
Expand All @@ -163,11 +163,11 @@ public Map<String, Object> postProcessAllModels(Map<String, Object> objs) {
iterateModel.replace("<", "").replace(">", "")
);

listOneOf.add(oneOfModel);
oneOfList.add(oneOfModel);
}

model.vendorExtensions.put("x-is-one-of-interface", true);
model.vendorExtensions.put("x-is-one-of-list", listOneOf);
model.vendorExtensions.put("x-is-one-of-list", oneOfList);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import io.swagger.util.Json;
import java.util.*;
import java.util.Map.Entry;
import org.openapitools.codegen.CodegenComposedSchemas;
import org.openapitools.codegen.CodegenModel;
import org.openapitools.codegen.CodegenOperation;
import org.openapitools.codegen.CodegenParameter;
Expand Down Expand Up @@ -205,7 +206,24 @@ private void handleModel(
int suffix
) throws CTSException {
if (!spec.getHasVars()) {
throw new CTSException("Spec has no vars.");
// In this case we might have a complex `allOf`, we will first check
// if it exists
CodegenComposedSchemas composedSchemas = spec.getComposedSchemas();

if (composedSchemas != null) {
List<CodegenProperty> allOf = composedSchemas.getAllOf();

if (allOf != null && !allOf.isEmpty()) {
traverseParams(paramName, param, allOf.get(0), parent, suffix);

return;
}
}
// We only throw if there is no `composedSchemas`, because `oneOf` can also
// be handled below
else {
throw new CTSException("Spec has no vars.");
}
}

if (spec.getItems() != null) {
Expand All @@ -224,12 +242,21 @@ private void handleModel(
);

HashMap<String, String> oneOfModel = new HashMap<>();
String typeName = getTypeName(match).replace("<", "").replace(">", "");

oneOfModel.put("classname", Utils.capitalize(baseType));
oneOfModel.put(
"name",
getTypeName(match).replace("<", "").replace(">", "")
);

if (typeName.equals("List")) {
CodegenProperty items = match.getItems();

if (items == null) {
throw new CTSException("Unhandled case for empty oneOf List items.");
}

typeName += getTypeName(items);
}

oneOfModel.put("name", typeName);
testOutput.put("oneOfModel", oneOfModel);

return;
Expand Down Expand Up @@ -466,7 +493,18 @@ private IJsonSchemaValidationProperties findMatchingOneOf(
return bestOneOf;
}
if (param instanceof List) {
// no idea for list
// NICE ---> no idea for list <--- NICE
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry for this, I'll right better error exception next time

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah actually I like this way of thinking!!

CodegenComposedSchemas composedSchemas = model.getComposedSchemas();

if (composedSchemas != null) {
List<CodegenProperty> oneOf = composedSchemas.getOneOf();

// Somehow this is not yet enough
if (oneOf != null && !oneOf.isEmpty()) {
return oneOf.get(0);
}
Comment on lines +502 to +505
Copy link
Member Author

Choose a reason for hiding this comment

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

This takes the first item of the list, it's not correct but allow the CI to work for now

}

return null;
}

Expand Down
27 changes: 13 additions & 14 deletions scripts/ci/createMatrix.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { CLIENTS, GENERATORS } from '../common';
import { createClientName } from '../cts/utils';
import { camelize, createClientName } from '../cts/utils';
import type { Language } from '../types';

import { getNbGitDiff } from './utils';
Expand All @@ -16,9 +16,10 @@ type BaseMatrix = {
};

type ClientMatrix = BaseMatrix & {
config?: string;
api?: string;
capitalizedName?: string;
config: string;
api: string;
capitalizedName: string;
camelizedName: string;
};

type SpecMatrix = BaseMatrix;
Expand Down Expand Up @@ -56,18 +57,16 @@ async function getClientMatrix({
continue;
}

const matchedGenerator: ClientMatrix = {
name: client,
path: output,
};

const clientName = createClientName(client, language);

matchedGenerator.config = `${clientName}Config`;
matchedGenerator.api = `${clientName}Client`;
matchedGenerator.capitalizedName = clientName;

matrix.client.push(matchedGenerator);
matrix.client.push({
name: client,
path: output,
config: `${clientName}Config`,
api: `${clientName}Client`,
capitalizedName: clientName,
camelizedName: camelize(client),
});
}

return matrix;
Expand Down
34 changes: 29 additions & 5 deletions scripts/cts/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,47 @@ export async function* walk(
}
}

/**
* Sets the first letter of the given string in capital.
*
* `searchClient` -> `SearchClient`.
*/
export function capitalize(str: string): string {
return str.charAt(0).toUpperCase() + str.slice(1);
}

export function createClientName(client: string, language: string): string {
const clientName = client
.split('-')
/**
* Splits a string for a given `delimiter` (defaults to `-`) and capitalize each
* parts except the first letter.
*
* `search-client` -> `searchClient`.
*/
export function camelize(str: string, delimiter: string = '-'): string {
return str
.split(delimiter)
.map((part, i) => {
if (language === 'javascript' && i === 0) {
if (i === 0) {
return part;
}

return capitalize(part);
})
.join('');
}

/**
* Returns the client name with the correct casing for its language.
*
* `search-client`, `java` -> `SearchClient`.
*
* `search-client`, `javascript` -> `searchClient`.
*/
export function createClientName(client: string, language: string): string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

much cleaner like that 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

😊

Copy link
Member Author

Choose a reason for hiding this comment

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

(hopefully it works now D:)

Copy link
Collaborator

Choose a reason for hiding this comment

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

except the cache got skipped ahah

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes it ran just before ._. oh god

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah no it's good!!!

Copy link
Contributor

Choose a reason for hiding this comment

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

Do I need to change something on my PHP test branch ? :o

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet but you should merge first, there's still an issue with the CI for Java, I'll come back to it later

if (language === 'javascript') {
return camelize(client);
}

return clientName;
return capitalize(camelize(client));
}

export async function createOutputDir({
Expand Down
7 changes: 1 addition & 6 deletions scripts/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,12 +194,7 @@ buildCommand
clientsTodo = CLIENTS;
}
// ignore cache when building from cli
await buildSpecs(
clientsTodo,
outputFormat!,
Boolean(verbose),
!skipCache
);
await buildSpecs(clientsTodo, outputFormat, Boolean(verbose), !skipCache);
}
);

Expand Down
71 changes: 50 additions & 21 deletions specs/common/schemas/SearchParams.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,31 +30,14 @@ baseSearchParams:
type: string
description: Filter the query with numeric, facet and/or tag filters.
default: ''
# There could be a pattern for this one (complicated one)
facetFilters:
type: array
items:
type: string
description: Filter hits by facet value.
default: []
$ref: '#/facetFilters'
optionalFilters:
type: array
items:
type: string
description: Create filters for ranking purposes, where records that match the filter are ranked higher, or lower in the case of a negative optional filter.
default: []
$ref: '#/optionalFilters'
numericFilters:
type: array
items:
type: string
description: Filter on numeric attributes.
default: []
$ref: '#/numericFilters'
tagFilters:
type: array
items:
type: string
description: Filter hits by tags.
default: []
$ref: '#/tagFilters'
sumOrFiltersScores:
type: boolean
description: Determines how to calculate the total score for filtering.
Expand Down Expand Up @@ -164,6 +147,8 @@ baseSearchParams:
type: boolean
description: Whether this search should use AI Re-Ranking.
default: true
reRankingApplyFilter:
$ref: '#/reRankingApplyFilter'

searchParamsString:
type: object
Expand Down Expand Up @@ -202,3 +187,47 @@ aroundRadius:
aroundRadiusAll:
type: string
enum: [all]

# There is duplicated logic here because we want to keep a correct description
# and using `$ref` override everything.
searchFiltersArrayString:
type: array
items:
type: string

searchFiltersNestedArrayString:
type: array
items:
type: array
items:
type: string

facetFilters:
description: Filter hits by facet value.
oneOf:
- $ref: '#/searchFiltersArrayString'
- $ref: '#/searchFiltersNestedArrayString'

reRankingApplyFilter:
description: When Dynamic Re-Ranking is enabled, only records that match these filters will be impacted by Dynamic Re-Ranking.
oneOf:
- $ref: '#/searchFiltersArrayString'
- $ref: '#/searchFiltersNestedArrayString'

tagFilters:
description: Filter hits by tags.
oneOf:
- $ref: '#/searchFiltersArrayString'
- $ref: '#/searchFiltersNestedArrayString'

numericFilters:
description: Filter on numeric attributes.
oneOf:
- $ref: '#/searchFiltersArrayString'
- $ref: '#/searchFiltersNestedArrayString'

optionalFilters:
description: Create filters for ranking purposes, where records that match the filter are ranked higher, or lower in the case of a negative optional filter.
oneOf:
- $ref: '#/searchFiltersArrayString'
- $ref: '#/searchFiltersNestedArrayString'
Loading