Updated opensearch-php to reflect the latest OpenSearch API spec

[opensearch-trigger-bot]

Updated opensearch-php to reflect the latest OpenSearch API spec
Date: 2024-08-12

[saimedhi]

Shouldn't be merged. ML namespace needs to be patched in generator to prevent breaking changes.

[dblock]

Let's block this PR to avoid merging accidentally.

@saimedhi are you fixing the ml part?

[saimedhi]

Let's block this PR to avoid merging accidentally.

@saimedhi are you fixing the ml part?

yes

[dblock]

@saimedhi There's another change in the spec that you might need to handle, similar to opensearch-project/opensearch-py#777, see opensearch-project/opensearch-api-specification#416 where node_id_or_metric should not be added as a parameter, but the individual items with title should.

[saimedhi]

@saimedhi There's another change in the spec that you might need to handle, similar to opensearch-project/opensearch-py#777, see opensearch-project/opensearch-api-specification#416 where node_id_or_metric should not be added as a parameter, but the individual items with title should.

I'll take a look at this when I have a moment. Thank you :)

[shyim]

We would need to keep the name MachineLearningNamespace until the next major otherwise it's a type break

[saimedhi]

We would need to keep the name MachineLearningNamespace until the next major otherwise it's a type break

Hi @shyim,

• The name was changed from MachineLearningNamespace to MlNamespace during the endpoint generation using the spec. PR.

• I tested the change here to see if it's breaking. But didn't notice any breaking.

• If it's a type break, we can modify the generator here in a way that if $namespace is 'Ml', replace it with 'MachineLearning.'

• With the above step, ensure that the property ml here is not replaced with machineLearning during generation, as it creates a breaking change. A few small changes might need to go to ClientEndpoint.php to prevent this.

• Please try to contribute if you find some time. Thank you.

[shyim]

Renaming a class or namespace is a break, I would stick to Ml as previous.

maybe we should add https://github.com/Roave/BackwardCompatibilityCheck to the pipelines

[dblock]

Renaming a class or namespace is a break, I would stick to Ml as previous.

Do client users have to reference this namespace explicitly, meaning why is it considered a breaking change?

I think long term we'd like the generated code to look more like the spec. Is there a way to patch/subclass the generated default name into a namespace for backwards compatibility. If not, yes.

maybe we should add https://github.com/Roave/BackwardCompatibilityCheck to the pipelines

Definitely. Open an issue so we don't forget?

I think @saimedhi isn't working on the fix. If you have time appreciate it, if not maybe I can take it up next. LMK!

[dblock]

One of the tests failed because closeCursor was renamed.

Note: Using configuration file /home/runner/work/opensearch-php/opensearch-php/phpstan.neon.dist.
Error: Call to an undefined method OpenSearch\Namespaces\SqlNamespace::closeCursor().
 ------ ---------------------------------------------------- 
  Line   tests/Namespaces/SqlNamespaceTest.php               
 ------ ---------------------------------------------------- 
  83     Call to an undefined method                         
         OpenSearch\Namespaces\SqlNamespace::closeCursor().  
 ------ ---------------------------------------------------- 

@saimedhi What's the right way to deal with the sql namespace that gets generated with different function names, like here where closeCursor got renamed to close? I'd like to define closeCursor as a deprecated one that calls close. Where do I put this code?

[dblock]

@shyim Take a look at this change and comments re: backwards-compatibility from @saimedhi above. Does this work for you or do we need to do something else with this PR?

[saimedhi]

One of the tests failed because closeCursor was renamed.

Note: Using configuration file /home/runner/work/opensearch-php/opensearch-php/phpstan.neon.dist.
Error: Call to an undefined method OpenSearch\Namespaces\SqlNamespace::closeCursor().
 ------ ---------------------------------------------------- 
  Line   tests/Namespaces/SqlNamespaceTest.php               
 ------ ---------------------------------------------------- 
  83     Call to an undefined method                         
         OpenSearch\Namespaces\SqlNamespace::closeCursor().  
 ------ ---------------------------------------------------- 

@saimedhi What's the right way to deal with the sql namespace that gets generated with different function names, like here where closeCursor got renamed to close? I'd like to define closeCursor as a deprecated one that calls close. Where do I put this code?

@dblock , It needs to be added here https://github.com/opensearch-project/opensearch-php/tree/main/util/EndpointProxies.

Please refer point in time APIs in above link. Thank you.

[dblock]

@saimedhi one more, in https://github.com/opensearch-project/opensearch-php/pull/213/files#diff-1e5c4748694dfd5e04e32ebaaaaf36245262522d929e9d9232d8afc2c32352b3L41 the API spec was changed to un-confuse metric and node_id. What's the best way to restore this behavior inside getUri?

[saimedhi]

@saimedhi one more, in https://github.com/opensearch-project/opensearch-php/pull/213/files#diff-1e5c4748694dfd5e04e32ebaaaaf36245262522d929e9d9232d8afc2c32352b3L41 the API spec was changed to un-confuse metric and node_id. What's the best way to restore this behavior inside getUri?

@dblock, I see that it is handled in opensearch-py here.

Similarly it can be done in opensearch-php here.

I hope I rightly answered your question. Thanks.

[shyim]

@dblock it's fine for me

[dblock]

@saimedhi one more, in https://github.com/opensearch-project/opensearch-php/pull/213/files#diff-1e5c4748694dfd5e04e32ebaaaaf36245262522d929e9d9232d8afc2c32352b3L41 the API spec was changed to un-confuse metric and node_id. What's the best way to restore this behavior inside getUri?

@dblock, I see that it is handled in opensearch-py here.

Similarly it can be done in opensearch-php here.

I hope I rightly answered your question. Thanks.

Thanks. The best workaround I found is #221. Please review.

[codecov]

Codecov Report

Attention: Patch coverage is 6.67870% with 517 lines in your changes missing coverage. Please review.

Project coverage is 28.07%. Comparing base (fc749ea) to head (27810f2).
Report is 7 commits behind head on main.

Files	Patch %	Lines
...c/OpenSearch/Namespaces/ObservabilityNamespace.php	0.00%	45 Missing ⚠️
src/OpenSearch/Namespaces/QueryNamespace.php	0.00%	33 Missing ⚠️
src/OpenSearch/Namespaces/PplNamespace.php	0.00%	26 Missing ⚠️
src/OpenSearch/Namespaces/SqlNamespace.php	31.57%	26 Missing ⚠️
...penSearch/Endpoints/Observability/UpdateObject.php	0.00%	25 Missing ⚠️
...penSearch/Endpoints/Observability/DeleteObject.php	0.00%	20 Missing ⚠️
...c/OpenSearch/Endpoints/Observability/GetObject.php	0.00%	20 Missing ⚠️
...rc/OpenSearch/Endpoints/Query/DatasourceDelete.php	0.00%	20 Missing ⚠️
.../OpenSearch/Endpoints/Query/DatasourceRetrieve.php	0.00%	20 Missing ⚠️
src/OpenSearch/Endpoints/Ppl/Explain.php	0.00%	19 Missing ⚠️
... and 22 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #213      +/-   ##
============================================
+ Coverage     22.01%   28.07%   +6.06%     
- Complexity     2260     2707     +447     
============================================
  Files           309      377      +68     
  Lines          9026    11056    +2030     
============================================
+ Hits           1987     3104    +1117     
- Misses         7039     7952     +913     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dblock]

I extracted the SQL backwards-compatibility fixes from here into #222, once that's merged the cron will reset this PR and all tests should pass. @shyim @saimedhi