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

Safely register rest handlers outside of constructor #51622

Closed
jaymode opened this issue Jan 29, 2020 · 3 comments · Fixed by #51950
Closed

Safely register rest handlers outside of constructor #51622

jaymode opened this issue Jan 29, 2020 · 3 comments · Fixed by #51950
Assignees
@jaymode
Labels
:Core/Infra/REST API REST infrastructure and utilities >refactoring

Comments

@jaymode
Copy link
Member

jaymode commented Jan 29, 2020

The common pattern used by the RestHandler objects within the codebase is to have a constructor which registers the handler with the RestController for a set of paths and methods, such as:

public MyRestHandler(RestController controller) {
    controller.registerHandler(Method.GET, "/_my_endpoint", this);
}

This is publishing the this reference before the object is fully constructed, which violates the JLS. This is similar to issues we have with registering cluster state listeners within constructors, see #38560.
@jaymode jaymode added :Core/Infra/REST API REST infrastructure and utilities >refactoring labels Jan 29, 2020
@jaymode jaymode self-assigned this Jan 29, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/REST API)

@rjernst
Copy link
Member

rjernst commented Jan 29, 2020

Thanks for opening this @jaymode! This has long bugged me. In addition to the JLS violation, this makes the handlers more complicated to initialize, when they could mostly have empty ctors, and more error prone to write since you may forget to actually register yourself.

WDYT about having BaseRestHandler have a method for returning a Map<String, List<RestRequest.Method>> and doing the registering from ActionModule after all the rest handlers have been collected?

@jaymode
Copy link
Member Author

jaymode commented Jan 29, 2020

@rjernst that's exactly my plan and the change I will be working on. It is necessary for part of the approach I plan to take for the initial system indices implementation

jaymode added a commit to jaymode/elasticsearch that referenced this issue Feb 5, 2020
@jaymode
RestHandlers declare handled methods and paths
2a1d841 
This commit changes how RestHandlers are registered with the
RestController so that a RestHandler no longer needs to register itself
with the RestController. Instead the RestHandler interface has new
methods which when called provide information about the method and path
combinations that are handled by the handler including any deprecated
and/or replaced combinations.

This change also makes the publication of RestHandlers safe since they
no longer publish a reference to themselves within their constructors.

Closes elastic#51622
@jaymode jaymode mentioned this issue Feb 5, 2020
Merged
jaymode added a commit to jaymode/elasticsearch that referenced this issue Feb 6, 2020
@jaymode
Squashed commit of the following:
be64008 
commit 771b4e1
Author: jaymode <[email protected]>
Date:   Thu Feb 6 14:41:36 2020 -0700

    fix duplicated api

commit 94f1124
Author: jaymode <[email protected]>
Date:   Thu Feb 6 14:24:01 2020 -0700

    update javadoc

commit 7c14750
Merge: 171e368 f21b641
Author: jaymode <[email protected]>
Date:   Thu Feb 6 14:21:00 2020 -0700

    Merge branch 'master' into rest_handler_safe_register

commit 171e368
Author: jaymode <[email protected]>
Date:   Thu Feb 6 14:20:28 2020 -0700

    refactor to routes

commit 263e2d2
Author: jaymode <[email protected]>
Date:   Wed Feb 5 11:44:24 2020 -0700

    fix inference api

commit 9167512
Author: jaymode <[email protected]>
Date:   Wed Feb 5 10:51:34 2020 -0700

    fixes

commit 2a1d841
Author: jaymode <[email protected]>
Date:   Tue Feb 4 21:18:39 2020 -0700

    RestHandlers declare handled methods and paths

    This commit changes how RestHandlers are registered with the
    RestController so that a RestHandler no longer needs to register itself
    with the RestController. Instead the RestHandler interface has new
    methods which when called provide information about the method and path
    combinations that are handled by the handler including any deprecated
    and/or replaced combinations.

    This change also makes the publication of RestHandlers safe since they
    no longer publish a reference to themselves within their constructors.

    Closes elastic#51622
jaymode added a commit that referenced this issue Feb 10, 2020
@jaymode @jasontedor
RestHandlers declare handled routes (#51950)
627d853 
This commit changes how RestHandlers are registered with the
RestController so that a RestHandler no longer needs to register itself
with the RestController. Instead the RestHandler interface has new
methods which when called provide information about the routes
(method and path combinations) that are handled by the handler
including any deprecated and/or replaced combinations.

This change also makes the publication of RestHandlers safe since they
no longer publish a reference to themselves within their constructors.

Closes #51622

Co-authored-by: Jason Tedor <[email protected]>
@jaymode jaymode mentioned this issue Feb 10, 2020
Merged
jaymode added a commit that referenced this issue Feb 10, 2020
@jaymode
RestHandlers declare handled routes (#52123)
3edadfe 
This commit changes how RestHandlers are registered with the
RestController so that a RestHandler no longer needs to register itself
with the RestController. Instead the RestHandler interface has new
methods which when called provide information about the routes
(method and path combinations) that are handled by the handler
including any deprecated and/or replaced combinations.

This change also makes the publication of RestHandlers safe since they
no longer publish a reference to themselves within their constructors.

Closes #51622

Co-authored-by: Jason Tedor <[email protected]>

Backport of #51950
@jaymode jaymode mentioned this issue Mar 5, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jaymode jaymode

Labels
:Core/Infra/REST API REST infrastructure and utilities >refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

RestHandlers declare handled routes jaymode/elasticsearch
3 participants
@rjernst @jaymode @elasticmachine