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

Conversation

marcingrzejszczak
Copy link
Contributor

No description provided.

VaadinFilter simulates an around aspect around processing of a request

related to vaadingh-17436
* renamed VaadinFilter to VaadinRequestInterceptor
* Vaadin uses Lookup to init the VaadinService
** Added VaadinRequestInterceptors to ServiceInitEvent
** Added a VaadinRequestInterceptorServiceInitListener that will add interceptors to the init event
** The listener works with ServiceLocator based DI and Spring based DI (has both arg and non-arg constructor)
** That way we have 1 way of initializing the interceptors
* Added the VaadinRequestInterceptorServiceInitListener to META-INF for automated locator discovery
* Added the VaadinRequestInterceptorServiceInitListener as a bean for Spring Boot auto configuration
* Extended the Instantiator mechanism to find all instances of a given class
** Implemented the method in Spring and non-Spring implementations
Co-authored-by: Mikhail Shabarov <[email protected]>
@marcingrzejszczak marcingrzejszczak marked this pull request as draft September 28, 2023 13:46
@mshabarov mshabarov added the Contribution PRs coming from the community or external to the team label Sep 28, 2023
@mshabarov mshabarov self-requested a review September 29, 2023 11:20
@mshabarov mshabarov changed the title Vaadin command interceptor feat: Vaadin command interceptor Oct 2, 2023
@@ -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.

@@ -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.

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

* @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.

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)}.

@@ -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"

@@ -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.

@mshabarov mshabarov self-assigned this Oct 30, 2023
@mshabarov mshabarov removed their assignment Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution PRs coming from the community or external to the team
Projects
Status: 🪵Product backlog
Development

Successfully merging this pull request may close these issues.

2 participants