Skip to content

Difficult to reason about application context type in SpringApplication #14814

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
dsyer opened this issue Oct 14, 2018 · 6 comments
Closed

Difficult to reason about application context type in SpringApplication #14814

dsyer opened this issue Oct 14, 2018 · 6 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@dsyer
Copy link
Member

dsyer commented Oct 14, 2018

I wanted to write a listener that ensured the application context type in a SpringApplication was not annotation-driven (the default setting). It almost works to simply inspect the webApplicationType (which has a public accessor) because the default application context type is strongly correlated with that, but not quite. It doesn't work in tests that expect a mock server (the default) because the test context loader sets the webApplicationType to REACTIVE (until recently it was NONE by accident), and then you have to guess that the user wants to run a real server and get it wrong (the server fails to start, but it's not supposed to even be created in the test).

We could fix this (for me) by simply providing a public accessor for the applicationContextType in SpringApplication. I have worked around it using reflection to access that field. I'm open to other ideas.

My listener currently, for reference, in case it helps:

if (event instanceof ApplicationEnvironmentPreparedEvent) {
	ApplicationEnvironmentPreparedEvent prepared = (ApplicationEnvironmentPreparedEvent) event;
	if (!isEnabled(prepared.getEnvironment())) {
			return;
	}
	SpringApplication application = prepared.getSpringApplication();
	WebApplicationType type = application.getWebApplicationType();
	Class<?> contextType = getApplicationContextType(application);
	if (type == WebApplicationType.NONE) {
		if (contextType == AnnotationConfigApplicationContext.class) {
			application
					.setApplicationContextClass(GenericApplicationContext.class);
		}
	}
	else if (type == WebApplicationType.REACTIVE) {
		if (contextType == AnnotationConfigReactiveWebApplicationContext.class) {
			application.setApplicationContextClass(
					ReactiveWebServerApplicationContext.class);
		}
	}
	else if (type == WebApplicationType.SERVLET) {
		if (contextType == AnnotationConfigServletWebServerApplicationContext.class) {
			application.setApplicationContextClass(
					ServletWebServerApplicationContext.class);
		}
	}
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 14, 2018
@snicoll
Copy link
Member

snicoll commented Oct 14, 2018

In the case of a test it is not that simple. There are actually more flavours of this as indicated by the WebEnvironment enum:

  • No web support at all
  • Mocking the web environment (with support for both Servlet and Reactive)
  • Starting a full web environment, either on a defined port or a random port (ditto)

The real problem IMO is that we still have a few places where the logic is handled manually rather than derive from something that was set upstream. I wonder now if changing that NONE to REACTIVE doesn't lead to a regression in integration tests.

@dsyer
Copy link
Member Author

dsyer commented Oct 14, 2018

I think that REACTIVE is still the best setting for that property in the test (and it works in regular Spring Boot apps). The problem is really that the complete state of SpringApplication is not knowable from outside, and sometimes you need to reason about it. We could either add more public accessors and let users figure out the state themselves, or, as you say, make the state more expressive.

@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 20, 2019
@wilkinsona wilkinsona added this to the 2.x milestone Mar 20, 2019
@andresetevejob
Copy link

I want to work on this issue

@snicoll snicoll added the status: pending-design-work Needs design work before any code can be developed label Apr 3, 2019
@snicoll
Copy link
Member

snicoll commented Apr 3, 2019

@andresetevejob thanks for the offer but with RSocket support this will have to be revisited one way or the other (ping @bclozel). I've added pending-design-work to make it clear we need to make up our mind on it.

@bclozel
Copy link
Member

bclozel commented Apr 11, 2019

I've closed #16412 as we don't need to do that anymore. The REACTIVE type might not be strictly necessary, so we still need to make up our mind about it - this time with the fact that it seems that RSocket apps don't really need their own application type to work.

@philwebb
Copy link
Member

philwebb commented May 4, 2022

We're cleaning out the issue tracker and closing issues that we've not seen much demand to fix. Feel free to comment with additional justifications if you feel that this one should not have been closed.

@philwebb philwebb closed this as completed May 4, 2022
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed type: bug A general bug status: pending-design-work Needs design work before any code can be developed labels May 4, 2022
@philwebb philwebb removed this from the 2.x milestone May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

7 participants