Skip to content

Conversation

@joegallo
Copy link
Contributor

@joegallo joegallo commented Feb 24, 2021

Related to #51816

Makes Routes RestApiVersion -aware (and RestHandlers RestApiVersion -agnostic). Refactors how Routes are constructed in the case of deprecation or replacement of routes.


Some examples of how this affects Rest Actions

Very normal existing rest action, no change on this PR:

public class RestMainAction extends BaseRestHandler {

    @Override
    public List<Route> routes() {
        return List.of(
            new Route(GET, "/"),
            new Route(HEAD, "/"));
    }

[...]

A rest action with a deprecated route, before:

public class RestPostDataAction extends BaseRestHandler {

    @Override
    public List<Route> routes() {
        return Collections.emptyList();
    }

    @Override
    public List<DeprecatedRoute> deprecatedRoutes() {
        return Collections.singletonList(
            new DeprecatedRoute(POST, MachineLearning.BASE_PATH + "anomaly_detectors/{" + Job.ID.getPreferredName() + "}/_data",
            "Posting data directly to anomaly detection jobs is deprecated, " +
                "in a future major version it will be compulsory to use a datafeed"));
    }

[...]

And after:

public class RestPostDataAction extends BaseRestHandler {

     @Override
     public List<Route> routes() {
        return List.of(
            Route.builder(POST, BASE_PATH + "anomaly_detectors/{" + Job.ID.getPreferredName() + "}/_data")
                .deprecated("Posting data directly to anomaly detection jobs is deprecated, " +
                    "in a future major version it will be compulsory to use a datafeed", RestApiVersion.V_8
                ).build()
        );
    }

[...]

A rest action with some replaced routes, before:

public class RestGetUsersAction extends SecurityBaseRestHandler {

    @Override
    public List<Route> routes() {
        return Collections.emptyList();
    }

    @Override
    public List<ReplacedRoute> replacedRoutes() {
        // TODO: remove deprecated endpoint in 8.0.0
        return List.of(
            new ReplacedRoute(GET, "/_security/user/", GET, "/_xpack/security/user/"),
            new ReplacedRoute(GET, "/_security/user/{username}", GET, "/_xpack/security/user/{username}")
        );
    }

[...]

And after:

public class RestGetUsersAction extends SecurityBaseRestHandler {

    @Override
    public List<Route> routes() {
        return List.of(
           Route.builder(GET, "/_security/user/")
               .replaces(GET, "/_xpack/security/user/", RestApiVersion.V_7).build(),
           Route.builder(GET, "/_security/user/{username}")
               .replaces(GET, "/_xpack/security/user/{username}", RestApiVersion.V_7).build()
        );
    }

[...]

@joegallo
Copy link
Contributor Author

Crucially, these sorts of methods fall out:

         @Override
         public List<DeprecatedRoute> deprecatedRoutes() {
             return [...];
         }

         @Override
         public List<ReplacedRoute> replacedRoutes() {
             return [...];
         }

And now there is only routes().

@joegallo joegallo requested a review from jaymode February 25, 2021 03:31
@pgomulka

This comment has been minimized.

@joegallo

This comment has been minimized.

@jaymode

This comment has been minimized.

@joegallo
Copy link
Contributor Author

joegallo commented Mar 4, 2021

@pgomulka 7bb0919 should address your comment w.r.t. imports

Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Member

@jaymode jaymode 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 few minor comments, otherwise LGTM.

joegallo added 2 commits March 5, 2021 13:54
Collapse two constructors into one, document the new parameter that it
grew, and add a test that exercises the new parameter.
@joegallo
Copy link
Contributor Author

joegallo commented Mar 5, 2021

Thank you @jakelandis, @jaymode, and @pgomulka for working with me on this one. I'm really happy with where it ended up!

Now to resurrect the _xpack/* APIs under this new mechanism. 😄

@joegallo joegallo merged commit 4cc4c2c into elastic:master Mar 6, 2021
@joegallo joegallo deleted the route-refactoring branch March 6, 2021 00:11
@joegallo joegallo self-assigned this Mar 6, 2021
joegallo added a commit that referenced this pull request Mar 10, 2021
joegallo added a commit to joegallo/elasticsearch that referenced this pull request Mar 10, 2021
Related to elastic#51816

Makes `Route`s  `RestApiVersion` -aware (and `RestHandler`s `RestApiVersion` -agnostic). Refactors
how `Route`s are constructed in the case of deprecation or replacement of routes.
joegallo added a commit that referenced this pull request Mar 16, 2021
Makes `Route`s  `RestApiVersion` -aware. Refactors how `Route`s are constructed in the case of deprecation or replacement of routes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/REST API REST infrastructure and utilities >non-issue Team:Core/Infra Meta label for core/infra team v7.13.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants