From 501da5f6ffad7789d60f7dfb815f3af2990feaba Mon Sep 17 00:00:00 2001 From: Simon Stewart Date: Thu, 12 Jul 2018 22:59:02 +0100 Subject: [PATCH] Hook injector into the server, replacing old DI approach --- .../src/org/openqa/selenium/injector/BUCK | 3 +- .../selenium/remote/server/AllHandlers.java | 63 ++------ .../org/openqa/selenium/remote/server/BUCK | 1 + .../remote/server/AllHandlersTest.java | 144 ------------------ .../remote/server/RemoteServerTests.java | 1 - 5 files changed, 18 insertions(+), 194 deletions(-) delete mode 100644 java/server/test/org/openqa/selenium/remote/server/AllHandlersTest.java diff --git a/java/server/src/org/openqa/selenium/injector/BUCK b/java/server/src/org/openqa/selenium/injector/BUCK index 76db0d20a2990..e19676f147fa5 100644 --- a/java/server/src/org/openqa/selenium/injector/BUCK +++ b/java/server/src/org/openqa/selenium/injector/BUCK @@ -7,6 +7,7 @@ java_library( "//third_party/java/guava:guava", ], visibility = [ - "//java/server/test/org/openqa/selenium/injector:" + "//java/server/src/org/openqa/selenium/...", + "//java/server/test/org/openqa/selenium/injector:", ], ) \ No newline at end of file diff --git a/java/server/src/org/openqa/selenium/remote/server/AllHandlers.java b/java/server/src/org/openqa/selenium/remote/server/AllHandlers.java index f0ab6aced160a..1aa8960d111d9 100644 --- a/java/server/src/org/openqa/selenium/remote/server/AllHandlers.java +++ b/java/server/src/org/openqa/selenium/remote/server/AllHandlers.java @@ -17,13 +17,12 @@ package org.openqa.selenium.remote.server; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Splitter; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; +import org.openqa.selenium.injector.Injector; import org.openqa.selenium.json.Json; import org.openqa.selenium.remote.SessionId; import org.openqa.selenium.remote.http.HttpMethod; @@ -36,15 +35,11 @@ import org.openqa.selenium.remote.server.commandhandler.Status; import org.openqa.selenium.remote.server.commandhandler.UploadFile; -import java.lang.reflect.Constructor; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; -import java.util.Set; import java.util.function.Function; -import java.util.stream.Collectors; -import java.util.stream.Stream; import javax.servlet.http.HttpServletRequest; @@ -52,17 +47,22 @@ class AllHandlers { private final Json json; - private final NewSessionPipeline pipeline; private final ActiveSessions allSessions; + private final Injector parentInjector; private final Map>> additionalHandlers; public AllHandlers(NewSessionPipeline pipeline, ActiveSessions allSessions) { - this.pipeline = pipeline; this.allSessions = Objects.requireNonNull(allSessions); this.json = new Json(); - additionalHandlers = ImmutableMap.of( + this.parentInjector = Injector.builder() + .register(json) + .register(allSessions) + .register(pipeline) + .build(); + + this.additionalHandlers = ImmutableMap.of( HttpMethod.DELETE, ImmutableList.of(), HttpMethod.GET, ImmutableList.of( handler("/session/{sessionId}/log/types", GetLogTypes.class), @@ -120,54 +120,21 @@ private Function handler( return null; } - ImmutableSet.Builder args = ImmutableSet.builder(); - args.add(pipeline); - args.add(allSessions); - args.add(json); + Injector.Builder child = Injector.builder().parent(parentInjector); if (match.getParameters().containsKey("sessionId")) { SessionId id = new SessionId(match.getParameters().get("sessionId")); - args.add(id); + child.register(id); ActiveSession session = allSessions.get(id); if (session != null) { - args.add(session); - args.add(session.getFileSystem()); + child.register(session); + child.register(session.getFileSystem()); } } match.getParameters().entrySet().stream() .filter(e -> !"sessionId".equals(e.getKey())) - .forEach(e -> args.add(e.getValue())); + .forEach(e -> child.register(e.getValue())); - return create(handler, args.build()); + return child.build().newInstance(handler); }; } - - @VisibleForTesting - T create(Class toCreate, Set args) { - Constructor constructor = Stream.of(toCreate.getDeclaredConstructors()) - .peek(c -> c.setAccessible(true)) - .sorted((l, r) -> r.getParameterCount() - l.getParameterCount()) - .filter(c -> - Stream.of(c.getParameters()) - .map(p -> args.stream() - .anyMatch(arg -> p.getType().isAssignableFrom(arg.getClass()))) - .reduce(Boolean::logicalAnd) - .orElse(true)) - .findFirst() - .orElseThrow(() -> new IllegalArgumentException("Cannot find constructor to populate")); - - List parameters = Stream.of(constructor.getParameters()) - .map(p -> args.stream() - .filter(arg -> p.getType().isAssignableFrom(arg.getClass())) - .findFirst() - .orElseThrow(() -> new IllegalArgumentException( - "Cannot find match for " + p + " in " + toCreate))) - .collect(Collectors.toList()); - - try { - Object[] objects = parameters.toArray(); - return (T) constructor.newInstance(objects); - } catch (ReflectiveOperationException e) { - throw new IllegalArgumentException("Cannot invoke constructor", e); - } - } } diff --git a/java/server/src/org/openqa/selenium/remote/server/BUCK b/java/server/src/org/openqa/selenium/remote/server/BUCK index 9aa585ec4c063..69acfd06c93d1 100644 --- a/java/server/src/org/openqa/selenium/remote/server/BUCK +++ b/java/server/src/org/openqa/selenium/remote/server/BUCK @@ -92,6 +92,7 @@ java_library( ':server', ':sessions', '//java/client/src/org/openqa/selenium/remote:remote', + '//java/server/src/org/openqa/selenium/injector:injector', '//java/server/src/org/openqa/selenium/remote/server/log:log', '//java/server/src/org/openqa/selenium/remote/server/jmx:jmx', '//third_party/java/guava:guava', diff --git a/java/server/test/org/openqa/selenium/remote/server/AllHandlersTest.java b/java/server/test/org/openqa/selenium/remote/server/AllHandlersTest.java deleted file mode 100644 index 0c17a637aedb8..0000000000000 --- a/java/server/test/org/openqa/selenium/remote/server/AllHandlersTest.java +++ /dev/null @@ -1,144 +0,0 @@ -// Licensed to the Software Freedom Conservancy (SFC) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The SFC licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -package org.openqa.selenium.remote.server; - -import static java.util.concurrent.TimeUnit.SECONDS; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.fail; - -import com.google.common.collect.ImmutableSet; - -import org.junit.Test; -import org.openqa.selenium.remote.SessionId; -import org.openqa.selenium.remote.http.HttpRequest; -import org.openqa.selenium.remote.http.HttpResponse; - -import java.util.Objects; -import java.util.concurrent.TimeUnit; - -public class AllHandlersTest { - private AllHandlers allHandlers = new AllHandlers( - NewSessionPipeline.builder().add(new ActiveSessionFactory()).create(), - new ActiveSessions(120, SECONDS)); - - static class NoArgs implements CommandHandler { - @Override - public void execute(HttpRequest req, HttpResponse resp) { - // Does nothing - } - } - - @Test - public void canCreateAnObject() { - ActiveSessions sessions = new ActiveSessions(3, TimeUnit.MINUTES); - NoArgs noArgs = allHandlers.create(NoArgs.class, ImmutableSet.of(sessions)); - - assertNotNull(noArgs); - } - - static class SingleArg implements CommandHandler { - private final SessionId id; - - public SingleArg(SessionId id) { - this.id = Objects.requireNonNull(id); - } - - @Override - public void execute(HttpRequest req, HttpResponse resp) { - // Does nothing - } - } - - @Test - public void willCallAConstructorThatTakesAnArgument() { - SessionId id = new SessionId("2345678"); - - SingleArg singleArg = allHandlers.create(SingleArg.class, ImmutableSet.of(id)); - - assertEquals(id, singleArg.id); - } - - static class MultipleArgs implements CommandHandler { - - private final SessionId id; - private final String cake; - - public MultipleArgs(String cake) { - this(null, cake); - } - - public MultipleArgs(SessionId id, String cake) { - this.id = id; - this.cake = cake; - } - - public MultipleArgs(SessionId id) { - this(id, null); - } - - @Override - public void execute(HttpRequest req, HttpResponse resp) { - // Does nothing - } - } - @Test - public void willCallLongestConstructor() { - SessionId id = new SessionId("12345678"); - String cake = "cheese"; - - MultipleArgs multipleArgs = allHandlers.create(MultipleArgs.class, ImmutableSet.of(cake, id)); - - assertEquals(id, multipleArgs.id); - assertEquals(cake, multipleArgs.cake); - } - - static class UnmatchableConstructor implements CommandHandler { - - private SessionId id; - - public UnmatchableConstructor(SessionId id, String cake) { - fail("I shall never be called"); - } - - public UnmatchableConstructor(SessionId id) { - this.id = id; - } - - @Override - public void execute(HttpRequest req, HttpResponse resp) { - // Do nothing - } - } - - @Test - public void shouldOnlyCallLongestConstructorWhereEverParameterCanBeFilled() { - SessionId id = new SessionId("2345678"); - - UnmatchableConstructor unmatchable = allHandlers.create( - UnmatchableConstructor.class, - ImmutableSet.of(id)); - - assertEquals(id, unmatchable.id); - } - - @Test(expected = IllegalArgumentException.class) - public void shouldThrowAnErrorIfThereIsNoCallableConstructor() { - allHandlers.create(SingleArg.class, ImmutableSet.of()); - } -} diff --git a/java/server/test/org/openqa/selenium/remote/server/RemoteServerTests.java b/java/server/test/org/openqa/selenium/remote/server/RemoteServerTests.java index d4e2dee8f4ff7..bac6078fdf1b4 100644 --- a/java/server/test/org/openqa/selenium/remote/server/RemoteServerTests.java +++ b/java/server/test/org/openqa/selenium/remote/server/RemoteServerTests.java @@ -31,7 +31,6 @@ @Suite.SuiteClasses({ ActiveSessionsTest.class, ActiveSessionFactoryTest.class, - AllHandlersTest.class, CapabilitiesComparatorTest.class, ConfigureTimeoutTest.class, CrossDomainRpcLoaderTest.class,