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.

Sign up for GitHub

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] Added default constructors for JSON parsing using Jackson #5369

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8817116
Added default constructor for JSON parsing using jackson
owaiskazi19 Nov 24, 2022
420f8f9
Updated changelog
owaiskazi19 Nov 24, 2022
63bd5b7
Formatted code using spotless
owaiskazi19 Nov 24, 2022
b577b57
Addressed PR comments
owaiskazi19 Nov 28, 2022
e202a93
Added comment for default construtor
owaiskazi19 Nov 28, 2022
b17cd47
Spotless
owaiskazi19 Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Enforce type safety for RegisterTransportActionsRequest([#4796](https://github.com/opensearch-project/OpenSearch/pull/4796))
- Modified local node request to return Discovery Node ([#4862](https://github.com/opensearch-project/OpenSearch/pull/4862))
- Enforce type safety for NamedWriteableRegistryParseRequest ([#4923](https://github.com/opensearch-project/OpenSearch/pull/4923))
- Added default constructor for JSON parsing using Jackson ([#5369](https://github.com/opensearch-project/OpenSearch/pull/5369))

## [Unreleased 2.x]
### Added
Expand Down
7 changes: 7 additions & 0 deletions server/src/main/java/org/opensearch/search/aggregations/AbstractAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ protected AbstractAggregationBuilder(StreamInput in) throws IOException {
metadata = in.readMap();
}

/**
* Constructor required for Jackson parsing
*/
protected AbstractAggregationBuilder() {
Copy link
Member

Choose a reason for hiding this comment

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

(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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

super();
}

@Override
public final void writeTo(StreamOutput out) throws IOException {
out.writeString(name);
Expand Down
7 changes: 6 additions & 1 deletion server/src/main/java/org/opensearch/search/aggregations/AggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,14 @@ public abstract class AggregationBuilder
BaseAggregationBuilder,
Rewriteable<AggregationBuilder> {

protected final String name;
protected String name;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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;
}

protected AggregatorFactories.Builder factoriesBuilder = AggregatorFactories.builder();

/**
* Constructor required for Jackson parsing
*/
AggregationBuilder() {}

/**
* Constructs a new aggregation builder.
*
Expand Down
7 changes: 7 additions & 0 deletions server/src/main/java/org/opensearch/search/aggregations/metrics/SumAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@ public SumAggregationBuilder(String name) {
super(name);
}

/**
* Constructor required for Jackson parsing
*/
private SumAggregationBuilder() {
super();
Copy link
Member

Choose a reason for hiding this comment

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

this should probably call super(NAME).

}

protected SumAggregationBuilder(
SumAggregationBuilder clone,
AggregatorFactories.Builder factoriesBuilder,
Expand Down
14 changes: 14 additions & 0 deletions .../main/java/org/opensearch/search/aggregations/support/ValuesSourceAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@
*/
public abstract class ValuesSourceAggregationBuilder<AB extends ValuesSourceAggregationBuilder<AB>> extends AbstractAggregationBuilder<AB> {

/**
* Constructor required for Jackson parsing
*/
private ValuesSourceAggregationBuilder() {
super();
}

public static <T> void declareFields(
AbstractObjectParser<? extends ValuesSourceAggregationBuilder<?>, T> objectParser,
boolean scriptable,
Expand Down Expand Up @@ -166,6 +173,13 @@ protected LeafOnly(StreamInput in) throws IOException {
super(in);
}

/**
* Constructor required for Jackson parsing
*/
public LeafOnly() {
super();
}

@Override
public final AB subAggregations(Builder subFactories) {
throw new AggregationInitializationException(
Expand Down