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.

Sign up for GitHub

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

Jump to bottom

Allow simpler extension of the SpringServlet / structuring of SpringBootAutoConfiguration #19994

Closed
pgould-taskize opened this issue Sep 19, 2024 · 10 comments · Fixed by #20276
Closed

Allow simpler extension of the SpringServlet / structuring of SpringBootAutoConfiguration #19994

pgould-taskize opened this issue Sep 19, 2024 · 10 comments · Fixed by #20276
Assignees
@tepi
Labels
enhancement prerelease version for Vaadin 24.6.0 Released with Vaadin 24.4.15 Released with Vaadin 24.5.1 Released with Vaadin 24.6.0.alpha2 spring waiting for author

Comments

@pgould-taskize
Copy link

Describe your motivation

We have certain ThreadLocal values that need to be set before access tasks are run.

The com.vaadin.flow.server.VaadinService provides a hook to allow overriding the accessSession, and in order to do this, one needs to extend the VaadinServlet class and override the createServletService(), which used to be advertised as a
https://vaadin.com/docs/v14/flow/advanced/tutorial-application-lifecycle.

    /**
     * Implementation for {@link VaadinSession#access(Command)}. This method is
     * implemented here instead of in {@link VaadinSession} to enable overriding
     * the implementation without using a custom subclass of VaadinSession.
     *
     * @param session
     *            the vaadin session to access
     * @param command
     *            the command to run with the session locked
     * @return a future that can be used to check for task completion and to
     *         cancel the task
     * @see VaadinSession#access(Command)
     */
    public Future<Void> accessSession(VaadinSession session, Command command) 
        ...
    }

This documentation has been removed in the V24 documentation, yet no other strategy for achieving the same process has been given, although it seems that this is still possible but requires requires cut pasting of thecom.vaadin.flow.spring.SpringBootAutoConfiguration#servletRegistrationBean.

Describe the solution you'd like

Could the auto-configuration be modified in such a way that cut and pasting the entire servletRegistrationBean is not necessary, for example, the only piece of code that I'd like to have to supply is the springServletBean definition in the following configuration:

@Configuration
public class OverriddenServletConfiguration {

    @Autowired
    private WebApplicationContext context;

    @Bean
    public SpringServlet springServletBean(VaadinConfigurationProperties configurationProperties) {
        String mapping = configurationProperties.getUrlMapping();
        boolean rootMapping = RootMappedCondition.isRootMapping(mapping);
        return new OverriddenSpringServlet(context, rootMapping);
    }

    @Bean
    public ServletRegistrationBean<SpringServlet> servletRegistrationBean(
            ObjectProvider<MultipartConfigElement> multipartConfig,
            VaadinConfigurationProperties configurationProperties,
            SpringServlet springServlet) {
        String mapping = configurationProperties.getUrlMapping();
        Map<String, String> initParameters = new HashMap<>();
        boolean rootMapping = RootMappedCondition.isRootMapping(mapping);

        if (rootMapping) {
            mapping = VaadinServletConfiguration.VAADIN_SERVLET_MAPPING;
            initParameters.put(
                    VaadinServlet.INTERNAL_VAADIN_SERVLET_VITE_DEV_MODE_FRONTEND_PATH,
                    "");
        }

        String pushUrl = rootMapping ? "" : mapping.replace("/*", "");
        pushUrl  = "/"   Constants.PUSH_MAPPING;

        initParameters.put(ApplicationConfig.JSR356_MAPPING_PATH, pushUrl);

        ServletRegistrationBean<SpringServlet> registration = new ServletRegistrationBean<>(springServlet, mapping);
        registration.setInitParameters(initParameters);
        registration
                .setAsyncSupported(configurationProperties.isAsyncSupported());
        registration.setName(
                ClassUtils.getShortNameAsProperty(SpringServlet.class));
        // Setup multi part form processing for non root servlet mapping to be
        // able to process Hilla login out of the box
        if (!rootMapping) {
            multipartConfig.ifAvailable(registration::setMultipartConfig);
        }
        registration.setLoadOnStartup(
                configurationProperties.isLoadOnStartup() ? 1 : -1);
        return registration;
    }

}

Describe alternatives you've considered

Any other hooks to execute code prior to access tasks being run and cleaned up afterwards would be perfectly acceptable.

Additional context

Is there anything else you can add about the proposal?
@tepi tepi self-assigned this Oct 1, 2024
@TatuLund
Copy link
Contributor

TatuLund commented Oct 2, 2024

A simpler approach to this would be to implement AbstractView which will be extended by Route classes and implement access delegate method there which also takes care of handling thread locals before and after ui.access(..).

public abstract class AbstractView extends Div {

    private UI ui;
    Future<Void> future;

    protected void onAttach(AttachEvent attachEvent) {
        super.onAttach(attachEvent);
        ui = attachEvent.getUI();
    }

    protected Future<Void> uiAccess(Command command) {
        if (ui == null) {
            throw new IllegalStateException("Component not attached");
        }
        // Set thread locals
        try {
            future = ui.access(command);
            return future;
        } finally {
            // Remove thread locals
        }
    }

    protected void onDetach(DetachEvent detachEvent) {
        super.onDetach(detachEvent);
        future.cancel(true);
        ui = null;
    }
}

@pgould-taskize
Copy link
Author

We can't do something like you suggested above, as the access call is coming from a flush requested by the DataCommunicator.

The data that we need available at the point that access is called is a cross cutting concern which our DataProviders should have no knowledge of. Whilst we could start doing things along the lines of having a custom CallbackDataProvider which all our other data providers extend to set this context, who is to say that there aren't other parts of the platform that will do similar if we use them.

However, the hook to do what we want to do is there, and documented in the Javadocs - so I don't understand the reluctance to make the facility easier to use. If we're not expected to use it, could I suggest that it be deprecated or the documentation updated to reflect this?

com.taskize.repository.tenant.rules.TenantBusinessRulesImpl.getUserOrganisations(TenantBusinessRulesImpl.java:474)
  at com.taskize.ui.bubble.bubblelist.BubbleListService.getParticipantQuerySpecs(BubbleListService.java:59)
  at com.taskize.ui.bubble.bubblelist.BubbleListService.countForPrincipalAndFilter(BubbleListService.java:46)
  at com.taskize.ui.bubble.bubblelist.BubbleListDataProviderCallbacks.count(BubbleListDataProviderCallbacks.java:52)
  at com.vaadin.flow.data.provider.CallbackDataProvider.sizeInBackEnd(CallbackDataProvider.java:142)
  at com.vaadin.flow.data.provider.AbstractBackEndDataProvider.size(AbstractBackEndDataProvider.java:66)
  at com.vaadin.flow.data.provider.DataCommunicator.getDataProviderSize(DataCommunicator.java:940)
  at com.vaadin.flow.data.provider.DataCommunicator.flush(DataCommunicator.java:1193)
  at com.vaadin.flow.data.provider.DataCommunicator.lambda$requestFlush$7258256f$1(DataCommunicator.java:1138)
  at com.vaadin.flow.internal.StateTree.lambda$runExecutionsBeforeClientResponse$2(StateTree.java:397)
  at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
  at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
  at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)
  at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
  at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
  at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
  at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
  at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
  at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
  at com.vaadin.flow.internal.StateTree.runExecutionsBeforeClientResponse(StateTree.java:392)
  at com.vaadin.flow.server.communication.UidlWriter.encodeChanges(UidlWriter.java:394)
  at com.vaadin.flow.server.communication.UidlWriter.createUidl(UidlWriter.java:170)
  at com.vaadin.flow.server.communication.UidlWriter.createUidl(UidlWriter.java:215)
  at com.vaadin.flow.server.communication.AtmospherePushConnection.push(AtmospherePushConnection.java:207)
  ... 15 common frames omitted

@jameskerrtaskize
Copy link

Hi, is there any update on this? Thanks

@tepi
Copy link
Contributor

tepi commented Oct 11, 2024

Hi! Sorry for slow response, we have been quite busy with 24.5 beta/RC phase. Will resume on this first thing Monday.

@tepi tepi mentioned this issue Oct 17, 2024
Merged
@tepi
Copy link
Contributor

tepi commented Oct 17, 2024

This proved to be difficult to implement due to Spring issues and also not able to break backwards compatibility. I added a static method for default configuration of the SpringServlet (or whatever is extending SpringServlet) here: #20276

This means that with this PR, providing a custom servlet with default configuration would look for example something like this:

package com.example.application;

import jakarta.servlet.MultipartConfigElement;

import org.springframework.beans.factory.ObjectProvider;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.web.servlet.ServletRegistrationBean;
import org.springframework.context.ApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.web.context.WebApplicationContext;

import com.vaadin.flow.function.DeploymentConfiguration;
import com.vaadin.flow.server.ServiceException;
import com.vaadin.flow.server.VaadinServletService;
import com.vaadin.flow.spring.RootMappedCondition;
import com.vaadin.flow.spring.SpringServlet;
import com.vaadin.flow.spring.VaadinConfigurationProperties;

import static com.vaadin.flow.spring.SpringBootAutoConfiguration.configureServletRegistrationBean;

@Configuration
public class OverriddenServletConfiguration {

    @Autowired
    private WebApplicationContext context;

    @Bean
    public ServletRegistrationBean<SpringServlet> servletRegistrationBean(
            ObjectProvider<MultipartConfigElement> multipartConfig,
            VaadinConfigurationProperties configurationProperties) {
        boolean rootMapping = RootMappedCondition
                .isRootMapping(configurationProperties.getUrlMapping());
        return configureServletRegistrationBean(multipartConfig,
                configurationProperties,
                new OverriddenSpringServlet(context, rootMapping));
    }

    public static class OverriddenSpringServlet extends SpringServlet {

        public OverriddenSpringServlet(ApplicationContext context,
                                       boolean rootMapping) {
            super(context, rootMapping);
            // TODO do custom stuff here
        }

        @Override
        protected VaadinServletService createServletService(
                DeploymentConfiguration deploymentConfiguration)
                throws ServiceException {
            // TODO do custom stuff here
            return super.createServletService(deploymentConfiguration);
        }
    }
}

Please let me know if this solution is usable enough for you. If not, we'll keep exploring possible other options.

@tepi
Copy link
Contributor

tepi commented Oct 21, 2024

@pgould-taskize @jameskerrtaskize If you have any thoughts on the PR and example above, please comment here. Thank you!

@pgould-taskize
Copy link
Author

pgould-taskize commented Oct 22, 2024 •
edited
Loading

That looks good to me, means that we don't have to essentially copy boiler plate code into our configuration. Thanks!

@tepi tepi closed this as completed in b026b9d Oct 22, 2024
vaadin-bot pushed a commit that referenced this issue Oct 22, 2024
@tepi @vaadin-bot
feat: enable easier override for SpringServlet (#20276)
34a990f 
Fixes #19994
vaadin-bot pushed a commit that referenced this issue Oct 22, 2024
@tepi @vaadin-bot
feat: enable easier override for SpringServlet (#20276)
ae54c70 
Fixes #19994
vaadin-bot added a commit that referenced this issue Oct 22, 2024
@vaadin-bot @tepi
feat: enable easier override for SpringServlet (#20276) (#20297)
b6ea504 
Fixes #19994

Co-authored-by: Teppo Kurki <[email protected]>
vaadin-bot added a commit that referenced this issue Oct 22, 2024
@vaadin-bot @tepi
feat: enable easier override for SpringServlet (#20276) (#20298)
a840fcc 
Fixes #19994

Co-authored-by: Teppo Kurki <[email protected]>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.5.1.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.6.0.alpha2 and is also targeting the upcoming stable 24.6.0 version.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.4.15.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tepi tepi

Labels
enhancement prerelease version for Vaadin 24.6.0 Released with Vaadin 24.4.15 Released with Vaadin 24.5.1 Released with Vaadin 24.6.0.alpha2 spring waiting for author
Projects
Vaadin Flow enhancements backlog (Vaadin 10 )
Status: Done / Pending Release
Vaadin Flow ongoing work (Vaadin 10 )
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: enable easier override for SpringServlet vaadin/flow
6 participants
@tepi @TatuLund @vaadin-bot @mshabarov @jameskerrtaskize @pgould-taskize