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.

Sign up for GitHub

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(generator): add hostWithFallback #479

Merged
merged 4 commits into from
May 6, 2022
Merged

fix(generator): add hostWithFallback #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ccd30e7
fix(generator): add `hostWithFallback`
shortcuts May 6, 2022
1e72f35
format json
shortcuts May 6, 2022
a690c77
fix predict spec
shortcuts May 6, 2022
d7688cf
remove unused if
shortcuts May 6, 2022
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
18 changes: 5 additions & 13 deletions generators/src/main/java/com/algolia/codegen/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public static void generateServer(
boolean hasRegionalHost = false;
boolean fallbackToAliasHost = false;
String host = "";
String topLevelDomain = "";
String hostWithFallback = "";
Set<String> allowedRegions = new HashSet<>();
for (Map<String, Object> server : servers) {
if (!server.containsKey("url")) {
Expand All @@ -102,7 +102,9 @@ public static void generateServer(
}
String otherUrl = (String) otherServer.getOrDefault("url", "");
if (otherUrl.replace(".{region}", "").equals(server.get("url"))) {
URL fallbackURL = new URL(otherUrl.replace(".{region}", ""));
fallbackToAliasHost = true;
hostWithFallback = fallbackURL.getHost();
break;
}
}
Expand Down Expand Up @@ -132,26 +134,16 @@ public static void generateServer(

// This is used for hosts like `insights` that uses `.io`
URL url = new URL((String) server.get("url"));
String[] hostParts = url.getHost().split("\\.");
host = hostParts[0];
topLevelDomain = hostParts[hostParts.length - 1];
host = url.getHost();
}
additionalProperties.put("hostWithFallback", hostWithFallback);
additionalProperties.put("hasRegionalHost", hasRegionalHost);
additionalProperties.put("fallbackToAliasHost", fallbackToAliasHost);
additionalProperties.put("host", host);
additionalProperties.put("topLevelDomain", topLevelDomain);
additionalProperties.put(
"allowedRegions",
allowedRegions.toArray(new String[0])
);

if (clientKebab.equals("predict")) {
additionalProperties.put("isExperimentalHost", true);
additionalProperties.put(
"host",
new URL((String) servers.get(0).get("url")).getHost()
);
}
} catch (Exception e) {
e.printStackTrace();
System.exit(1);
Expand Down
2 changes: 1 addition & 1 deletion scripts/generate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export async function generate(

await generateOpenapitools(generators);

const availableWorkspaces = await run('yarn workspaces list', { verbose });
const availableWorkspaces = await run('yarn workspaces list');
Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot to push this fix last time

const langs = [...new Set(generators.map((gen) => gen.language))];
const useCustomGenerator = langs
.map((lang) => getCustomGenerator(lang))
Expand Down
7 changes: 6 additions & 1 deletion specs/predict/spec.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ components:
apiKey:
$ref: '../common/securitySchemes.yml#/apiKey'
servers:
- url: https://predict-api-432xa6wemq-ew.a.run.app
- url: https://predict-api-432xa6wemq-{region}.a.run.app
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should switch the order of the stack PR so this doesn't appear here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not the same change as in #473, it just fixes the spec because ew is in fact a region

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah ok I didn't know ! Which region is it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Western Europe I believe? cc @anghelalexandra :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

Europe West ? 🤨

Copy link
Member Author

Choose a reason for hiding this comment

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

Estonia? https://en.wikipedia.org/wiki/ISO_3166-1_alpha-2#EW

variables:
region:
enum:
- ew
default: ew
security:
- appId: []
apiKey: []
Expand Down
21 changes: 8 additions & 13 deletions templates/java/libraries/okhttp-gson/api.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@ public class {{classname}} extends ApiClient {

{{^hasRegionalHost}}
public {{classname}}(String appId, String apiKey) {
this(appId, apiKey, new HttpRequester(getDefaultHosts({{^isExperimentalHost}}appId{{/isExperimentalHost}})), null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this variable isExperimentalHost created ? Is there any reason why it's used in the java and javascript templates and not in the php one ? Did I miss something ?

Copy link
Member Author

Choose a reason for hiding this comment

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

You missed it because you did not generated the Predict client for PHP

It was there because the URL was not following the correct schema, but in fact the ew value was a region.

We now don't use this variable anymore

this(appId, apiKey, new HttpRequester(getDefaultHosts(appId)), null);
}

public {{classname}}(String appId, String apiKey, UserAgent.Segment[] userAgentSegments) {
this(appId, apiKey, new HttpRequester(getDefaultHosts({{^isExperimentalHost}}appId{{/isExperimentalHost}})), userAgentSegments);
this(appId, apiKey, new HttpRequester(getDefaultHosts(appId)), userAgentSegments);
}
{{/hasRegionalHost}}

Expand All @@ -66,7 +66,7 @@ public class {{classname}} extends ApiClient {
super(appId, apiKey, requester, "{{{baseName}}}", userAgentSegments);
}

{{^hasRegionalHost}}{{^isExperimentalHost}}
{{^hasRegionalHost}}
private static List<StatefulHost> getDefaultHosts(String appId) {
List<StatefulHost> hosts = new ArrayList<StatefulHost>();
hosts.add(new StatefulHost(appId + "-dsn.algolia.net", "https", EnumSet.of(CallType.READ)));
Expand All @@ -81,20 +81,15 @@ public class {{classname}} extends ApiClient {

return Stream.concat(hosts.stream(), commonHosts.stream()).collect(Collectors.toList());
}
{{/isExperimentalHost}}{{/hasRegionalHost}}

{{#isExperimentalHost}}
private static List<StatefulHost> getDefaultHosts() {
List<StatefulHost> hosts = new ArrayList<StatefulHost>();
hosts.add(new StatefulHost("{{{host}}}", "https", EnumSet.of(CallType.READ, CallType.WRITE)));
return hosts;
}
{{/isExperimentalHost}}
{{/hasRegionalHost}}

{{#hasRegionalHost}}
private static List<StatefulHost> getDefaultHosts(String region) {
List<StatefulHost> hosts = new ArrayList<StatefulHost>();
hosts.add(new StatefulHost("{{{host}}}." + {{#fallbackToAliasHost}}(region == null ? "" : region + "."){{/fallbackToAliasHost}}{{^fallbackToAliasHost}}region{{/fallbackToAliasHost}} + "algolia.{{#topLevelDomain}}{{.}}{{/topLevelDomain}}{{^topLevelDomain}}com{{/topLevelDomain}}", "https", EnumSet.of(CallType.READ, CallType.WRITE)));

String url = {{#fallbackToAliasHost}}region == null ? "{{{hostWithFallback}}}" : {{/fallbackToAliasHost}} "{{{host}}}".replace("{region}", region);

hosts.add(new StatefulHost(url, "https", EnumSet.of(CallType.READ, CallType.WRITE)));
return hosts;
}
{{/hasRegionalHost}}
Expand Down
16 changes: 5 additions & 11 deletions templates/javascript/api-single.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const apiClientVersion = '{{packageVersion}}';
export type Region = {{#allowedRegions}}'{{.}}'{{^-last}}|{{/-last}}{{/allowedRegions}};
{{/hasRegionalHost}}

{{^hasRegionalHost}}{{^isExperimentalHost}}
{{^hasRegionalHost}}
function getDefaultHosts(appId: string): Host[] {
return (
[
Expand Down Expand Up @@ -66,27 +66,21 @@ function getDefaultHosts(appId: string): Host[] {
])
);
}
{{/isExperimentalHost}}{{/hasRegionalHost}}

{{#isExperimentalHost}}
function getDefaultHosts(): Host[] {
return [{ url: '{{host}}', accept: 'readWrite', protocol: 'https' }];
}
{{/isExperimentalHost}}
{{/hasRegionalHost}}

{{#hasRegionalHost}}
function getDefaultHosts(region{{#fallbackToAliasHost}}?{{/fallbackToAliasHost}}: Region): Host[] {
{{#fallbackToAliasHost}}const regionHost = region ? `.${region}.` : '.';{{/fallbackToAliasHost}}
const url = {{#fallbackToAliasHost}}!region ? '{{{hostWithFallback}}}' : {{/fallbackToAliasHost}} '{{{host}}}'.replace('{region}', region);

return [{ url: `{{{host}}}{{#fallbackToAliasHost}}${regionHost}{{/fallbackToAliasHost}}{{^fallbackToAliasHost}}.${region}.{{/fallbackToAliasHost}}algolia.{{#topLevelDomain}}{{.}}{{/topLevelDomain}}{{^topLevelDomain}}com{{/topLevelDomain}}`, accept: 'readWrite', protocol: 'https' }];
return [{ url, accept: 'readWrite', protocol: 'https' }];
}
{{/hasRegionalHost}}

// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
export function create{{capitalizedApiName}}(options: CreateClientOptions{{#hasRegionalHost}} & {region{{#fallbackToAliasHost}}?{{/fallbackToAliasHost}}: Region }{{/hasRegionalHost}}) {
const auth = createAuth(options.appId, options.apiKey, options.authMode);
const transporter = createTransporter({
hosts: options?.hosts ?? getDefaultHosts({{^hasRegionalHost}}{{^isExperimentalHost}}options.appId{{/isExperimentalHost}}{{/hasRegionalHost}}{{#hasRegionalHost}}options.region{{/hasRegionalHost}}),
hosts: options?.hosts ?? getDefaultHosts({{^hasRegionalHost}}options.appId{{/hasRegionalHost}}{{#hasRegionalHost}}options.region{{/hasRegionalHost}}),
hostsCache: options.hostsCache,
requestsCache: options.requestsCache,
responsesCache: options.responsesCache,
Expand Down
3 changes: 2 additions & 1 deletion templates/php/api.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ use {{invokerPackage}}\RetryStrategy\ClusterHosts;
// If a list of hosts was passed, we ignore the cache
$clusterHosts = ClusterHosts::create($hosts);
} else {
$clusterHosts = ClusterHosts::create('{{host}}.'.$config->getRegion().'.algolia.{{topLevelDomain}}');
$url = str_replace('{region}', $config->getRegion(), '{{{host}}}');
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the {{{host}}} variable is supposed to contain now ? Has it changed ? Before it was "analytics", "insights", ... no ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, but it contained a lot of logic like topLevelDomain etc.

It stores the whole domain now, and we only replace the region accordingly, so we don't have to compute other stuff than host

For insights for example its value is insights.{region}.algolia.io, so we only replace the {region} field with the region value

Copy link
Member Author

@shortcuts shortcuts May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You can see it in action here btw: dd5d4db

to make sure the logic is right for PHP

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I see, looks good then 👍

$clusterHosts = ClusterHosts::create($url);
}
{{/useCache}}

Expand Down
3 changes: 1 addition & 2 deletions tests/CTS/client/abtesting/parameters.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
"type": "createClient",
"parameters": {
"appId": "my-app-id",
"apiKey": "my-api-key",
"region": ""
"apiKey": "my-api-key"
},
"expected": {
"error": false
Expand Down
3 changes: 1 addition & 2 deletions tests/CTS/client/analytics/parameters.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
"type": "createClient",
"parameters": {
"appId": "my-app-id",
"apiKey": "my-api-key",
"region": ""
"apiKey": "my-api-key"
},
"expected": {
"error": false
Expand Down
3 changes: 1 addition & 2 deletions tests/CTS/client/insights/parameters.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
"type": "createClient",
"parameters": {
"appId": "my-app-id",
"apiKey": "my-api-key",
"region": ""
"apiKey": "my-api-key"
},
"expected": {
"error": false
Expand Down