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

[api_generator] API path builder update #913

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

nhtruong
Copy link
Collaborator

@nhtruong nhtruong commented Nov 15, 2024

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

  • /_nodes/{node_id}/stats/{metric}/{index_metric}
  • /_nodes/{node_id}/stats/{metric}
  • /_nodes/{node_id}/stats
  • /_nodes/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:

  • /_cluster/stats/{metric}/{index_metric}/nodes/{node_id}
  • /_cluster/stats/{metric}/nodes/{node_id}
  • /_cluster/stats/nodes/{node_id}
  • /_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.

Updated URL path building strategy to accommodate for more complex set of URLs.

Signed-off-by: Theo Truong <[email protected]>
@dblock
Copy link
Member

dblock commented Nov 15, 2024

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This is fine, but I bet in a year nobody will understand what it's doing for lack of tests. Approving to unblock.

@nhtruong nhtruong merged commit ba69002 into opensearch-project:main Nov 18, 2024
51 of 52 checks passed
@nhtruong nhtruong deleted the path-selection-logic branch November 18, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants