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

[BUG] Some Namespaces do not exist in the API spec #262

Closed
kimpepper opened this issue Jan 23, 2025 · 9 comments
Closed

[BUG] Some Namespaces do not exist in the API spec #262

kimpepper opened this issue Jan 23, 2025 · 9 comments
Labels
bug Something isn't working

Comments

@kimpepper
Copy link
Collaborator

kimpepper commented Jan 23, 2025

What is the bug?

There are a few endpoints and namespaces that look like they no longer exist. They don't get generated when running composer run generate-api.

Namespaces:

  • MonitoringNamespace
  • AsyncSearchNamespace
  • DataFrameTransformDeprecatedNamespace
  • SslNamespace
  • SearchableSnapshotsNamespace

This was discovered by phpstan error messages about the class using deprecated methods in #259

How can one reproduce the bug?

Delete the endpoints/namespaces and run composer run generate-api

What is the expected behavior?

The endpoints/namespaces should be generated.

What is your host/environment?

Operating system, version.

Do you have any screenshots?

If applicable, add screenshots to help explain your problem.

Do you have any additional context?

Add any other context about the problem.

@kimpepper kimpepper added bug Something isn't working untriaged labels Jan 23, 2025
@kimpepper
Copy link
Collaborator Author

@dblock Can we confirm these no longer exist before we create an MR to remove them from the client lib?

@kimpepper
Copy link
Collaborator Author

Maybe we should deprecate them first.

@kimpepper
Copy link
Collaborator Author

I just found this #204 which added

// The following namespaces do not have OpenSearch API specifications
$patchnamespaces = ['async_search', 'searchable_snapshots', 'ssl', 'sql', 'data_frame_transform_deprecated', 'monitoring'];

Not really sure how to proceed here.

@dblock
Copy link
Member

dblock commented Jan 25, 2025

I am guessing this was added to the generator to skip generating the namespaces for which we had written code by hand and that did not have API specs yet (@saimedhi can confirm).

So we still have the manually rolled out code? I see https://github.com/opensearch-project/opensearch-php/blob/main/src/OpenSearch/Namespaces/AsyncSearchNamespace.php. We can just leave it alone and would need to fix it not to use deprecated classes.

Now, for each of these namespace we may have added a spec since, so check https://github.com/opensearch-project/opensearch-api-specification/ for it and if it's present, remove the exception that you mention above, and use the generated version after comparing it to the manual version. This can be done later, too.

Eventually no manual code should be in the client for these endpoints.

@saimedhi
Copy link
Collaborator

saimedhi commented Jan 27, 2025

I am guessing this was added to the generator to skip generating the namespaces for which we had written code by hand and that did not have API specs yet (@saimedhi can confirm).

@dblock , yes I added those lines for namespaces that do not have OpenSearch API specifications at the time of PHP Client Generation implementation.

@kimpepper
Copy link
Collaborator Author

AsynchronousSearchNamespace exists. Are you sure AsyncSearchNamespace is not a duplicate?

@dblock
Copy link
Member

dblock commented Jan 27, 2025

@kimpepper I think the spec for async search was added after, and it was misnamed before here. A backwards compatible version would make AsyncSearchNamespace alias AsynchronousSearchNamespace (and double check methods).

@kimpepper kimpepper changed the title [BUG] Some Endpoints and Namespaces do not exist [BUG] Some Endpoints and Namespaces do not exist in API spec Jan 28, 2025
@kimpepper kimpepper changed the title [BUG] Some Endpoints and Namespaces do not exist in API spec [BUG] Some Namespaces do not exist in the API spec Jan 28, 2025
@kimpepper
Copy link
Collaborator Author

Had another look and these were added for bakwards compatibility, ie. they previously existed but don't anymore in the current API spec. They weren't added for future compatiblity for a spec that doesn't exist yet.

I've created #270 which deprecates these namespaces for removal in 3.0.0.

@dblock dblock removed the untriaged label Jan 30, 2025
@kimpepper
Copy link
Collaborator Author

This was resolved in #270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants