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

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

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

opensearch-trigger-bot[bot]
Copy link
Contributor

@opensearch-trigger-bot opensearch-trigger-bot bot commented Jul 13, 2024

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

@saimedhi
Copy link
Collaborator

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

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.

Let's block this PR to avoid merging accidentally.

@saimedhi are you fixing the ml part?

@saimedhi
Copy link
Collaborator

Let's block this PR to avoid merging accidentally.

@saimedhi are you fixing the ml part?

yes

@dblock
Copy link
Member

dblock commented Jul 16, 2024

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

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

shyim commented Jul 17, 2024

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

@saimedhi
Copy link
Collaborator

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

shyim commented Jul 19, 2024

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
Copy link
Member

dblock commented Jul 19, 2024

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!

@opensearch-trigger-bot opensearch-trigger-bot bot force-pushed the automated-api-update branch 2 times, most recently from 0c93ea4 to 6965230 Compare July 24, 2024 03:35
@opensearch-trigger-bot opensearch-trigger-bot bot force-pushed the automated-api-update branch 4 times, most recently from f4b8b9d to 5c6d162 Compare July 30, 2024 03:35
@opensearch-trigger-bot opensearch-trigger-bot bot force-pushed the automated-api-update branch 7 times, most recently from 889e26b to 4db986d Compare August 7, 2024 03:35
@opensearch-trigger-bot opensearch-trigger-bot bot force-pushed the automated-api-update branch 2 times, most recently from d69961e to 60dcdba Compare August 9, 2024 03:35
@dblock
Copy link
Member

dblock commented Aug 9, 2024

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
Copy link
Member

dblock commented Aug 9, 2024

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

saimedhi commented Aug 9, 2024

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.

saimedhi
saimedhi previously approved these changes Aug 9, 2024
@dblock
Copy link
Member

dblock commented Aug 9, 2024

@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 dblock marked this pull request as draft August 9, 2024 21:17
@saimedhi
Copy link
Collaborator

saimedhi commented Aug 9, 2024

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

shyim commented Aug 9, 2024

@dblock it's fine for me

@opensearch-trigger-bot opensearch-trigger-bot bot force-pushed the automated-api-update branch 2 times, most recently from 27e6238 to 5ea1873 Compare August 12, 2024 03:35
@dblock
Copy link
Member

dblock commented Aug 12, 2024

@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.

Copy link

codecov bot commented Aug 12, 2024

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
Copy link
Member

dblock commented Aug 12, 2024

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

…4-08-12)

Signed-off-by: GitHub <noreply@github.com>
@dblock dblock marked this pull request as ready for review August 12, 2024 15:30
@dblock dblock merged commit bad675e into main Aug 12, 2024
88 checks passed
@dblock dblock deleted the automated-api-update branch August 12, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants