Skip to content

Try to drop EndpointExposure #10176

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

Closed
philwebb opened this issue Sep 6, 2017 · 6 comments
Closed

Try to drop EndpointExposure #10176

philwebb opened this issue Sep 6, 2017 · 6 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@philwebb
Copy link
Member

philwebb commented Sep 6, 2017

It might be possible to drop the EndpointExposure enum and replace it with a class. Currently it's somewhat limiting in that additional items can't be added easily.

We might be able to use Class<? extends AnnotationEndpointDiscoverer>.

@philwebb philwebb modified the milestone: 2.0.0.M5 Sep 6, 2017
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed priority: normal type: enhancement A general enhancement labels Sep 6, 2017
@philwebb
Copy link
Member Author

philwebb commented Sep 6, 2017

The enum really reflects reality. For example, we need to generate *.jmx and *.web property meta-data. We probably shouldn't try to over complicate things.

@snicoll snicoll closed this as completed Sep 6, 2017
@philwebb philwebb reopened this Sep 12, 2017
@philwebb philwebb added for: team-attention An issue we'd like other members of the team to review priority: normal and removed status: declined A suggestion or change that we don't feel we should currently apply labels Sep 12, 2017
@philwebb philwebb added this to the 2.0.0.M5 milestone Sep 12, 2017
@philwebb
Copy link
Member Author

Actually, now the package restructure is in I would like to reconsider our options. Something doesn't feel quite right about EndpointExposure, especially as it's really only used to limit a couple of endpoints.

The jmx and web meta-data might also be worth a revisit. Enabling on a per technology basis is adding complexity. Perhaps we can just get away with a single enabled?

@philwebb philwebb added the type: enhancement A general enhancement label Sep 12, 2017
@wilkinsona
Copy link
Member

Perhaps we can just get away with a single enabled?

The need for per-technology enablement is driven by the shutdown endpoint. For an app that doesn't need HTTP security, I think it's useful to be able to only expose the shutdown endpoint over JMX otherwise anyone with HTTP access to the app could shut it down.

@snicoll
Copy link
Member

snicoll commented Sep 22, 2017

I second that and disagree with the complex argument. Things are located in a single class and the contract is isolated. I agree this particular class is a bit complex but that doesn't leak elsewhere.

@philwebb philwebb removed the type: enhancement A general enhancement label Sep 22, 2017
@philwebb
Copy link
Member Author

I'll try to spike something then we can make a decision.

@philwebb philwebb added type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review labels Sep 22, 2017
@philwebb philwebb self-assigned this Sep 22, 2017
@philwebb philwebb modified the milestones: 2.0.0.M5, 2.0.0.M6 Oct 10, 2017
@philwebb philwebb modified the milestones: 2.0.0.M6, 2.0.0.RC1 Nov 1, 2017
@philwebb
Copy link
Member Author

philwebb commented Nov 8, 2017

Still a way to go, but I've started experimenting here.

Trying an annotation approach for both specializations and extensions:

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
@Documented
@EndpointExtension(filter = WebEndpointFilter.class) // Extension that only applies when the filter matches
public @interface EndpointWebExtension {

	@AliasFor(annotation = EndpointExtension.class)
	Class<?> endpoint();

}
@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
@Documented
@Endpoint
@FilteredEndpoint(WebEndpointFilter.class) // Endpoint only discovered when the filter matches
public @interface WebEndpoint {

	@AliasFor(annotation = Endpoint.class)
	String id();

	@AliasFor(annotation = Endpoint.class)
	boolean enableByDefault() default true;

}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants