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
Expand Up @@ -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
Expand All @@ -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.
Expand All @@ -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
Expand All @@ -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;
}

/**
Expand All @@ -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;

Expand All @@ -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));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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();
Expand Down
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
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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);

Expand All @@ -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()));
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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);
Expand Down
Loading
Loading