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

feat: Vaadin command interceptor #17738

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
31 changes: 27 additions & 4 deletions flow-server/src/main/java/com/vaadin/flow/server/FutureAccess.java
Original file line number Diff line number Diff line change
@@ -15,11 +15,14 @@
*/
package com.vaadin.flow.server;

import java.util.concurrent.ExecutionException;
import java.util.concurrent.FutureTask;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.FutureTask;

/**
* Encapsulates a {@link Command} submitted using
* {@link VaadinSession#access(Command)}. This class is used internally by the
@@ -31,6 +34,9 @@
public class FutureAccess extends FutureTask<Void> {
private final VaadinSession session;
private final Command command;
private final Iterable<VaadinCommandInterceptor> interceptors;
private final Map<Object, Object> context = new HashMap<>(); // TODO:
Copy link
Contributor

Choose a reason for hiding this comment

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

When FutureAccess objects are invoked, the session to which they belong is locked. The lifespan of context is scoped to a single FutureAccess object. Thus, I think there is no need to make this collection concurrent, because looks like only one thread invokes run, handleError and get method at any point in time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any examples of what can be shared between interceptors via this context?

// ConcurrentHashMap?

/**
* Creates an instance for the given command.
@@ -40,10 +46,19 @@ public class FutureAccess extends FutureTask<Void> {
* @param command
* the command to run when this task is purged from the queue
*/
public FutureAccess(VaadinSession session, Command command) {
public FutureAccess(Iterable<VaadinCommandInterceptor> interceptors,
VaadinSession session, Command command) {
super(command::execute, null);
this.session = session;
this.command = command;
this.interceptors = interceptors;
}

@Override
public void run() {
this.interceptors.forEach(interceptor -> interceptor
.commandExecutionStart(context, command));
super.run();
}

@Override
@@ -59,7 +74,10 @@ public Void get() throws InterruptedException, ExecutionException {
* easier to detect potential problems.
*/
VaadinService.verifyNoOtherSessionLocked(session);
return super.get();
Void unused = super.get();
interceptors.forEach(interceptor -> interceptor
.commandExecutionEnd(context, command));
return unused;
}

/**
@@ -70,6 +88,8 @@ public Void get() throws InterruptedException, ExecutionException {
*/
public void handleError(Exception exception) {
try {
interceptors.forEach(interceptor -> interceptor
.handleException(context, command, exception));
if (command instanceof ErrorHandlingCommand) {
ErrorHandlingCommand errorHandlingCommand = (ErrorHandlingCommand) command;

@@ -88,6 +108,9 @@ public void handleError(Exception exception) {
}
} catch (Exception e) {
getLogger().error(e.getMessage(), e);
} finally {
interceptors.forEach(interceptor -> interceptor
.commandExecutionEnd(context, command));
}
}

Original file line number Diff line number Diff line change
@@ -39,6 +39,7 @@ public class ServiceInitEvent extends EventObject {
private List<IndexHtmlRequestListener> addedIndexHtmlRequestListeners = new ArrayList<>();
private List<DependencyFilter> addedDependencyFilters = new ArrayList<>();
private List<VaadinRequestInterceptor> addedVaadinRequestInterceptors = new ArrayList<>();
private List<VaadinCommandInterceptor> addedVaadinCommandInterceptor = new ArrayList<>();

/**
* Creates a new service init event for a given {@link VaadinService} and
@@ -107,6 +108,20 @@ public void addVaadinRequestInterceptor(
addedVaadinRequestInterceptors.add(vaadinRequestInterceptor);
}

/**
* Adds a new command interceptor that will be used by this service.
*
* @param vaadinCommandInterceptor
* the interceptor to add, not <code>null</code>
*/
public void addVaadinCommandInterceptor(
VaadinCommandInterceptor vaadinCommandInterceptor) {
Objects.requireNonNull(vaadinCommandInterceptor,
"Command Interceptor cannot be null");

addedVaadinCommandInterceptor.add(vaadinCommandInterceptor);
}

/**
* Gets a stream of all custom request handlers that have been added for the
* service.
@@ -147,6 +162,16 @@ public Stream<VaadinRequestInterceptor> getAddedVaadinRequestInterceptor() {
return addedVaadinRequestInterceptors.stream();
}

/**
* Gets a stream of all Vaadin command interceptors that have been added for
* the service.
*
* @return the stream of added command interceptors
*/
public Stream<VaadinCommandInterceptor> getAddedVaadinCommandInterceptor() {
return addedVaadinCommandInterceptor.stream();
}

@Override
public VaadinService getSource() {
return (VaadinService) super.getSource();
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright 2000-2023 Vaadin Ltd.
*
* Licensed 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 com.vaadin.flow.server;

import java.io.Serializable;
import java.util.Map;

/**
* Used to provide an around-like aspect option around command processing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Used to provide an around-like aspect option around command processing.
* Used to provide an around-like aspect option around processing of a {@link Command} submitted using {@link VaadinSession#access(Command)}.

*
* @author Marcin Grzejszczak
* @since 24.2
*/
public interface VaadinCommandInterceptor extends Serializable {

/**
* Called when command is about to be started.
*
* @param context
* mutable map passed between methods of this interceptor
* @param command
* command
*/
void commandExecutionStart(Map<Object, Object> context, Command command);
Copy link
Contributor

Choose a reason for hiding this comment

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

How Command object would help developers? It's just a callback that give no information.
I'd rather pass to these methods something meaningful like session ID or any other information that helps to identify the command from another side rather than callback.

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to pass the whole VaadinSession object as a parameter instead of Command.


/**
* Called when an exception occurred
*
* @param context
* mutable map passed between methods of this interceptor
* @param command
* command
* @param t
* exception
*/
void handleException(Map<Object, Object> context, Command command,
Exception t);

/**
* Called at the end of processing a command. Will be called regardless of
* whether there was an exception or not.
*
* @param context
* mutable map passed between methods of this interceptor
* @param command
* command
*/
void commandExecutionEnd(Map<Object, Object> context, Command command);
}
Original file line number Diff line number Diff line change
@@ -181,6 +181,8 @@ public abstract class VaadinService implements Serializable {

private Iterable<VaadinRequestInterceptor> vaadinRequestInterceptors;

private Iterable<VaadinCommandInterceptor> vaadinCommandInterceptors;

/**
* Creates a new vaadin service based on a deployment configuration.
*
@@ -225,6 +227,7 @@ public void init() throws ServiceException {
// list
// and append ones from the ServiceInitEvent
List<VaadinRequestInterceptor> requestInterceptors = createVaadinRequestInterceptors();
List<VaadinCommandInterceptor> commandInterceptors = createVaadinCommandInterceptor();

ServiceInitEvent event = new ServiceInitEvent(this);

@@ -248,6 +251,14 @@ public void init() throws ServiceException {
vaadinRequestInterceptors = Collections
.unmodifiableCollection(requestInterceptors);

event.getAddedVaadinCommandInterceptor()
.forEach(commandInterceptors::add);

Collections.reverse(commandInterceptors);

vaadinCommandInterceptors = Collections
.unmodifiableCollection(commandInterceptors);

dependencyFilters = Collections.unmodifiableCollection(instantiator
.getDependencyFilters(event.getAddedDependencyFilters())
.collect(Collectors.toList()));
@@ -352,6 +363,22 @@ protected List<VaadinRequestInterceptor> createVaadinRequestInterceptors()
return new ArrayList<>();
}

/**
* Called during initialization to add the request handlers for the service.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should say "command interceptors", not "request interceptors"

* Note that the returned list will be reversed so the last interceptor will
* be called first. This enables overriding this method and using add on the
* returned list to add a custom interceptors which overrides any predefined
* handler.
*
* @return The list of request handlers used by this service.
* @throws ServiceException
* if a problem occurs when creating the request interceptors
*/
protected List<VaadinCommandInterceptor> createVaadinCommandInterceptor()
throws ServiceException {
return new ArrayList<>();
}

/**
* Creates an instantiator to use with this service.
* <p>
@@ -2004,7 +2031,8 @@ public static boolean isCsrfTokenValid(UI ui, String requestToken) {
* @see VaadinSession#access(Command)
*/
public Future<Void> accessSession(VaadinSession session, Command command) {
FutureAccess future = new FutureAccess(session, command);
FutureAccess future = new FutureAccess(vaadinCommandInterceptors,
session, command);
session.getPendingAccessQueue().add(future);

ensureAccessQueuePurged(session);
118 changes: 92 additions & 26 deletions flow-server/src/test/java/com/vaadin/flow/component/UITest.java
Original file line number Diff line number Diff line change
@@ -16,31 +16,6 @@

package com.vaadin.flow.component;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;

import org.hamcrest.CoreMatchers;
import org.hamcrest.MatcherAssert;
import org.junit.After;
import org.junit.Assert;
import org.junit.Ignore;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
import org.mockito.ArgumentMatchers;
import org.mockito.Mockito;
import org.slf4j.Logger;

import com.vaadin.flow.component.internal.PendingJavaScriptInvocation;
import com.vaadin.flow.component.page.History;
import com.vaadin.flow.component.page.History.HistoryStateChangeEvent;
@@ -78,10 +53,13 @@
import com.vaadin.flow.router.internal.AfterNavigationHandler;
import com.vaadin.flow.router.internal.BeforeEnterHandler;
import com.vaadin.flow.router.internal.BeforeLeaveHandler;
import com.vaadin.flow.server.Command;
import com.vaadin.flow.server.InvalidRouteConfigurationException;
import com.vaadin.flow.server.MockVaadinContext;
import com.vaadin.flow.server.MockVaadinServletService;
import com.vaadin.flow.server.MockVaadinSession;
import com.vaadin.flow.server.ServiceException;
import com.vaadin.flow.server.VaadinCommandInterceptor;
import com.vaadin.flow.server.VaadinContext;
import com.vaadin.flow.server.VaadinRequest;
import com.vaadin.flow.server.VaadinResponse;
@@ -91,6 +69,31 @@
import com.vaadin.flow.shared.Registration;
import com.vaadin.tests.util.AlwaysLockedVaadinSession;
import com.vaadin.tests.util.MockUI;
import org.hamcrest.CoreMatchers;
import org.hamcrest.MatcherAssert;
import org.junit.After;
import org.junit.Assert;
import org.junit.Ignore;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
import org.mockito.ArgumentMatchers;
import org.mockito.Mockito;
import org.slf4j.Logger;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;

public class UITest {

@@ -173,6 +176,30 @@ public void visit(StateNode node, NodeVisitor visitor) {
}
}

static class MyInterceptor implements VaadinCommandInterceptor {

Map<Object, Object> map;

@Override
public void commandExecutionStart(Map<Object, Object> context,
Command command) {
map = context;
context.put("observation.started", true);
}

@Override
public void handleException(Map<Object, Object> context,
Command command, Exception t) {
context.put("observation.error", t);
}

@Override
public void commandExecutionEnd(Map<Object, Object> context,
Command command) {
context.put("observation.end", true);
}
}

@After
public void tearDown() {
CurrentInstance.clearAll();
@@ -209,7 +236,13 @@ private static MockUI createAccessableTestUI() {
}

private static void initUI(UI ui, String initialLocation,
ArgumentCaptor<Integer> statusCodeCaptor)
ArgumentCaptor<Integer> statusCodeCaptor) {
initUI(ui, initialLocation, statusCodeCaptor, null);
}

private static void initUI(UI ui, String initialLocation,
ArgumentCaptor<Integer> statusCodeCaptor,
MyInterceptor myInterceptor)
throws InvalidRouteConfigurationException {
VaadinServletRequest request = Mockito.mock(VaadinServletRequest.class);
VaadinResponse response = Mockito.mock(VaadinResponse.class);
@@ -228,6 +261,15 @@ private static void initUI(UI ui, String initialLocation,
public VaadinContext getContext() {
return new MockVaadinContext();
}

@Override
protected List<VaadinCommandInterceptor> createVaadinCommandInterceptor()
throws ServiceException {
if (myInterceptor != null) {
return Collections.singletonList(myInterceptor);
}
return super.createVaadinCommandInterceptor();
}
};
service.setCurrentInstances(request, response);

@@ -608,6 +650,30 @@ public void unsetSession_accessErrorHandlerStillWorks() throws IOException {
logOutputNoDebug.contains("UIDetachedException"));
}

@Test
public void unsetSession_commandInterceptorGetsExecuted()
throws IOException {
MyInterceptor myInterceptor = new MyInterceptor();
UI ui = createTestUI();
initUI(ui, "", null, myInterceptor);

ui.getSession().access(() -> ui.getInternals().setSession(null));
ui.access(() -> {
Assert.fail("We should never get here because the UI is detached");
});

// Unlock to run pending access tasks
ui.getSession().unlock();

Map<Object, Object> map = myInterceptor.map;
Assert.assertTrue("Listener must be called on command start",
map.containsKey("observation.started"));
Assert.assertTrue("Listener must be called on command error",
map.containsKey("observation.error"));
Assert.assertTrue("Listener must be called on command end",
map.containsKey("observation.end"));
}

@Test
public void beforeClientResponse_regularOrder() {
UI ui = createTestUI();
Original file line number Diff line number Diff line change
@@ -129,4 +129,4 @@ public ServerEndpointExporter websocketEndpointDeployer() {
return new VaadinWebsocketEndpointExporter();
}

}
}
Original file line number Diff line number Diff line change
@@ -15,15 +15,18 @@
*/
package com.vaadin.flow.spring.service;

import com.vaadin.flow.server.Command;
import com.vaadin.flow.server.VaadinRequest;
import com.vaadin.flow.server.VaadinRequestInterceptor;
import com.vaadin.flow.server.VaadinResponse;
import com.vaadin.flow.server.VaadinSession;
import org.springframework.beans.factory.ObjectProvider;
import org.springframework.boot.SpringBootConfiguration;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.ComponentScan;
import org.springframework.context.annotation.Configuration;

import java.util.Map;

@Configuration
@ComponentScan
@SpringBootConfiguration
@@ -35,6 +38,12 @@ static class TestConfig {
MyRequestInterceptor myFilter() {
return new MyRequestInterceptor();
}

@Bean
MyVaadinComandInterceptor myVaadinComandInterceptor() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a counterpart in test, where this interceptor is asserted to be invoked.

return new MyVaadinComandInterceptor();
}

}

static class MyRequestInterceptor implements VaadinRequestInterceptor {
@@ -58,4 +67,25 @@ public void requestEnd(VaadinRequest request, VaadinResponse response,
request.setAttribute("stopped", "true");
}
}

static class MyVaadinComandInterceptor implements VaadinCommandInterceptor {

@Override
public void commandExecutionStart(Map<Object, Object> context,
Command command) {

}

@Override
public void handleException(Map<Object, Object> context,
Command command, Exception t) {

}

@Override
public void commandExecutionEnd(Map<Object, Object> context,
Command command) {

}
}
}