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

Jump to bottom

improve FetchSourceContext and add unit tests #77346

Merged
merged 2 commits into from
Sep 9, 2021
Merged

improve FetchSourceContext and add unit tests #77346

merged 2 commits into from
Sep 9, 2021

Conversation

weizijun
Copy link
Contributor

@weizijun weizijun commented Sep 7, 2021

improve FetchSourceContext and add unit tests

  • add null value check in fromXContent, if no null value check, FetchSourceContextTests fromXContent tests will failed. It is similar to GetMappingsResponse.
  • fromXContent improve ParsingException's message.
  • add FetchSourceContextTests.

@elasticsearchmachine elasticsearchmachine added v8.0.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Sep 7, 2021
@weizijun
Copy link
Contributor Author

weizijun commented Sep 8, 2021

@nik9000 can you help to review this PR?

@nik9000 nik9000 added the :Search/Search Search-related issues that do not fall into other categories label Sep 8, 2021
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Sep 8, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

nik9000
nik9000 reviewed Sep 8, 2021
View reviewed changes
server/src/test/java/org/elasticsearch/search/fetch/subphase/FetchSourceContextTests.java
public class FetchSourceContextTests extends AbstractSerializingTestCase<FetchSourceContext> {
@Override
protected FetchSourceContext doParseInstance(XContentParser parser) throws IOException {
return FetchSourceContext.fromXContent(parser);
Copy link
Member

Choose a reason for hiding this comment

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

When I wrote some tests that recently had the NPE on this sort of thing I would do:

        ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser);
       FetchSourceContext result = FetchSourceContext.fromXContent(parser);
       ensureExpectedToken(XContentParser.Token.END_OBJECT, parser.nextToken(), parser);
       return parser;

That way I don't have to modify the production code for the test. I think it's probably the right thing to do here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if fetchSource equals false, the XContent is not object, nor json, it is a value of false.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed it does!

@nik9000
Copy link
Member

nik9000 commented Sep 9, 2021

@elasticmachine, test this please

@nik9000 nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 9, 2021
@nik9000
Copy link
Member

nik9000 commented Sep 9, 2021

@elasticmachine, update branch

@nik9000
Copy link
Member

nik9000 commented Sep 9, 2021

@elasticmachine pdate branch

@nik9000
Copy link
Member

nik9000 commented Sep 9, 2021

@elasticmachine update branch

@nik9000
Copy link
Member

nik9000 commented Sep 9, 2021

@elasticmachine, test this please

@elasticsearchmachine elasticsearchmachine merged commit f57a1e7 into elastic:master Sep 9, 2021
@nik9000
Copy link
Member

nik9000 commented Sep 9, 2021

@elasticsearchmachine elasticsearchmachine merged commit f57a1e7 into elastic:master 19 minutes ago

Good bot.

@nik9000
Copy link
Member

nik9000 commented Sep 9, 2021

Thanks @weizijun! I'll backport this to 7.x now.

@nik9000 nik9000 added the v7.16.0 label Sep 9, 2021
nik9000 pushed a commit to nik9000/elasticsearch that referenced this pull request Sep 9, 2021
@weizijun @elasticmachine @nik9000
improve FetchSourceContext and add unit tests (elastic#77346)
04e81d3 
Co-authored-by: Elastic Machine <[email protected]>
elasticsearchmachine pushed a commit that referenced this pull request Sep 9, 2021
@nik9000 @weizijun @elasticmachine
improve FetchSourceContext and add unit tests (#77346) (#77521)
c6180cf 
Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: weizijun <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
@weizijun
Copy link
Contributor Author

Thanks @weizijun! I'll backport this to 7.x now.

Thanks @nik9000 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees
No one assigned
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.16.0 v8.0.0-alpha2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@weizijun @elasticmachine @nik9000 @jakelandis @danhermann @elasticsearchmachine