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

Remove types from Graph API #46935

Merged
merged 2 commits into from
Sep 23, 2019

Conversation

romseygeek
Copy link
Contributor

The actual usage of types within the graph code was removed by #42112,
so this commit just removes them from the Rest API

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@jpountz jpountz mentioned this pull request Sep 20, 2019
66 tasks
public GraphExploreRequest(StreamInput in) throws IOException {
super(in);

indices = in.readStringArray();
indicesOptions = IndicesOptions.readIndicesOptions(in);
types = in.readStringArray();
if (in.getVersion().before(Version.V_8_0_0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned this in the PR for the explain API as well -- I think it'd be nice to read the types array and assert that it is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++

@@ -187,12 +163,16 @@ public GraphExploreRequest timeout(String timeout) {
return this;
}

private static final String[] EMPTY_ARRAY = new String[]{};
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of defining this variable, I think we could use Strings.EMPTY_ARRAY below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I knew we had a predefined array somewhere, and for some reason was unable to find it. Thanks!

controller.registerWithDeprecatedHandler(
POST, "/{index}/{type}/_graph/explore", this,
POST, "/{index}/{type}/_xpack/graph/_explore", deprecationLogger);
controller.registerHandler(GET, "/{index}/_graph/explore", this);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we removed the deprecated _xpack endpoints here instead of the ones containing {type}. We could remove the ones with {type} in this PR, and leave the _xpack removal for a future PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, good catch - will fix.

@romseygeek
Copy link
Contributor Author

I also spotted that I hadn't removed types from the Rest-High-Level client; I have updated the PR accordingly.

@romseygeek romseygeek merged commit 8927735 into elastic:master Sep 23, 2019
@romseygeek romseygeek deleted the types-removal/graph-action branch September 23, 2019 16:57
@pgomulka pgomulka mentioned this pull request Mar 25, 2020
66 tasks
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Jun 16, 2021
adds back the typed and xpack endpoints for  graph explore api
prevoiusly removed in elastic#46935

relates main meta issue elastic#51816
relates types removal issue elastic#54160
pgomulka added a commit that referenced this pull request Jun 23, 2021
adds back the typed and xpack endpoints for graph explore api
prevoiusly removed in #46935

relates main meta issue #51816
relates types removal issue #54160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants