search

opensearch-project / opensearch-php

Official PHP Client for OpenSearch
Other
110 stars 58 forks source link

Updated opensearch-php to reflect the latest OpenSearch API spec #213

Closed opensearch-trigger-bot[bot] closed 3 months ago

opensearch-trigger-bot[bot] commented 4 months ago

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

saimedhi commented 4 months ago

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

saimedhi commented 4 months ago

Let's block this PR to avoid merging accidentally.

@saimedhi are you fixing the ml part?

yes

dblock commented 4 months ago

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

saimedhi commented 4 months ago

@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 commented 4 months ago

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

saimedhi commented 4 months ago

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

Hi @shyim,

shyim commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@dblock it's fine for me

dblock commented 3 months ago

@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 https://github.com/opensearch-project/opensearch-php/pull/221. Please review.

codecov[bot] commented 3 months ago

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 :warning:
src/OpenSearch/Namespaces/QueryNamespace.php 0.00% 33 Missing :warning:
src/OpenSearch/Namespaces/PplNamespace.php 0.00% 26 Missing :warning:
src/OpenSearch/Namespaces/SqlNamespace.php 31.57% 26 Missing :warning:
...penSearch/Endpoints/Observability/UpdateObject.php 0.00% 25 Missing :warning:
...penSearch/Endpoints/Observability/DeleteObject.php 0.00% 20 Missing :warning:
...c/OpenSearch/Endpoints/Observability/GetObject.php 0.00% 20 Missing :warning:
...rc/OpenSearch/Endpoints/Query/DatasourceDelete.php 0.00% 20 Missing :warning:
.../OpenSearch/Endpoints/Query/DatasourceRetrieve.php 0.00% 20 Missing :warning:
src/OpenSearch/Endpoints/Ppl/Explain.php 0.00% 19 Missing :warning:
... and 22 more
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dblock commented 3 months ago

I extracted the SQL backwards-compatibility fixes from here into https://github.com/opensearch-project/opensearch-php/pull/222, once that's merged the cron will reset this PR and all tests should pass. @shyim @saimedhi