Skip to content

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

Merged
owaiskazi19 merged 2 commits intoopensearch-project:feature/extensionsfrom
mloufra:compilerwarning
Oct 21, 2022
Merged

[Feature/extensions]Enforce type safety for RegisterTransportActionsRequest#4796
owaiskazi19 merged 2 commits intoopensearch-project:feature/extensionsfrom
mloufra:compilerwarning

Conversation

@mloufra
Copy link
Copy Markdown
Contributor

@mloufra mloufra commented Oct 14, 2022

Signed-off-by: mloufra mloufra@amazon.com

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
Copy Markdown
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
Copy Markdown
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link
Copy Markdown

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
Copy Markdown
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.

private Map<String, Class<?>> transportActions;

public RegisterTransportActionsRequest(String uniqueId, Map<String, Class> transportActions) {
public RegisterTransportActionsRequest(String uniqueId, Map<String, Class<?>> transportActions) {
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Gradle Check (Jenkins) Run Completed with:

@mloufra mloufra requested a review from dbwiddis October 17, 2022 19:07
@owaiskazi19
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Signed-off-by: mloufra <mloufra@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: mloufra <mloufra@amazon.com>
@github-actions
Copy link
Copy Markdown
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