[Feature/extensions] Added default constructors for JSON parsing using Jackson #5369

owaiskazi19 · 2022-11-24T00:27:39Z

Signed-off-by: Owais Kazi [email protected]

Added default constructor for JSON parsing using Jackson required for SearchRequest of Java Client for Get Detector feature of AD Extension

Description

Part of opensearch-project/opensearch-sdk-java#214

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

github-actions · 2022-11-24T00:51:25Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7305/
CommitID: c0b88dd
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-24T00:56:53Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7306/
CommitID: d52a381
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-24T01:12:26Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7308/
CommitID: 25b451d
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-24T01:44:51Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7311/
CommitID: 25b451d
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

dbwiddis

LGTM, a few questions!

dbwiddis · 2022-11-25T06:53:57Z

server/src/main/java/org/opensearch/search/aggregations/AbstractAggregationBuilder.java

@@ -77,6 +77,10 @@ protected AbstractAggregationBuilder(StreamInput in) throws IOException {
        metadata = in.readMap();
    }

+    protected AbstractAggregationBuilder() {


(Preference) Would be nice to throw in a comment with the reason the constructor exists (required by Jackson). Repeat for the other 6 constructors in this PR. Although I notice in another constructor there's a @JsonCreator annotation which I presume does the same thing. What's the difference and why prefer one over the other?

Added comments for all the constructors. Registering module in SDKClient here helped to remove @JsonCreator annotation.
JsonCreator just helps to tune the constructor for deserialization but default constructor is already taking care of it.

dbwiddis · 2022-11-25T06:58:30Z

server/src/main/java/org/opensearch/search/aggregations/AggregationBuilder.java

 public abstract class AggregationBuilder
    implements
        NamedWriteable,
        ToXContentFragment,
        BaseAggregationBuilder,
        Rewriteable<AggregationBuilder> {

-    protected final String name;
+    protected String name;


I understand you had to remove the final because your no-arg constructor wasn't setting this, but I'm wondering if you can make the 1-arg (name) constructor the one Jackson uses. See this SO post for more info than I know, but you've clearly been digging into this so might understand it more than I do.

We already have a 1-arg constructor but Jackson requires a default constructor for deserialization.

The bigger question here is: if we are throwing an exception for a null name, how is it we are allowing Jackson to construct it with a null name? Specifically: if a null name is bad, we should not allow Jackson to create it null. If a null name is not bad, we should not be throwing an exception.

As far as a solution if we think a null name is bad, we could just define the default constructor to call this("defaultConstructor") internally and retain the final.

Alternately, and the point of my previous comment and SO link, there's a @JsonCreator annotation that will direct Jackson to use that one instead:

@JsonCreator protected AggregationBuilder(@JsonProperty("defaultName") String name) { if (name == null) { throw new IllegalArgumentException("[name] must not be null: [" + name + "]"); } this.name = name; }

dbwiddis · 2022-11-25T07:00:24Z

server/src/main/java/org/opensearch/search/aggregations/AggregationBuilder.java

@@ -100,11 +109,13 @@ public String getName() {
    public abstract AggregationBuilder subAggregation(PipelineAggregationBuilder aggregation);

    /** Return the configured set of subaggregations **/
+    @JsonProperty("subAggregations")


I didn't think this was necessary if the property name exactly matched the getter syntax. (getS ... s and the rest is the same). Same comment below for pipelines.

Registering the modules in SDKClient helped to remove the JsonProperty for OpenSearch and in AD Extension as well.

joshpalis · 2022-11-25T19:28:55Z

@owaiskazi19 It seems that gradle check is failing due to an apache lucence dependency issue. I suggest that you rebase your PR with feature/extensions now that @ryanbogan rebased this with main

github-actions · 2022-11-28T20:33:00Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7395/
CommitID: d068556
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-28T20:45:23Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7396/
CommitID: fdf70d6
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-29T02:26:31Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7400/
CommitID: 7756d70
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-29T21:00:04Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7411/
CommitID: 7756d70
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-11-30T19:01:06Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/7451/
CommitID: 7756d70
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: Owais Kazi <[email protected]>

github-actions · 2022-12-02T21:31:15Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/7556/
CommitID: b17cd47

dbwiddis · 2022-12-08T16:38:17Z

server/src/main/java/org/opensearch/search/aggregations/AggregationBuilder.java

 public abstract class AggregationBuilder
    implements
        NamedWriteable,
        ToXContentFragment,
        BaseAggregationBuilder,
        Rewriteable<AggregationBuilder> {

-    protected final String name;
+    protected String name;


The bigger question here is: if we are throwing an exception for a null name, how is it we are allowing Jackson to construct it with a null name? Specifically: if a null name is bad, we should not allow Jackson to create it null. If a null name is not bad, we should not be throwing an exception.

As far as a solution if we think a null name is bad, we could just define the default constructor to call this("defaultConstructor") internally and retain the final.

Alternately, and the point of my previous comment and SO link, there's a @JsonCreator annotation that will direct Jackson to use that one instead:

@JsonCreator protected AggregationBuilder(@JsonProperty("defaultName") String name) { if (name == null) { throw new IllegalArgumentException("[name] must not be null: [" + name + "]"); } this.name = name; }

dbwiddis · 2022-12-08T16:39:55Z

server/src/main/java/org/opensearch/search/aggregations/metrics/SumAggregationBuilder.java

+     * Constructor required for Jackson parsing
+     */
+    private SumAggregationBuilder() {
+        super();


this should probably call super(NAME).

ryanbogan · 2023-02-09T17:20:24Z

@owaiskazi19 Did this get merged into main at some point? If so can we close it

owaiskazi19 · 2023-02-20T19:55:04Z

Closing this PR as we have integrated High level client for the migration.

owaiskazi19 requested review from a team and reta as code owners November 24, 2022 00:27

owaiskazi19 changed the title ~~Added default constructor for JSON parsing using Jackson~~ [Feature/extensions] Added default constructor for JSON parsing using Jackson Nov 24, 2022

owaiskazi19 changed the title ~~[Feature/extensions] Added default constructor for JSON parsing using Jackson~~ [Feature/extensions] Added default constructors for JSON parsing using Jackson Nov 24, 2022

owaiskazi19 requested review from dbwiddis, ryanbogan and joshpalis November 24, 2022 00:46

dbwiddis reviewed Nov 25, 2022

View reviewed changes

owaiskazi19 force-pushed the get-detector branch from 25b451d to d068556 Compare November 28, 2022 20:11

owaiskazi19 added 6 commits December 2, 2022 13:01

Added default constructor for JSON parsing using jackson

8817116

Signed-off-by: Owais Kazi <[email protected]>

Updated changelog

420f8f9

Signed-off-by: Owais Kazi <[email protected]>

Formatted code using spotless

63bd5b7

Signed-off-by: Owais Kazi <[email protected]>

Addressed PR comments

b577b57

Signed-off-by: Owais Kazi <[email protected]>

Added comment for default construtor

e202a93

Signed-off-by: Owais Kazi <[email protected]>

Spotless

b17cd47

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 force-pushed the get-detector branch from 7756d70 to b17cd47 Compare December 2, 2022 21:01

owaiskazi19 requested a review from dbwiddis December 2, 2022 21:33

joshpalis approved these changes Dec 5, 2022

View reviewed changes

ryanbogan approved these changes Dec 6, 2022

View reviewed changes

dbwiddis reviewed Dec 8, 2022

View reviewed changes

owaiskazi19 closed this Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature/extensions] Added default constructors for JSON parsing using Jackson #5369

[Feature/extensions] Added default constructors for JSON parsing using Jackson #5369

owaiskazi19 commented Nov 24, 2022

github-actions bot commented Nov 24, 2022

github-actions bot commented Nov 24, 2022

github-actions bot commented Nov 24, 2022

github-actions bot commented Nov 24, 2022

dbwiddis left a comment

dbwiddis Nov 25, 2022

owaiskazi19 Nov 28, 2022

dbwiddis Nov 25, 2022

owaiskazi19 Nov 28, 2022

dbwiddis Dec 8, 2022

dbwiddis Nov 25, 2022

owaiskazi19 Nov 28, 2022

joshpalis commented Nov 25, 2022

github-actions bot commented Nov 28, 2022

github-actions bot commented Nov 28, 2022

github-actions bot commented Nov 29, 2022

github-actions bot commented Nov 29, 2022

github-actions bot commented Nov 30, 2022

github-actions bot commented Dec 2, 2022

dbwiddis Dec 8, 2022

dbwiddis Dec 8, 2022

ryanbogan commented Feb 9, 2023 •

edited

Loading

owaiskazi19 commented Feb 20, 2023

[Feature/extensions] Added default constructors for JSON parsing using Jackson #5369

[Feature/extensions] Added default constructors for JSON parsing using Jackson #5369

Conversation

owaiskazi19 commented Nov 24, 2022

Description

Issues Resolved

Check List

github-actions bot commented Nov 24, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 24, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 24, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 24, 2022

Gradle Check (Jenkins) Run Completed with:

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshpalis commented Nov 25, 2022

github-actions bot commented Nov 28, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 28, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 29, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 29, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Nov 30, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 2, 2022

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbogan commented Feb 9, 2023 • edited Loading

owaiskazi19 commented Feb 20, 2023

ryanbogan commented Feb 9, 2023 •

edited

Loading