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

Issue #4903 - Improved behavior for Custom ServerEndpointConfig.Configurator #4959

Merged
merged 4 commits into from
Jun 11, 2020
Merged

Issue #4903 - Improved behavior for Custom ServerEndpointConfig.Configurator #4959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9bfc168
Issue #4903 - Improved behavior for Custom ServerEndpointConfig.Confi…
joakime Jun 10, 2020
b22e306
Issue #4903 - Improved behavior for Custom ServerEndpointConfig.Confi…
joakime Jun 10, 2020
f41f601
Issue #4903 - fix validation on custom Configurator annotated endpoint
lachlan-roberts Jun 10, 2020
66ef0eb
Issue #4903 - do ServerEndpointConfig validation check before started
lachlan-roberts Jun 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...rc/main/java/org/eclipse/jetty/websocket/jsr356/server/AnnotatedServerEndpointConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ else if (anno.configurator() == ServerEndpointConfig.Configurator.class)
{
try
{
resolvedConfigurator = anno.configurator().getDeclaredConstructor().newInstance();
resolvedConfigurator = anno.configurator().getConstructor().newInstance();
}
catch (Exception e)
{
Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/org/eclipse/jetty/websocket/jsr356/server/ContainerDefaultConfigurator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public <T> T getEndpointInstance(Class<T> endpointClass) throws InstantiationExc
{
// Since this is started via a ServiceLoader, this class has no Scope or context
// that can be used to obtain a ObjectFactory from.
return endpointClass.getDeclaredConstructor().newInstance();
return endpointClass.getConstructor().newInstance();
}
catch (Exception e)
{
Expand Down
46 changes: 42 additions & 4 deletions ...-server-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/server/ServerContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,51 @@ public EndpointInstance newClientEndpointInstance(Object endpoint, ServerEndpoin
return new EndpointInstance(endpoint, cec, metadata);
}

private void validateEndpointConfig(ServerEndpointConfig config) throws DeploymentException
{
if (config == null)
{
throw new DeploymentException("Unable to deploy null ServerEndpointConfig");
}

ServerEndpointConfig.Configurator configurator = config.getConfigurator();
if (configurator == null)
{
throw new DeploymentException("Unable to deploy with null ServerEndpointConfig.Configurator");
}

Class<?> endpointClass = config.getEndpointClass();
if (endpointClass == null)
{
throw new DeploymentException("Unable to deploy null endpoint class from ServerEndpointConfig: " + config.getClass().getName());
}

if (configurator.getClass() == ContainerDefaultConfigurator.class)
{
if (!ReflectUtils.isDefaultConstructable(endpointClass))
{
throw new DeploymentException("Cannot access default constructor for the class: " + endpointClass.getName());
}
}
}

@Override
public void addEndpoint(Class<?> endpointClass) throws DeploymentException
{
if (endpointClass == null)
{
throw new DeploymentException("Unable to deploy null endpoint class");
}

if (isStarted() || isStarting())
{
if (LOG.isDebugEnabled())
{
LOG.debug("addEndpoint({})", endpointClass);
}

ServerEndpointMetadata metadata = getServerEndpointMetadata(endpointClass, null);
validateEndpointConfig(metadata.getConfig());
addEndpoint(metadata);
}
else
Expand All @@ -136,24 +175,23 @@ public void addEndpoint(Class<?> endpointClass) throws DeploymentException
}
}

private void addEndpoint(ServerEndpointMetadata metadata) throws DeploymentException
private void addEndpoint(ServerEndpointMetadata metadata)
{
if (!ReflectUtils.isDefaultConstructable(metadata.getEndpointClass()))
throw new DeploymentException("Cannot access default constructor for the Endpoint class");

JsrCreator creator = new JsrCreator(this, metadata, this.configuration.getFactory().getExtensionFactory());
this.configuration.addMapping("uri-template|" + metadata.getPath(), creator);
}

@Override
public void addEndpoint(ServerEndpointConfig config) throws DeploymentException
{
validateEndpointConfig(config);
if (isStarted() || isStarting())
{
if (LOG.isDebugEnabled())
{
LOG.debug("addEndpoint({}) path={} endpoint={}", config, config.getPath(), config.getEndpointClass());
}

ServerEndpointMetadata metadata = getServerEndpointMetadata(config.getEndpointClass(), config);
addEndpoint(metadata);
}
Expand Down
147 changes: 142 additions & 5 deletions ...et/jsr356/server/PrivateEndpointTest.java → ...socket/jsr356/server/AddEndpointTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@

package org.eclipse.jetty.websocket.jsr356.server;

import java.util.concurrent.TimeUnit;
import javax.websocket.CloseReason;
import javax.websocket.ContainerProvider;
import javax.websocket.DeploymentException;
import javax.websocket.Endpoint;
import javax.websocket.EndpointConfig;
import javax.websocket.MessageHandler;
import javax.websocket.OnMessage;
import javax.websocket.OnOpen;
import javax.websocket.Session;
import javax.websocket.WebSocketContainer;
import javax.websocket.server.ServerEndpoint;
Expand All @@ -33,17 +36,22 @@
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.util.component.LifeCycle;
import org.eclipse.jetty.websocket.api.util.WSURI;
import org.eclipse.jetty.websocket.jsr356.server.deploy.WebSocketServerContainerInitializer;
import org.eclipse.jetty.websocket.jsr356.server.samples.BasicOpenCloseSocket;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.instanceOf;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class PrivateEndpointTest
public class AddEndpointTest
{
private Server server;
private WebSocketContainer client;
Expand Down Expand Up @@ -94,6 +102,59 @@ public void onMessage(String message)
}
}

@SuppressWarnings("InnerClassMayBeStatic")
private class CustomPrivateEndpoint extends Endpoint
{
@Override
public void onOpen(Session session, EndpointConfig config)
{
}
}

@SuppressWarnings("InnerClassMayBeStatic")
@ServerEndpoint(value = "/", configurator = CustomAnnotatedEndpointConfigurator.class)
public static class CustomAnnotatedEndpoint
{
public CustomAnnotatedEndpoint(String id)
{
}

@OnOpen
public void onOpen(Session session, EndpointConfig config)
{
}
}

public static class CustomAnnotatedEndpointConfigurator extends ServerEndpointConfig.Configurator
{
@SuppressWarnings("unchecked")
@Override
public <T> T getEndpointInstance(Class<T> endpointClass)
{
return (T)new CustomAnnotatedEndpoint("server");
}
}

public static class CustomEndpoint extends Endpoint implements MessageHandler.Whole<String>
{
public CustomEndpoint(String id)
{
// This is a valid no-default-constructor implementation, and can be added via a custom
// ServerEndpointConfig.Configurator
}

@Override
public void onOpen(Session session, EndpointConfig config)
{
session.addMessageHandler(this);
}

@Override
public void onMessage(String message)
{
}
}

@SuppressWarnings("InnerClassMayBeStatic")
public class ServerSocketNonStatic extends Endpoint implements MessageHandler.Whole<String>
{
Expand Down Expand Up @@ -131,11 +192,87 @@ private void onMessage(String message)
public void testEndpoint()
{
RuntimeException error = assertThrows(RuntimeException.class, () ->
start(container -> container.addEndpoint(ServerEndpointConfig.Builder.create(ServerSocket.class, "/").build())));
{
ServerEndpointConfig config = ServerEndpointConfig.Builder.create(ServerSocket.class, "/").build();
start(container -> container.addEndpoint(config));
});

assertThat(error.getCause(), instanceOf(DeploymentException.class));
DeploymentException deploymentException = (DeploymentException)error.getCause();
assertThat(deploymentException.getMessage(), containsString("Cannot access default constructor"));
}

@Test
public void testCustomEndpoint() throws Exception
{
ServerEndpointConfig config = ServerEndpointConfig.Builder.create(CustomEndpoint.class, "/")
.configurator(new ServerEndpointConfig.Configurator()
{
@SuppressWarnings("unchecked")
@Override
public <T> T getEndpointInstance(Class<T> endpointClass)
{
return (T)new CustomEndpoint("server");
}
}).build();
start(container -> container.addEndpoint(config));

BasicOpenCloseSocket clientEndpoint = new BasicOpenCloseSocket();
Session session = client.connectToServer(clientEndpoint, WSURI.toWebsocket(server.getURI().resolve("/")));
assertNotNull(session);
session.close();
assertTrue(clientEndpoint.closeLatch.await(5, TimeUnit.SECONDS));
assertThat(clientEndpoint.closeReason.getCloseCode(), Matchers.is(CloseReason.CloseCodes.NORMAL_CLOSURE));
}

@Test
public void testCustomPrivateEndpoint() throws Exception
{
ServerEndpointConfig config = ServerEndpointConfig.Builder.create(CustomPrivateEndpoint.class, "/")
.configurator(new ServerEndpointConfig.Configurator()
{
@SuppressWarnings("unchecked")
@Override
public <T> T getEndpointInstance(Class<T> endpointClass)
{
return (T)new CustomPrivateEndpoint();
}
}).build();
start(container -> container.addEndpoint(config));

BasicOpenCloseSocket clientEndpoint = new BasicOpenCloseSocket();
Session session = client.connectToServer(clientEndpoint, WSURI.toWebsocket(server.getURI().resolve("/")));
assertNotNull(session);
session.close();
assertTrue(clientEndpoint.closeLatch.await(5, TimeUnit.SECONDS));
assertThat(clientEndpoint.closeReason.getCloseCode(), Matchers.is(CloseReason.CloseCodes.NORMAL_CLOSURE));
}

@Test
public void testCustomAnnotatedEndpoint() throws Exception
{
start(container -> container.addEndpoint(CustomAnnotatedEndpoint.class));

BasicOpenCloseSocket clientEndpoint = new BasicOpenCloseSocket();
Session session = client.connectToServer(clientEndpoint, WSURI.toWebsocket(server.getURI().resolve("/")));
assertNotNull(session);
session.close();
assertTrue(clientEndpoint.closeLatch.await(5, TimeUnit.SECONDS));
assertThat(clientEndpoint.closeReason.getCloseCode(), Matchers.is(CloseReason.CloseCodes.NORMAL_CLOSURE));
}

@Test
public void testCustomEndpointNoConfigurator()
{
RuntimeException error = assertThrows(RuntimeException.class, () ->
{
ServerEndpointConfig config = ServerEndpointConfig.Builder.create(CustomEndpoint.class, "/").build();
start(container -> container.addEndpoint(config));
});

assertThat(error.getCause(), instanceOf(DeploymentException.class));
DeploymentException deploymentException = (DeploymentException)error.getCause();
assertThat(deploymentException.getMessage(), containsString("Cannot access default constructor for the Endpoint class"));
assertThat(deploymentException.getMessage(), containsString("Cannot access default constructor"));
}

@Test
Expand All @@ -146,7 +283,7 @@ public void testInnerEndpoint()

assertThat(error.getCause(), instanceOf(DeploymentException.class));
DeploymentException deploymentException = (DeploymentException)error.getCause();
assertThat(deploymentException.getMessage(), containsString("Cannot access default constructor for the Endpoint class"));
assertThat(deploymentException.getMessage(), containsString("Cannot access default constructor"));
}

@Test
Expand All @@ -157,7 +294,7 @@ public void testAnnotatedEndpoint()

assertThat(error.getCause(), instanceOf(DeploymentException.class));
DeploymentException deploymentException = (DeploymentException)error.getCause();
assertThat(deploymentException.getMessage(), containsString("Cannot access default constructor for the Endpoint class"));
assertThat(deploymentException.getMessage(), containsString("Cannot access default constructor"));
}

@Test
Expand Down
2 changes: 2 additions & 0 deletions ...src/test/java/org/eclipse/jetty/websocket/jsr356/server/samples/BasicOpenCloseSocket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

package org.eclipse.jetty.websocket.jsr356.server.samples;

import javax.websocket.ClientEndpoint;
import javax.websocket.CloseReason;
import javax.websocket.OnClose;
import javax.websocket.OnOpen;
Expand All @@ -26,6 +27,7 @@
import org.eclipse.jetty.websocket.jsr356.server.TrackingSocket;

@ServerEndpoint(value = "/basic")
@ClientEndpoint
public class BasicOpenCloseSocket extends TrackingSocket
{
@OnOpen
Expand Down