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.

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

Deprecate _source_include and _source_exclude url parameters #33475

Merged
merged 5 commits into from
Oct 29, 2018

Conversation

lipsill
Copy link
Contributor

@lipsill lipsill commented Sep 6, 2018

Relates to #22792

@nik9000 nik9000 added the :Search/Search Search-related issues that do not fall into other categories label Sep 6, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I left something minor but this seems like the right way to do it to me.

Can anyone from @elastic/es-clients comment on the changes to the rest-api-spec?

sIncludes = request.param("_source_include", sIncludes);
String sInclude = request.param("_source_include");
if (sInclude != null) {
DEPRECATION_LOGGER.deprecated("Deprecated field [_source_include] used, expected [_source_includes] instead");
Copy link
Member

Choose a reason for hiding this comment

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

What about replacing "field" with "parameter"?

@nik9000
Copy link
Member

nik9000 commented Sep 6, 2018

I labelled this Search/Search because I figured that it is mostly about the search API. There are a few indices APIs that it effects as well though.

@nik9000
Copy link
Member

nik9000 commented Sep 6, 2018

@elasticmachine, test this please

`source_excludes`
the deprecation message refers to `parameter` instead of `field`
"Deprecated _source_include and _source_exclude":

- skip:
version: " - 6.4.99"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

// TODO consider changing the version (if needed) once the clients specs are changed to _source_includes & _source_excludes

Copy link
Member

Choose a reason for hiding this comment

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

I'd use 6.99.99 for now. After I merge this I'll backport it to the 6.x branch. Then I'll come in and update the range here to include the backport target version.

I'm fairly sure this is what caused the build to fail. If you crack open the PR build's full console it'll have a REPRODUCE WITH line that should reproduce the failure. And I'm fairly sure switching this to 6.99.99 will make it go away.

@lipsill
Copy link
Contributor Author

lipsill commented Sep 7, 2018

@nik9000 thanks for the review! And starting the CI. I wonder why the high level REST client tests passed though. I guess I can look into this next ;)

Should I assume that this PR will stay on hold until the clients change their specs?

@nik9000
Copy link
Member

nik9000 commented Sep 7, 2018

@nik9000 thanks for the review! And starting the CI. I wonder why the high level REST client tests passed though. I guess I can look into this next ;)

I'm happy to do it! Why the tests passed is a good question. If they are ignoring deprecation messages that'd be bad but a problem for another PR. Happy hunting!

Should I assume that this PR will stay on hold until the clients change their specs?

They won't need to change their specs but they might need to do something. I'm not sure what. The .NET client generates things from these files.

@colings86 colings86 added v6.6.0 and removed v6.5.0 labels Oct 25, 2018
@lipsill
Copy link
Contributor Author

lipsill commented Oct 26, 2018

@nik9000 I just updated the PR to include the the strict deprecation mode and resolve the conflicts.
I was gonna ask somebody to trigger the CI, but it started by itself 😇

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I left a request to change the line that I think caused the build to fail.

"Deprecated _source_include and _source_exclude":

- skip:
version: " - 6.4.99"
Copy link
Member

Choose a reason for hiding this comment

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

I'd use 6.99.99 for now. After I merge this I'll backport it to the 6.x branch. Then I'll come in and update the range here to include the backport target version.

I'm fairly sure this is what caused the build to fail. If you crack open the PR build's full console it'll have a REPRODUCE WITH line that should reproduce the failure. And I'm fairly sure switching this to 6.99.99 will make it go away.

@nik9000
Copy link
Member

nik9000 commented Oct 26, 2018

Thanks for updating this @lipsill! I think it is super close. Hopefully the REPRODUCE WITH line in the build failure will let you reproduce the issue so it isn't too bad to fix. Thanks again!

@lipsill
Copy link
Contributor Author

lipsill commented Oct 26, 2018

@nik9000 thanks for the tip work around the issue;)

Can you trigger another CI when you have the chance?

@nik9000
Copy link
Member

nik9000 commented Oct 26, 2018

@elasticmachine, test this please

@nik9000 nik9000 merged commit 6df1c9e into elastic:master Oct 29, 2018
nik9000 pushed a commit that referenced this pull request Oct 29, 2018
)

Deprecates `_source_include` and `_source_exclude` url parameters
in favor of `_source_inclues` and `_source_excludes` because those
are consistent with the rest of Elasticsearch's APIs.

Relates to #22792
@nik9000
Copy link
Member

nik9000 commented Oct 29, 2018

@lipsill all merged and backported! Thanks so much for this!

kcm pushed a commit that referenced this pull request Oct 30, 2018
)

Deprecates `_source_include` and `_source_exclude` url parameters
in favor of `_source_inclues` and `_source_excludes` because those
are consistent with the rest of Elasticsearch's APIs.

Relates to #22792
nik9000 added a commit that referenced this pull request Oct 30, 2018
Update the test for the deprecated `_source_exclude` parameter to run
against 6.6.0+ now that we've backported the deprecation.

Relates to #33475
nik9000 added a commit that referenced this pull request Oct 30, 2018
Update the test for the deprecated `_source_exclude` parameter to run
against 6.6.0+ now that we've backported the deprecation.

Relates to #33475
nik9000 pushed a commit that referenced this pull request Oct 31, 2018
…de` (#35097)

Removes `_source_include` and `_source_exclude` url parameters. 
These parameters have been deprecated in #33475.

Closes #22792
@Mpdreamz
Copy link
Member

This did end up biting us @nik9000 thank you for the ping though, not sure how it got lost on my end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>deprecation :Search/Search Search-related issues that do not fall into other categories v6.6.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants