-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add interfaces to OpenSearchClient's namespaces API specifications (fixes #423) #646
Add interfaces to OpenSearchClient's namespaces API specifications (fixes #423) #646
Conversation
Signed-off-by: Spencer G. Jones <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making this PR it's greatly appreciated! Just a couple minor things:
- I think it'd be good to also add the interface for the Http namespace for consistency
- You'll need to add an entry to the
CHANGELOG.md
saying this has been added/changed - Info should also be added to
UPGRADING.md
as changing the property types on the client is technically a breaking change
Signed-off-by: Spencer G. Jones <[email protected]>
Signed-off-by: Spencer G. Jones <[email protected]>
CHANGELOG and UPGRADING has been adjusted. I am not exactly sure how to layout the UPGRADING change, so feedback on the preferred text and format would be great. IHttpNamespace has been added. |
UPGRADING.md
Outdated
#### General | ||
- Namespaced APIs, exposed in `IOpenSearchClient`, have gained an interface, and the properties on `IOpenSearchClient` and `OpenSearchClient` have been changed to the new interfaces. For example, `IOpenSearchClient.Cluster` was `ClusterNamespace` and now is `IClusterNamespace`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I should have given more info, 2.0.0
is the in-development unreleased major version, so this can go in the existing 1.x.y to 2.0.0 > OpenSearch.Client > General
section.
Wording wise I would potentially rephrase this as:
#### General | |
- Namespaced APIs, exposed in `IOpenSearchClient`, have gained an interface, and the properties on `IOpenSearchClient` and `OpenSearchClient` have been changed to the new interfaces. For example, `IOpenSearchClient.Cluster` was `ClusterNamespace` and now is `IClusterNamespace`. | |
#### General | |
- The namespaced APIs exposed in `IOpenSearchClient` have each gained a corresponding interface and the types of the properties on `IOpenSearchClient` and `OpenSearchClient` have been changed from the concrete implementations to the matching interfaces. For example, `IOpenSearchClient.Cluster` was `ClusterNamespace` and now is `IClusterNamespace`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been updated.
CHANGELOG.md
Outdated
@@ -4,6 +4,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) | |||
## [Unreleased] | |||
### ⚠️ Breaking Changes ⚠️ | |||
- As part of [efforts to re-generate the client](https://github.com/opensearch-project/opensearch-net/pulls?q=is%3Apr+label%3Acode-gen+is%3Aclosed) from our [OpenAPI specification](https://github.com/opensearch-project/opensearch-api-specification) there have been numerous corrections and changes that resulted in breaking changes. Please refer to [UPGRADING.md](UPGRADING.md) for a complete list of these breakages and any relevant guidance for upgrading to this version of the client. | |||
- To allow easier unit testing of code using OpenSearch, interfaces were introduced for all classes under OpenSearch.Client.Specification, and the properties in IOpenServiceClient and OpenServiceClient were changed from the concrete classes to the new interfaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can move this further down under a ### Changed
heading, and it should generally follow the layout and verbiage of other entries, something like:
- To allow easier unit testing of code using OpenSearch, interfaces were introduced for all classes under OpenSearch.Client.Specification, and the properties in IOpenServiceClient and OpenServiceClient were changed from the concrete classes to the new interfaces. | |
- Changed the namespace client properties on `IOpenSearchClient` to return corresponding interfaces to better enable mocking & unit testing ([#646](https://github.com/opensearch-project/opensearch-net/pull/646)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been updated
Signed-off-by: Spencer G. Jones <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks @mispencer!
Description
Say you created a method using IOpenSearchClient:
Then you want to write a unit test for it. Since you don't want unit tests to depend on external services, you would likely mock out the IOpenSearchClient. So, you might write:
But this doesn't work. IOpenSearchClient is an interface and can be mocked, but IOpenSearchClient.Indices is IndicesNamespace, a concrete class, and concrete classes are not mockable unless they have a public constructor and have visual methods - neither which apply to IndicesNamespace or any other class under OpenSearch.Client.Specification. As such, you get an error on running this test or any test that references or calls code that references any of the namespaced API endpoints.
This PR adds interfaces to all of the auto-generated classes under OpenSearch.Client.Specification, and adjusts IOpenSearchClient and OpenSearchClient to return these interfaces instead of the concrete classes. This allows mocking to work for these classes, so that users of this library can unit test methods using them.
Issues Resolved
#423
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.