Begin centralizing XContentParser creation into RestRequest #22041

nik9000 · 2016-12-08T03:14:46Z

To get #22003 in cleanly we need to centralize as much XContentParser creation as possible into RestRequest. That'll mean we have to plumb the NamedXContentRegistry into fewer places.

This removes RestAction.hasBody, RestAction.guessBodyContentType, and RestActions.getRestContent, moving callers over to RestRequest.hasContentOrSourceParam, RestRequest.contentOrSourceParam, and RestRequest.contentOrSourceParamParser and RestRequest.withContentOrSourceParamParserOrNull. The idea is to use withContentOrSourceParamParserOrNull if you need to handle requests without any sort of body content and to use contentOrSourceParamParser otherwise.

I believe the vast majority of this PR to be purely mechanical but I know I've made the following behavioral change (I'll add more if I think of more):

If you make a request to an endpoint that requires a request body and has cut over to the new APIs instead of getting Failed to derive xcontent you'll get Body required.
Template parsing is now non-strict by default. This is important because we need to be able to deprecate things without requests failing.

nik9000 · 2016-12-08T03:15:31Z

core/src/main/java/org/elasticsearch/action/get/MultiGetRequest.java

I removed these because they didn't look used and I was breaking backwards compatibility with them anyway....

nik9000 · 2016-12-08T03:17:13Z

core/src/main/java/org/elasticsearch/rest/RestRequest.java

I'd kind of prefer to drop this in favor of contentOrSourceParamParserOrNull because of the nice dichotomy between it and contentOrSourceParamParser but that'd be a larger.

I've removed contentOrSourceParamParserOrNull and use withContentOrSourceParamParserOrNull everywhere now because closing nullable parsers is fiddly.

nik9000 · 2016-12-08T03:18:17Z

core/src/main/java/org/elasticsearch/rest/RestRequest.java

I'm not really sure how much I like this. It saves a few lines every time but only a few. If we could entirely replace contentOrSourceParamParserOrNull with this then I'd like it, otherwise I'm tempted to remove this.

I removed contentOrSourceParamParserOrNull this morning so this comment is now out of date. Ignore please.

nik9000 · 2016-12-08T03:19:23Z

core/src/main/java/org/elasticsearch/rest/RestRequest.java

Originally I'd exposed this as @Deprecated, that is why the two methods. I feel like it should be deprecated because we want to work with parsers where we can and we're parsing on the coordinating node, but too many APIs use it.

nik9000 · 2016-12-08T03:21:08Z

core/src/main/java/org/elasticsearch/rest/action/RestActions.java

I think eventually we can remove this one as well. Because as a followup to #22003 we'll be able to remove IndicesQueriesRegistry and ParseFieldMatcher is also going to go eventually.

nik9000 · 2016-12-08T03:22:26Z

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestAnalyzeAction.java

I moved the try into the calling method so this is mostly indentation changing.

nik9000 · 2016-12-08T03:24:34Z

core/src/test/java/org/elasticsearch/rest/RestRequestTests.java

I feel like I should add more here.

I'm about to push a patch that adds more here.

nik9000 · 2016-12-08T13:06:30Z

To get #22003 in cleanly we need to centralize as much XContentParser creation as possible into RestRequest.

On second thought this isn't 100% required for #22003, but it is nice to have.

nik9000 · 2016-12-08T16:09:18Z

@rjernst, can you have a look at this when you get a chance? This isn't strictly required by #22003 but will make the kind of centralizing of the parser that we'd talked about easier. Or at least more consistent.

Ultimately I'd love to never expose a BytesReference in the public API of RestRequest and the design of this PR is informed by that desire.

nik9000 · 2016-12-09T22:08:13Z

elasticsearch-ci, retest this please

rjernst

This looks ok to me. I'm not a fan of all the variants of parsing methods on RestRequest, but I realize this is mostly a rote consolidation of all the many ways we already created parsers. But I'm hoping we can continue to improve this to cut down on the different methodologies.

rjernst · 2016-12-09T21:48:29Z

core/src/main/java/org/elasticsearch/common/IOConsumer.java

Can you move out the CheckedConsumer I added to ActionListener and reuse that?

I think so. I'll have a look.

rjernst · 2016-12-09T21:52:53Z

core/src/main/java/org/elasticsearch/rest/RestRequest.java

Shouldn't callers call hasContentOrSourceParam() first, so a precondition for this method should be content != null?

This is fairly rarely used so we can do what we like. For the most part they don't but we can certainly force them to.

I'm just removing it.

rjernst · 2016-12-09T22:16:00Z

core/src/main/java/org/elasticsearch/rest/RestRequest.java

Is this helper really needed?

I added it when I thought I could get away with making contentOrSourceParam private or at least deprecated. Now that I've pretty much given up on that we don't need this. In fact I can probably drop contentOrSourceParamXContentType as well under the same logic. Good intentions led to bad results I think.

nik9000 · 2016-12-09T22:26:34Z

I'm hoping we can continue to improve this to cut down on the different methodologies.

Right. I think smashing them all into one place will help us to look at them and say "I don't need this one".

nik9000 · 2016-12-09T22:26:57Z

elasticsearch-ci, retest this please

I didn't mean to leave this comment for times....

Moves `RestActions.hasBody` to a much more obvious location and method name `RestRequest.hasContentOrSourceParam` and documents when you should use it. This should make is more obvious and is going to go well with subsequent refactorings.

Move `RestActions.guessBodyContentType` to `RestRequest.contentOrSourceParamXContentType` and document that is only useful if you need to do something different if the request is plain text.

`RestActions.getRestContent` is easy to forget to use. This starts to move calls to that method to the more obvious spot `RestRequest.contentOrSourceParamXContentType`. In addition to being a more obvious place this facilites moving most `XContentParser` creation into `RestRequest` which would make future changes much more convenient.

Since it only exists to support deprecated APIs it is deprecated. Prefer `RestRequest.contentOrSourceParamParser`.

This makes it compatible with `RestRequest.contentOrSourceParamParser`.

It has been entirely replaced by `RestRequest.contentOrSourceParamParser` and friends.

Use `withContentOrSourceParamParserOrNull` instead.

I give up on making `RestRequest.contentOrBodyParam` private or deprecated. I just advise folks to use the other methods if you need a parser.

nik9000 · 2016-12-10T01:38:01Z

Thanks for reviewing @rjernst!

master: 3adefb7
5.x: 0fd1e8f

To get #22003 in cleanly we need to centralize as much `XContentParser` creation as possible into `RestRequest`. That'll mean we have to plumb the `NamedXContentRegistry` into fewer places. This removes `RestAction.hasBody`, `RestAction.guessBodyContentType`, and `RestActions.getRestContent`, moving callers over to `RestRequest.hasContentOrSourceParam`, `RestRequest.contentOrSourceParam`, and `RestRequest.contentOrSourceParamParser` and `RestRequest.withContentOrSourceParamParserOrNull`. The idea is to use `withContentOrSourceParamParserOrNull` if you need to handle requests without any sort of body content and to use `contentOrSourceParamParser` otherwise. I believe the vast majority of this PR to be purely mechanical but I know I've made the following behavioral change (I'll add more if I think of more): * If you make a request to an endpoint that requires a request body and has cut over to the new APIs instead of getting `Failed to derive xcontent` you'll get `Body required`. * Template parsing is now non-strict by default. This is important because we need to be able to deprecate things without requests failing.

nik9000 added >non-issue v5.2.0 v6.0.0-alpha1 WIP labels Dec 8, 2016

nik9000 commented Dec 8, 2016

View reviewed changes

nik9000 requested a review from rjernst December 8, 2016 03:24

nik9000 added review and removed WIP labels Dec 8, 2016

nik9000 added :Internal and removed >non-issue labels Dec 9, 2016

rjernst approved these changes Dec 9, 2016

View reviewed changes

nik9000 added 13 commits December 9, 2016 19:10

Move RestActions.hasBody

184623d

Moves `RestActions.hasBody` to a much more obvious location and method name `RestRequest.hasContentOrSourceParam` and documents when you should use it. This should make is more obvious and is going to go well with subsequent refactorings.

Move RestActions.guessBodyContentType

aabd5c0

Move `RestActions.guessBodyContentType` to `RestRequest.contentOrSourceParamXContentType` and document that is only useful if you need to do something different if the request is plain text.

Create RestRequest.contentOrSourceParam

874f1b4

Since it only exists to support deprecated APIs it is deprecated. Prefer `RestRequest.contentOrSourceParamParser`.

Convert RestActions.getQueryContent to take XContentParser

0e2c112

This makes it compatible with `RestRequest.contentOrSourceParamParser`.

Move multiget

9a7866c

Fix reindex

45a1fed

More

59f4e88

More

1c4b50a

Cut over scroll

746a9cb

Remove RestActions.getRestContent

a3bbbff

It has been entirely replaced by `RestRequest.contentOrSourceParamParser` and friends.

Javadoc

aa1f297

Remove contentOrSourceParamParserOrNull

a2c5f23

Use `withContentOrSourceParamParserOrNull` instead.

nik9000 added 5 commits December 9, 2016 19:10

Fix javadoc

2577d56

Unbreak validation query action

92eebe6

Remove some helpers we don't need

6943381

I give up on making `RestRequest.contentOrBodyParam` private or deprecated. I just advise folks to use the other methods if you need a parser.

Replace IOConsumer with CheckedConsumer

68c6f7a

Finish removing methods we don't want any more

c10fb1f

nik9000 force-pushed the rest_request_more_methods branch from 31c13f2 to c10fb1f Compare December 10, 2016 01:00

nik9000 merged commit 3adefb7 into elastic:master Dec 10, 2016

clintongormley added the >non-issue label Dec 12, 2016

Begin centralizing XContentParser creation into RestRequest #22041

Begin centralizing XContentParser creation into RestRequest #22041

Uh oh!

Conversation

nik9000 commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Dec 8, 2016

Uh oh!

nik9000 commented Dec 8, 2016

Uh oh!

nik9000 commented Dec 9, 2016

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Dec 9, 2016

Uh oh!

nik9000 commented Dec 9, 2016

Uh oh!

nik9000 commented Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nik9000 commented Dec 8, 2016 •

edited

Loading

nik9000 commented Dec 10, 2016 •

edited

Loading