From 9ab327090582ce860c482c253cbb6636840551a6 Mon Sep 17 00:00:00 2001 From: Denis Anisimov Date: Wed, 25 Nov 2020 08:32:46 +0300 Subject: [PATCH 1/6] fix: don't initialize Vaadin servlet until Lookup is not available --- .../com/vaadin/flow/server/VaadinServlet.java | 36 +++++--- .../vaadin/flow/server/VaadinServiceTest.java | 2 + .../vaadin/flow/server/VaadinServletTest.java | 86 +++++++++++++++++++ 3 files changed, 113 insertions(+), 11 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java b/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java index 7905a3f42b1..9b2ad0817bd 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java @@ -16,6 +16,7 @@ package com.vaadin.flow.server; import javax.servlet.ServletConfig; +import javax.servlet.ServletContext; import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; @@ -27,6 +28,7 @@ import java.util.Properties; import com.vaadin.flow.component.UI; +import com.vaadin.flow.di.Lookup; import com.vaadin.flow.function.DeploymentConfiguration; import com.vaadin.flow.internal.CurrentInstance; import com.vaadin.flow.server.HandlerHelper.RequestType; @@ -64,23 +66,35 @@ public class VaadinServlet extends HttpServlet { @Override public void init(ServletConfig servletConfig) throws ServletException { CurrentInstance.clearAll(); - super.init(servletConfig); - try { - servletService = createServletService(); - } catch (ServiceException e) { - throw new ServletException("Could not initialize VaadinServlet", e); + if (getServletConfig() == null) { + super.init(servletConfig); + } + + ServletContext servletContext = getServletConfig().getServletContext(); + if (new VaadinServletContext(servletContext) + .getAttribute(Lookup.class) == null) { + return; } - // Sets current service as it is needed in static file server even - // though there are no request and response. - servletService.setCurrentInstances(null, null); + if (servletService == null) { + try { + servletService = createServletService(); + } catch (ServiceException e) { + throw new ServletException("Could not initialize VaadinServlet", + e); + } - staticFileHandler = createStaticFileHandler(servletService); + // Sets current service as it is needed in static file server even + // though there are no request and response. + servletService.setCurrentInstances(null, null); - servletInitialized(); - CurrentInstance.clearAll(); + staticFileHandler = createStaticFileHandler(servletService); + servletInitialized(); + } + + CurrentInstance.clearAll(); } /** diff --git a/flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java b/flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java index a573e9f74f4..531341e7273 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java @@ -212,6 +212,8 @@ public void nullUrlIsSetToACriticalNotification() { public void serviceContainsStreamRequestHandler() throws ServiceException, ServletException { ServletConfig servletConfig = new MockServletConfig(); + servletConfig.getServletContext().setAttribute(Lookup.class.getName(), + Mockito.mock(Lookup.class)); VaadinServlet servlet = new VaadinServlet() { @Override protected DeploymentConfiguration createDeploymentConfiguration() diff --git a/flow-server/src/test/java/com/vaadin/flow/server/VaadinServletTest.java b/flow-server/src/test/java/com/vaadin/flow/server/VaadinServletTest.java index d975940da47..486ac408ce9 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/VaadinServletTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/VaadinServletTest.java @@ -15,14 +15,20 @@ */ package com.vaadin.flow.server; +import javax.servlet.ServletConfig; import javax.servlet.ServletContext; +import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; +import java.util.concurrent.atomic.AtomicBoolean; + import net.jcip.annotations.NotThreadSafe; import org.junit.Assert; import org.junit.Test; import org.mockito.Mockito; +import com.vaadin.flow.di.Lookup; + @NotThreadSafe public class VaadinServletTest { @@ -64,6 +70,86 @@ public void testGetLastPathParameter() { .getLastPathParameter("http://myhost.com/a;hello/;b=1,c=2/")); } + @Test + public void init_superInitCalledOnce() throws ServletException { + AtomicBoolean called = new AtomicBoolean(); + VaadinServlet servlet = new VaadinServlet() { + + @Override + public void init() throws ServletException { + Assert.assertFalse(called.get()); + called.set(true); + } + }; + + ServletConfig config = mockConfig(); + servlet.init(config); + + Assert.assertTrue(called.get()); + + servlet.init(mockConfig()); + + Assert.assertSame(config, servlet.getServletConfig()); + } + + @Test + public void init_noLookup_servletIsNotInitialized() + throws ServletException { + AtomicBoolean called = new AtomicBoolean(); + VaadinServlet servlet = new VaadinServlet() { + + @Override + protected void servletInitialized() throws ServletException { + called.set(true); + } + }; + + ServletConfig config = mockConfig(); + servlet.init(config); + + Assert.assertFalse(called.get()); + } + + @Test + public void init_contextHasLookup_servletIsInitialized() + throws ServletException { + AtomicBoolean called = new AtomicBoolean(); + VaadinServlet servlet = new VaadinServlet() { + + @Override + protected VaadinServletService createServletService() + throws ServletException, ServiceException { + return Mockito.mock(VaadinServletService.class); + } + + @Override + protected StaticFileHandler createStaticFileHandler( + VaadinServletService servletService) { + return Mockito.mock(StaticFileHandler.class); + } + + @Override + protected void servletInitialized() throws ServletException { + called.set(true); + } + }; + + ServletConfig config = mockConfig(); + ServletContext servletContext = config.getServletContext(); + Mockito.when(servletContext.getAttribute(Lookup.class.getName())) + .thenReturn(Mockito.mock(Lookup.class)); + servlet.init(config); + + Assert.assertTrue(called.get()); + } + + private ServletConfig mockConfig() { + ServletConfig config = Mockito.mock(ServletConfig.class); + ServletContext context = Mockito.mock(ServletContext.class); + Mockito.when(config.getServletContext()).thenReturn(context); + return config; + } + private HttpServletRequest createRequest( MockServletServiceSessionSetup mocks, String path) { HttpServletRequest httpServletRequest = Mockito From fff803728b9e25b66913a7ffddb41e15c63c065c Mon Sep 17 00:00:00 2001 From: Denis Anisimov Date: Thu, 26 Nov 2020 11:27:38 +0300 Subject: [PATCH 2/6] fix : merge two "if" checks and call clearAll in finally block --- .../com/vaadin/flow/server/VaadinServlet.java | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java b/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java index 9b2ad0817bd..7a9d30f9a3c 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java @@ -67,17 +67,19 @@ public class VaadinServlet extends HttpServlet { public void init(ServletConfig servletConfig) throws ServletException { CurrentInstance.clearAll(); - if (getServletConfig() == null) { - super.init(servletConfig); - } + try { + if (getServletConfig() == null) { + super.init(servletConfig); + } - ServletContext servletContext = getServletConfig().getServletContext(); - if (new VaadinServletContext(servletContext) - .getAttribute(Lookup.class) == null) { - return; - } + ServletContext servletContext = getServletConfig() + .getServletContext(); + if (servletService != null + || new VaadinServletContext(servletContext) + .getAttribute(Lookup.class) == null) { + return; + } - if (servletService == null) { try { servletService = createServletService(); } catch (ServiceException e) { @@ -92,9 +94,9 @@ public void init(ServletConfig servletConfig) throws ServletException { staticFileHandler = createStaticFileHandler(servletService); servletInitialized(); + } finally { + CurrentInstance.clearAll(); } - - CurrentInstance.clearAll(); } /** From ae6a15965899c79a8816095fd39eee63a3670da7 Mon Sep 17 00:00:00 2001 From: Denis Anisimov Date: Thu, 26 Nov 2020 11:41:16 +0300 Subject: [PATCH 3/6] fix: prevent call "init" method with different config instance --- .../java/com/vaadin/flow/server/VaadinServlet.java | 6 ++++++ .../com/vaadin/flow/server/VaadinServletTest.java | 13 ++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java b/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java index 7a9d30f9a3c..14c55a0420b 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java @@ -72,6 +72,12 @@ public void init(ServletConfig servletConfig) throws ServletException { super.init(servletConfig); } + if (getServletConfig() != servletConfig) { + throw new IllegalArgumentException( + "Servlet config instance may not differ from the " + + "instance which has been used for the initial method call"); + } + ServletContext servletContext = getServletConfig() .getServletContext(); if (servletService != null diff --git a/flow-server/src/test/java/com/vaadin/flow/server/VaadinServletTest.java b/flow-server/src/test/java/com/vaadin/flow/server/VaadinServletTest.java index 486ac408ce9..763b07b8e44 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/VaadinServletTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/VaadinServletTest.java @@ -87,11 +87,22 @@ public void init() throws ServletException { Assert.assertTrue(called.get()); - servlet.init(mockConfig()); + servlet.init(config); Assert.assertSame(config, servlet.getServletConfig()); } + @Test(expected = IllegalArgumentException.class) + public void init_passDifferentConfigInstance_throws() + throws ServletException { + VaadinServlet servlet = new VaadinServlet(); + + ServletConfig config = mockConfig(); + servlet.init(config); + + servlet.init(mockConfig()); + } + @Test public void init_noLookup_servletIsNotInitialized() throws ServletException { From c8385025cad9150d3fe04dac5a8e3cdeb562bc61 Mon Sep 17 00:00:00 2001 From: Denis Anisimov Date: Thu, 26 Nov 2020 12:31:31 +0300 Subject: [PATCH 4/6] fix: fix unit tests --- .../com/vaadin/flow/server/VaadinServlet.java | 16 ++++++++++++++++ .../flow/server/MockVaadinServletService.java | 4 +++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java b/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java index 14c55a0420b..aa87b97b5c8 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java @@ -68,6 +68,22 @@ public void init(ServletConfig servletConfig) throws ServletException { CurrentInstance.clearAll(); try { + /* + * There are plenty of reasons why the check should be done. The + * main reason is: init method is public which means that everyone + * may call this method at any time (including an app developer). + * But it's not supposed to be called any times any time. + * + * This code protects weak API from being called several times so + * that config is reset after the very first initialization. + * + * Normally "init" method is called only once by the servlet + * container. But in a specific OSGi case {@code + * ServletCointextListener} may be called after the servlet + * initialized. To be able to initialize the VaadinServlet properly + * its "init" method is called from the {@code + * ServletCointextListener} with the same ServletConfig instance. + */ if (getServletConfig() == null) { super.init(servletConfig); } diff --git a/flow-server/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java b/flow-server/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java index 496554a8f65..b6fc5fc7653 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java @@ -109,7 +109,9 @@ public void init() { try { MockVaadinServlet servlet = (MockVaadinServlet) getServlet(); servlet.service = this; - getServlet().init(new MockServletConfig()); + if (getServlet().getServletConfig() == null) { + getServlet().init(new MockServletConfig()); + } super.init(); } catch (ServiceException | ServletException e) { throw new RuntimeException(e); From 3dd49c9b06c1f01fa3eb6c3fc1742c2c88e0f985 Mon Sep 17 00:00:00 2001 From: Denis Anisimov Date: Thu, 26 Nov 2020 13:07:15 +0300 Subject: [PATCH 5/6] fix: fix unit test --- .../com/vaadin/flow/server/MockVaadinServletService.java | 8 +++----- .../com/vaadin/flow/server/MockVaadinServletService.java | 8 +++----- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/flow-lit-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java b/flow-lit-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java index 983474068b2..9dbcf79f95f 100644 --- a/flow-lit-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java +++ b/flow-lit-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java @@ -15,13 +15,13 @@ */ package com.vaadin.flow.server; -import java.util.Collections; -import java.util.List; - import javax.servlet.ServletConfig; import javax.servlet.ServletContext; import javax.servlet.ServletException; +import java.util.Collections; +import java.util.List; + import org.mockito.Mockito; import com.vaadin.flow.di.Instantiator; @@ -95,8 +95,6 @@ protected List createRequestHandlers() public void init(Instantiator instantiator) { this.instantiator = instantiator; - - init(); } @Override diff --git a/flow-polymer-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java b/flow-polymer-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java index 983474068b2..9dbcf79f95f 100644 --- a/flow-polymer-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java +++ b/flow-polymer-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java @@ -15,13 +15,13 @@ */ package com.vaadin.flow.server; -import java.util.Collections; -import java.util.List; - import javax.servlet.ServletConfig; import javax.servlet.ServletContext; import javax.servlet.ServletException; +import java.util.Collections; +import java.util.List; + import org.mockito.Mockito; import com.vaadin.flow.di.Instantiator; @@ -95,8 +95,6 @@ protected List createRequestHandlers() public void init(Instantiator instantiator) { this.instantiator = instantiator; - - init(); } @Override From 4363810e206a44ae9e4960aed0ac22f1ce6281d5 Mon Sep 17 00:00:00 2001 From: Denis Anisimov Date: Thu, 26 Nov 2020 14:30:58 +0300 Subject: [PATCH 6/6] fix: one more tests fix --- .../flow/server/MockVaadinServletService.java | 23 +++++++++++-------- .../flow/server/MockVaadinServletService.java | 23 +++++++++++-------- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/flow-lit-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java b/flow-lit-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java index 9dbcf79f95f..c62b6398d22 100644 --- a/flow-lit-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java +++ b/flow-lit-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java @@ -95,6 +95,8 @@ protected List createRequestHandlers() public void init(Instantiator instantiator) { this.instantiator = instantiator; + + init(); } @Override @@ -110,15 +112,18 @@ public void init() { try { MockVaadinServlet servlet = (MockVaadinServlet) getServlet(); servlet.service = this; - ServletConfig config = Mockito.mock(ServletConfig.class); - ServletContext context = Mockito.mock(ServletContext.class); - Mockito.when(config.getServletContext()).thenReturn(context); - - Mockito.when(lookup.lookup(ResourceProvider.class)) - .thenReturn(resourceProvider); - Mockito.when(context.getAttribute(Lookup.class.getName())) - .thenReturn(lookup); - getServlet().init(config); + + if (getServlet().getServletConfig() == null) { + ServletConfig config = Mockito.mock(ServletConfig.class); + ServletContext context = Mockito.mock(ServletContext.class); + Mockito.when(config.getServletContext()).thenReturn(context); + + Mockito.when(lookup.lookup(ResourceProvider.class)) + .thenReturn(resourceProvider); + Mockito.when(context.getAttribute(Lookup.class.getName())) + .thenReturn(lookup); + getServlet().init(config); + } super.init(); } catch (ServiceException | ServletException e) { throw new RuntimeException(e); diff --git a/flow-polymer-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java b/flow-polymer-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java index 9dbcf79f95f..c62b6398d22 100644 --- a/flow-polymer-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java +++ b/flow-polymer-template/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java @@ -95,6 +95,8 @@ protected List createRequestHandlers() public void init(Instantiator instantiator) { this.instantiator = instantiator; + + init(); } @Override @@ -110,15 +112,18 @@ public void init() { try { MockVaadinServlet servlet = (MockVaadinServlet) getServlet(); servlet.service = this; - ServletConfig config = Mockito.mock(ServletConfig.class); - ServletContext context = Mockito.mock(ServletContext.class); - Mockito.when(config.getServletContext()).thenReturn(context); - - Mockito.when(lookup.lookup(ResourceProvider.class)) - .thenReturn(resourceProvider); - Mockito.when(context.getAttribute(Lookup.class.getName())) - .thenReturn(lookup); - getServlet().init(config); + + if (getServlet().getServletConfig() == null) { + ServletConfig config = Mockito.mock(ServletConfig.class); + ServletContext context = Mockito.mock(ServletContext.class); + Mockito.when(config.getServletContext()).thenReturn(context); + + Mockito.when(lookup.lookup(ResourceProvider.class)) + .thenReturn(resourceProvider); + Mockito.when(context.getAttribute(Lookup.class.getName())) + .thenReturn(lookup); + getServlet().init(config); + } super.init(); } catch (ServiceException | ServletException e) { throw new RuntimeException(e);