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

refactor: use StaticFileHandler as a service #10229

Merged
merged 5 commits into from Mar 10, 2021

Conversation

denis-anisimov
Copy link
Contributor

Part of vaadin/osgi#50

@denis-anisimov denis-anisimov marked this pull request as ready for review March 9, 2021 09:28
caalador
caalador previously approved these changes Mar 9, 2021
@caalador
Copy link
Contributor

caalador commented Mar 9, 2021

java.lang.IllegalStateException: Found several implementations in the classpath for ApplicationConfigurationFactory SPI: [class com.vaadin.flow.server.startup.DefaultApplicationConfigurationFactory, class com.vaadin.flow.server.startup.LookupServletContainerInitializerTest$TestApplicationConfigurationFactory]. Only one implementation should be registered. Use lookupAll to get all instances of the given type.
	at com.vaadin.flow.di.LookupInitializer.ensureService(LookupInitializer.java:321)
	at com.vaadin.flow.di.LookupInitializer.initialize(LookupInitializer.java:275)
	at com.vaadin.flow.server.startup.LookupServletContainerInitializer.process(LookupServletContainerInitializer.java:85)
	at com.vaadin.flow.server.startup.LookupServletContainerInitializerTest.mockLookup(LookupServletContainerInitializerTest.java:223)
	at com.vaadin.flow.server.startup.LookupServletContainerInitializerTest.mockLookup(LookupServletContainerInitializerTest.java:232)
	at com.vaadin.flow.server.startup.LookupServletContainerInitializerTest.processApplicationConfigurationFactory_factoryIsProvided_providedFactoryIsCreated(LookupServletContainerInitializerTest.java:160)

@denis-anisimov
Copy link
Contributor Author

java.lang.IllegalStateException: Found several implementations in the classpath for ApplicationConfigurationFactory SPI: [class com.vaadin.flow.server.startup.DefaultApplicationConfigurationFactory, class com.vaadin.flow.server.startup.LookupServletContainerInitializerTest$TestApplicationConfigurationFactory]. Only one implementation should be registered. Use lookupAll to get all instances of the given type.
	at com.vaadin.flow.di.LookupInitializer.ensureService(LookupInitializer.java:321)
	at com.vaadin.flow.di.LookupInitializer.initialize(LookupInitializer.java:275)
	at com.vaadin.flow.server.startup.LookupServletContainerInitializer.process(LookupServletContainerInitializer.java:85)
	at com.vaadin.flow.server.startup.LookupServletContainerInitializerTest.mockLookup(LookupServletContainerInitializerTest.java:223)
	at com.vaadin.flow.server.startup.LookupServletContainerInitializerTest.mockLookup(LookupServletContainerInitializerTest.java:232)
	at com.vaadin.flow.server.startup.LookupServletContainerInitializerTest.processApplicationConfigurationFactory_factoryIsProvided_providedFactoryIsCreated(LookupServletContainerInitializerTest.java:160)

Done (I hope).

@caalador
Copy link
Contributor

caalador commented Mar 9, 2021

Now Lit template tests fail LitTemplateParserImplTest

@denis-anisimov
Copy link
Contributor Author

Now Lit template tests fail LitTemplateParserImplTest

yeah, yeah, it will be a long journey. All the modules which use VaadinServlet for some reason may fail.
So there will be many iterations.

if (request instanceof HttpServletRequest && ((HttpServletRequest) request).getRequestedSessionId() == null) {
if (request instanceof HttpServletRequest
&& ((HttpServletRequest) request)
.getRequestedSessionId() == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRITICAL Remove use of this unsecured "getRequestedSessionId()" method rule

@@ -51,7 +52,7 @@
DeprecatedPolymerPublishedEventHandler.class,
Copy link
Collaborator

Choose a reason for hiding this comment

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

MINOR Remove this use of "DeprecatedPolymerPublishedEventHandler"; it is deprecated. rule

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 7 issues

  • CRITICAL 3 critical
  • MAJOR 1 major
  • MINOR 3 minor

Watch the comments in this conversation to review them.

5 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. CRITICAL LookupInitializer.java#L166: Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation. rule
  2. CRITICAL LookupInitializer.java#L244: Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation. rule
  3. MAJOR LookupServletContainerInitializer.java#L89: "servletContext" is a method parameter, and should not be used for synchronization. rule
  4. MINOR VaadinServlet.java#L196: Remove the declaration of thrown exception 'javax.servlet.ServletException', as it cannot be thrown from method's body. rule
  5. MINOR VaadinServlet.java#L546: Replace this lambda with a method reference. rule

@denis-anisimov denis-anisimov merged commit 168a9f8 into master Mar 10, 2021
2 of 3 checks passed
@denis-anisimov denis-anisimov deleted the osgi-50-static-resources branch March 10, 2021 08:10
@vaadin-bot
Copy link
Collaborator

Hi @denis-anisimov , this commit cannot be picked to 6.0 by this bot, can you take a look and pick it manually?

denis-anisimov pushed a commit that referenced this pull request Mar 10, 2021
refactor: use StaticFileHandler as a service

Part of vaadin/osgi#50
denis-anisimov pushed a commit that referenced this pull request Mar 10, 2021
* refactor: use StaticFileHandler as a service (#10229)

Part of vaadin/osgi#50
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.

None yet

3 participants