Skip to content

Commit

Permalink
Merge pull request #4959 from eclipse/jetty-9.4.x-4903-fix-public-end…
Browse files Browse the repository at this point in the history 
…point-check

Issue #4903 - Improved behavior for Custom ServerEndpointConfig.Configurator
  • Loading branch information
@joakime
joakime authored Jun 11, 2020
2 parents c48aee0 + 66ef0eb commit de6273b
Show file tree
Hide file tree
Showing 5 changed files with 188 additions and 11 deletions.
2 changes: 1 addition & 1 deletion ...rc/main/java/org/eclipse/jetty/websocket/jsr356/server/AnnotatedServerEndpointConfig.java
View file
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
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
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
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
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

0 comments on commit de6273b

Please sign in to comment.