opensearch-project / opensearch-js

Node.js Client for OpenSearch
https://opensearch.org/docs/latest/clients/javascript/
Apache License 2.0
189 stars 123 forks source link

[api_generator] API path builder update #913

Closed nhtruong closed 1 week ago

nhtruong commented 1 week ago

Handling of more complex sets of URLs.

Previously, the api_generator assumed that all URL paths of the same x-operation-group shared the same static components (i.e. if you remove all {path_param} components in each path, the paths are identical): For example these 4 paths of the nodes.stats group have an identical set of static components of _nodes and stats

And the generator would build the path as

const path = ['/_nodes/', node_id, '/stats/', metric, '/', index_metric].filter(c => c).join('').replace('//', '/');

However this assumption not true for some operation groups like cluster.stats:

Notice that the first 3 paths have _cluster, stats and nodes as their static components while the last one only has _cluster and stats. Therefore the following path that the generator use to make is incorrect:

const path = ['/_cluster/', metric, '/', index_metric, 'stats/nodes/', node_id].filter(c => c).join('').replace('//', '/');

because when none of the path params are provided, we would end up with const path = '/_cluster/states/nodes', which is not a valid path. With this PR, the generator will be able to detect this edge case and generate a more complex path building strategy for cluster.stats:

let path;
if (metric != null && index_metric != null && node_id != null) {
  path = '/_cluster/stats/' + metric + '/' + index_metric + '/nodes/' + node_id;
}  else if (metric != null && node_id != null) {
  path = '/_cluster/stats/' + metric + '/nodes/' + node_id;
}  else if (node_id != null) {
  path = '/_cluster/stats/nodes/' + node_id;
}  else {
  path = '/_cluster/stats';
}

Streamlined path building code

The generated path building logic for operation groups with uniform static path components is now simpler and more performant. Here are a few examples of old vs new code:

const path = ['/_nodes/', node_id, '/stats/', metric, '/', index_metric].filter(c => c).join('').replace('//', '/');
const path = ['/_nodes', node_id, 'stats', metric, index_metric].filter(c => c).join('/');
const path = ['/', index, '/_alias/', name].filter(c => c).join('').replace('//', '/');
const path = ['', index, '_alias', name].filter(c => c).join('/');
const path = ['/', index, '/_validate/query'].filter(c => c).join('').replace('//', '/');
const path = ['', index, '_validate/query'].filter(c => c).join('/');

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

dblock commented 1 week ago

Why are these in the same operation group when they are different APIs?

/_cluster/stats/{metric}/{index_metric}/nodes/{node_id}
/_cluster/stats/{metric}/nodes/{node_id}
/_cluster/stats/nodes/{node_id}
/_cluster/stats

Should they be in separate operation groups?

cluster_stats_nodes

/_cluster/stats/{metric}/{index_metric}/nodes/{node_id}
/_cluster/stats/{metric}/nodes/{node_id}
/_cluster/stats/nodes/{node_id}

cluster_stats

/_cluster/stats
nhtruong commented 1 week ago

@dblock that's how it's specified in the spec. The fix should be in the spec, if that's an error. Regardless, this code gen will now be able to handle this edge case if it appears in the spec.

nhtruong commented 1 week ago

I dont think we should change the groups because of how it's implemented on a client. That will also result in a breaking change for all clients.