feat: Bring back vaadin servlet mapping behind feature flag #14694

MarcinVaadin · 2022-09-29T12:06:00Z

Description

As there is a need to support custom servlet mapping after it has been removed (#14602) - it has been bring back behind feature flag.

Fixes #14641

Type of change

Bugfix
Feature

Checklist

I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
I have added a description following the guideline.
The issue is created in the corresponding repository and I have referenced it.
I have added tests to ensure my change is effective and works as intended.
New and existing tests are passing locally with my change.
I have performed self-review and corrected misspellings.

Additional for `Feature` type of change

Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

* fix: Only map websockets to servlet path and not any sub paths (cherry picked from commit f4c7497)

…ind-feature-flag

github-actions · 2022-09-29T12:33:39Z

Unit Test Results

  978 files +23   978 suites +23 50m 25s ⏱️ - 3m 31s
6 082 tests ±  0 6 028 ✔️ ±  0 54 💤 ±0 0 ❌ ±0
6 378 runs +41 6 315 ✔️ +40 63 💤 +1 0 ❌ ±0

Results for commit 2c0ab6a. ± Comparison against base commit dc00ef1.

♻️ This comment has been updated with latest results.

flow-server/src/main/java/com/vaadin/experimental/FeatureFlags.java

...spring/src/main/java/com/vaadin/flow/spring/security/VaadinWebSecurityConfigurerAdapter.java

Co-authored-by: Artur <artur@vaadin.com>

vaadin-spring/src/main/java/com/vaadin/flow/spring/FeatureFlagsUtil.java

…sUtil.java Co-authored-by: sonatype-lift[bot] <37194012+sonatype-lift[bot]@users.noreply.github.com>

…ervlet-behind-feature-flag' into feature/14641-bring-back-vaadinServlet-behind-feature-flag

For sake of Quarcus and other non-spring applications JSR356_PATH_MAPPING_LENGTH has been moved to general Atmosphere init parameters in PushRequestHandler Co-authored-by: Marco Collovati <marco@vaadin.com>

…-back-vaadinServlet-behind-feature-flag

…erty

sonarcloud · 2022-10-06T12:01:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

mcollovati · 2022-10-06T13:55:06Z

vaadin-spring/src/main/java/com/vaadin/flow/spring/VaadinServletConfiguration.java

@@ -58,6 +58,9 @@
 public class VaadinServletConfiguration {

    static final String VAADIN_SERVLET_MAPPING = "/vaadinServlet/*";


Can VAADIN_SERVLET_MAPPING be removed?

mcollovati · 2022-10-06T14:00:29Z

vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringServlet.java

+        if (rootMapping
+                && isPushServletMappingEnabled(context.getEnvironment())) {
+            // in the case of root mapping, push requests should go to
+            // /vaadinServlet/pushUrl


Suggested change

// /vaadinServlet/pushUrl

// /VAADIN/push/pushUrl

mcollovati · 2022-10-06T14:00:43Z

vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringServlet.java

+                    .replace("/*", "");
+            customPushUrl = customPushUrl.replaceFirst("context://", "/");
+            customPushUrl = customPushUrl.replaceFirst(Pattern.quote(mapping),
+                    ""); // if workaround "/vaadinServlet/myCustomUrl" used


Suggested change

""); // if workaround "/vaadinServlet/myCustomUrl" used

""); // if workaround "/VAADIN/push/myCustomUrl" used

mshabarov · 2022-10-07T06:00:32Z

.../vaadin-spring-tests/test-spring-boot-push-mapping/src/main/resources/application.properties

@@ -0,0 +1,4 @@
+server.port=8888
+vaadin.exclude-urls=/swagger-ui/**
+vaadin.pushServletMapping=true


I'd propose "nonRootPushServletMapping" (or vice versa - "pushRootServletMapping", true by default) as a name for this parameter, because "pushServletMapping" doesn't tell much about its meaning. Also, not "pushServletMapping=false" doesn't make sense, since mapping is always there.

mshabarov · 2022-10-07T06:04:49Z

vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringBootAutoConfiguration.java

@@ -84,7 +84,7 @@ public ServletRegistrationBean<SpringServlet> servletRegistrationBean(
        String pushRegistrationPath;

        if (rootMapping) {
-            mapping = VaadinServletConfiguration.VAADIN_SERVLET_MAPPING;
+            mapping = VaadinServletConfiguration.VAADIN_SERVLET_PUSH_MAPPING;


mapping is used here not for push endpoint, but for SpringServlet registration, which is fine.
Apart from Push, it is fine to have /vaadinServlet mapped spring servlet.
We only talk about mapping path for JSR356_MAPPING_PATH, it should be changed, not the spring servlet.
Or I terribly missed something.

So in my opinion you should change pushRegistrationPath to include /VAADIN/push in case of root mapping, but not mapping itself.

mshabarov · 2022-10-07T06:26:10Z

vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringServlet.java

+    protected boolean isPushServletMappingEnabled(Environment environment) {
+        if (SpringUtil.isSpringBoot()) {
+            try {
+                return (boolean) Class.forName(


Why we have to do this? Isn't VaadinConfigurationProperties instance available in the context where the method isPushServletMappingEnabled is needed?

mshabarov · 2022-10-07T06:28:02Z

vaadin-spring/src/main/java/com/vaadin/flow/spring/VaadinMVCWebAppInitializer.java

+                        makeContextRelative(mapping.replace("*", "")));
+            }
+
+            mapping = VaadinServletConfiguration.VAADIN_SERVLET_PUSH_MAPPING;


mapping should be "/vaadinServlet", because it corresponds to the servlet, not to the push registration.
pushRegistrationPath should be modified.

mcollovati · 2022-10-07T06:31:44Z

vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringBootAutoConfiguration.java

+        if (configurationProperties.isPushServletMapping()) {
+            initParameters.put(ApplicationConfig.JSR356_MAPPING_PATH,
+                    mapping.replace("/*", ""));
+        } else {


Did I understand correctly that if I've set a custom vaadin url mapping (e.g. /ui/*), the mapping for PUSH will be the on /ui/* and not on /ui/VAADIN/push?
If so, the AJP reverse proxy configuration will still have the same problem of missing a dedicate path for websockets

Yes, it would be /ui/* and it was so in the past even with /vaadinServlet. So seems like if someone uses AJP with non root url mapping, this fix wouldn't work (as it wouldn't work even with old code with /vaadinServlet).

What if we add VAADIN/push always when this new parameter is turned on?

I agree, it seems more consistent to me

MarcinVaadin · 2022-10-07T09:29:27Z

Closing a PR due to approach change. New PR will be created.

Artur- and others added 2 commits September 28, 2022 13:22

fix: Only map websockets to servlet path and not any sub paths (#14638)

1f0c797

* fix: Only map websockets to servlet path and not any sub paths (cherry picked from commit f4c7497)

feat: Added vaadinServlet mapping behind FeatureFlag (#14641)

9b01ced

MarcinVaadin linked an issue Sep 29, 2022 that may be closed by this pull request

Websocket in combination with AJP broken behind reverse-proxy in 23.2. #14641

Closed

Merge branch 'master' into feature/14641-bring-back-vaadinServlet-beh…

8433057

…ind-feature-flag

vaadin-bot added +1.0.0 +0.1.0 and removed +1.0.0 labels Sep 29, 2022

Artur- requested changes Sep 30, 2022

View reviewed changes

MarcinVaadin and others added 2 commits September 30, 2022 12:45

Apply suggestions from code review

1fe0844

Co-authored-by: Artur <artur@vaadin.com>

feat: Added vaadinServlet mapping behind FeatureFlag (#14641)

0aec679

vaadin-bot added +1.0.0 and removed +0.1.0 labels Sep 30, 2022

formatter

cd5258b

sonatype-lift bot reviewed Sep 30, 2022

View reviewed changes

vaadin-spring/src/main/java/com/vaadin/flow/spring/FeatureFlagsUtil.java Outdated Show resolved Hide resolved

MarcinVaadin and others added 2 commits September 30, 2022 14:07

Update vaadin-spring/src/main/java/com/vaadin/flow/spring/FeatureFlag…

b0bc721

…sUtil.java Co-authored-by: sonatype-lift[bot] <37194012+sonatype-lift[bot]@users.noreply.github.com>

Merge remote-tracking branch 'origin/feature/14641-bring-back-vaadinS…

412b9e8

…ervlet-behind-feature-flag' into feature/14641-bring-back-vaadinServlet-behind-feature-flag

mshabarov self-requested a review October 3, 2022 11:05

MarcinVaadin and others added 3 commits October 3, 2022 15:51

chore: move atmosphere JSR356_PATH_MAPPING_LENGTH to PushRequestHandler

4176206

For sake of Quarcus and other non-spring applications JSR356_PATH_MAPPING_LENGTH has been moved to general Atmosphere init parameters in PushRequestHandler Co-authored-by: Marco Collovati <marco@vaadin.com>

Merge remote-tracking branch 'origin/master' into feature/14641-bring…

b644cdb

…-back-vaadinServlet-behind-feature-flag

feat: Set mapping to /VAADIN/push, enabled by setting Spring app prop…

2c0ab6a

…erty

vaadin-bot added +0.0.1 and removed +1.0.0 labels Oct 6, 2022

MarcinVaadin mentioned this pull request Oct 6, 2022

PoC for /VAADIN/push set by default (to be removed) #14750

Closed

mcollovati reviewed Oct 7, 2022

View reviewed changes

mshabarov requested changes Oct 7, 2022

View reviewed changes

mcollovati reviewed Oct 7, 2022

View reviewed changes

MarcinVaadin closed this Oct 7, 2022

MarcinVaadin deleted the feature/14641-bring-back-vaadinServlet-behind-feature-flag branch October 7, 2022 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Bring back vaadin servlet mapping behind feature flag #14694

feat: Bring back vaadin servlet mapping behind feature flag #14694

MarcinVaadin commented Sep 29, 2022

github-actions bot commented Sep 29, 2022 •

edited

Loading

sonarcloud bot commented Oct 6, 2022

mcollovati Oct 6, 2022

mcollovati Oct 6, 2022

mcollovati Oct 6, 2022

mshabarov Oct 7, 2022

mshabarov Oct 7, 2022

mshabarov Oct 7, 2022

mshabarov Oct 7, 2022

mshabarov Oct 7, 2022

mcollovati Oct 7, 2022

mshabarov Oct 7, 2022 •

edited

Loading

mshabarov Oct 7, 2022

mcollovati Oct 7, 2022

MarcinVaadin commented Oct 7, 2022

		@@ -58,6 +58,9 @@
		public class VaadinServletConfiguration {

		static final String VAADIN_SERVLET_MAPPING = "/vaadinServlet/*";

	""); // if workaround "/vaadinServlet/myCustomUrl" used
	""); // if workaround "/VAADIN/push/myCustomUrl" used

feat: Bring back vaadin servlet mapping behind feature flag #14694

feat: Bring back vaadin servlet mapping behind feature flag #14694

Conversation

MarcinVaadin commented Sep 29, 2022

Description

Type of change

Checklist

Additional for Feature type of change

github-actions bot commented Sep 29, 2022 • edited Loading

Unit Test Results

sonarcloud bot commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshabarov Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcinVaadin commented Oct 7, 2022

Additional for `Feature` type of change

github-actions bot commented Sep 29, 2022 •

edited

Loading

mshabarov Oct 7, 2022 •

edited

Loading