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

[Feature/extensions]Enforce type safety for RegisterTransportActionsRequest #4796

Merged

Conversation

mloufra
Copy link
Contributor

@mloufra mloufra commented Oct 14, 2022

Signed-off-by: mloufra [email protected]

Description

Add unbounded wildcard for RegisterTransportActionsRequest constructor to clean up compiler warning
Equivalent PR on OpenSearch-sdk-java: opensearch-project/opensearch-sdk-java#195

Issues Resolved

opensearch-project/opensearch-sdk-java#130

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.

@dbwiddis
Copy link
Member

@mloufra Can you edit your title to add [Feature/extensions] as a prefix so the maintainers know our team is handling it?

@mloufra mloufra changed the title Compiler warning Compiler warning [Feature/extensions] Oct 14, 2022
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2022

Codecov Report

Merging #4796 (f74f20f) into feature/extensions (eee59c5) will increase coverage by 0.09%.
The diff coverage is 76.82%.

@@                   Coverage Diff                    @@
##             feature/extensions    #4796      +/-   ##
========================================================
+ Coverage                 70.52%   70.62%   +0.09%     
+ Complexity                57898    57799      -99     
========================================================
  Files                      4708     4706       -2     
  Lines                    278528   277836     -692     
  Branches                  40661    40389     -272     
========================================================
- Hits                     196420   196208     -212     
+ Misses                    65740    65354     -386     
+ Partials                  16368    16274      -94     
Impacted Files Coverage Δ
...main/java/org/opensearch/gradle/PublishPlugin.java 43.43% <0.00%> (-0.45%) ⬇️
.../java/org/opensearch/gradle/pluginzip/Publish.java 11.62% <0.00%> (-0.57%) ⬇️
...earch/analysis/common/NGramTokenFilterFactory.java 75.00% <0.00%> (+10.71%) ⬆️
...gregations/bucket/geogrid/BucketPriorityQueue.java 75.00% <ø> (ø)
...arch/aggregations/bucket/geogrid/CellIdSource.java 69.23% <ø> (ø)
...ions/bucket/geogrid/GeoGridAggregationBuilder.java 75.67% <ø> (ø)
.../bucket/geogrid/GeoHashGridAggregationBuilder.java 100.00% <ø> (ø)
.../bucket/geogrid/GeoTileGridAggregationBuilder.java 100.00% <ø> (ø)
...ions/bucket/geogrid/InternalGeoHashGridBucket.java 100.00% <ø> (ø)
...ions/bucket/geogrid/InternalGeoTileGridBucket.java 100.00% <ø> (ø)
... and 691 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally put the feature/extensions tag at the beginning of the title. A more descriptive title than "Compiler Warning" might be helpful, such as "Enforce type safety for RegisterTransportActionsRequest".

Also per CI checks you need a change log.


public RegisterTransportActionsRequest(String uniqueId, Map<String, Class> transportActions) {
public RegisterTransportActionsRequest(String uniqueId, Map<String, Class<?>> transportActions) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is called from the SDK with this line:

new RegisterTransportActionsRequest(uniqueId, transportActions),

See comment here for narrower types needed.

@mloufra mloufra changed the title Compiler warning [Feature/extensions] Enforce type safety for RegisterTransportActionsRequest [Feature/extensions] Oct 14, 2022
@mloufra mloufra changed the title Enforce type safety for RegisterTransportActionsRequest [Feature/extensions] [Feature/extensions]Enforce type safety for RegisterTransportActionsRequest Oct 14, 2022
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@mloufra mloufra requested a review from dbwiddis October 17, 2022 19:07
@owaiskazi19
Copy link
Member

@mloufra you need to make the changes in test files as well

/home/runner/work/OpenSearch/OpenSearch/server/src/test/java/org/opensearch/extensions/RegisterTransportActionsRequestTests.java:24: error: incompatible types: inference variable V has incompatible bounds
        this.originalRequest = new RegisterTransportActionsRequest("extension-uniqueId", Map.of("testAction", Map.class));
> Task :server:compileTestJava
                               ^
    equality constraints: Class<? extends TransportAction<? extends ActionRequest,? extends ActionResponse>>
    lower bounds: Class<org.opensearch.common.collect.Map>
  where V,K are type-variables:
    V extends Object declared in method <K,V>of(K,V)
    K extends Object declared in method <K,V>of(K,V)
/home/runner/work/OpenSearch/OpenSearch/server/src/test/java/org/opensearch/extensions/action/ExtensionTransportActionsHandlerTests.java:132: error: incompatible types: inference variable V has incompatible bounds
        RegisterTransportActionsRequest request = new RegisterTransportActionsRequest(

@mloufra
Copy link
Contributor Author

mloufra commented Oct 17, 2022

@mloufra you need to make the changes in test files as well

/home/runner/work/OpenSearch/OpenSearch/server/src/test/java/org/opensearch/extensions/RegisterTransportActionsRequestTests.java:24: error: incompatible types: inference variable V has incompatible bounds
        this.originalRequest = new RegisterTransportActionsRequest("extension-uniqueId", Map.of("testAction", Map.class));
> Task :server:compileTestJava
                               ^
    equality constraints: Class<? extends TransportAction<? extends ActionRequest,? extends ActionResponse>>
    lower bounds: Class<org.opensearch.common.collect.Map>
  where V,K are type-variables:
    V extends Object declared in method <K,V>of(K,V)
    K extends Object declared in method <K,V>of(K,V)
/home/runner/work/OpenSearch/OpenSearch/server/src/test/java/org/opensearch/extensions/action/ExtensionTransportActionsHandlerTests.java:132: error: incompatible types: inference variable V has incompatible bounds
        RegisterTransportActionsRequest request = new RegisterTransportActionsRequest(

Thanks for the reminder, I'm trying to fix this.

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes here look good, per other comment need to also update test files.

@dbwiddis dbwiddis self-requested a review October 18, 2022 18:30
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@owaiskazi19 owaiskazi19 merged commit 58f2c6e into opensearch-project:feature/extensions Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants