feat: use superclass of VadinServlet/Request if possible #9699

stbischof · 2020-12-20T21:28:02Z

I cases you are not using the VaadinServletService because you ship your own own (Servlet-based) VaadinService.

Then you are, at the moment, not able to use fundamental classes like VaadinServlet. This is because they use your VaadinServletService. But you are not using any methods of VaadinServletService because VaadinService is enough. So I suggest to use superclasses if possible.

VaadinServletRequestneeds a VaadinServletService

    @Override
    public VaadinServletService getService() {
        return vaadinService;
    }

If you have your own VaadinService implementation you can't rely on VaadinServletRequest
ant an the moment you cant use a lot of classes like VaadinServlet, PushRequestHandler, StreamResourceHandler, WebComponentBootstrapHandler

vaadin-bot · 2020-12-21T02:22:27Z

flow-server/src/main/java/com/vaadin/flow/server/communication/StreamResourceHandler.java

@@ -16,6 +16,8 @@
 package com.vaadin.flow.server.communication;

 import javax.servlet.ServletContext;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletRequestWrapper;


Remove this unused import 'javax.servlet.ServletRequestWrapper'.

pleku

Thanks for the contribution and sorry for the long delay with the review, but this PR basically shows how reluctant people are to comment on PRs that don't explain why changes are proposed. There is no issue open that would explain why this is needed so you need to start with that.

Secondly many of the changes seem like invalid and, sorry for being direct but, a waste of time. Please help me understand why this would be valuable. If not, I think we'll just have to say no thanks this time - I hope you understand.

flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java

flow-server/src/main/java/com/vaadin/flow/server/VaadinServletRequest.java

flow-server/src/main/java/com/vaadin/flow/server/VaadinServletService.java

flow-server/src/main/java/com/vaadin/flow/server/communication/PushRequestHandler.java

flow-server/src/main/java/com/vaadin/flow/server/communication/StreamResourceHandler.java

...-server/src/main/java/com/vaadin/flow/server/communication/WebComponentBootstrapHandler.java

pleku

If we we're to allow having your own VaadinService then it would be good if there was some sort of test in the project that would make it so that this feature is not accidentally broken.

Now that the reasoning behind the PR is clear, the next question would be about the added value and use case - why do you want to use your own VaadinService ?

(edit: this type of discussions would be better to have in an issue before starting the implementation)

flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java

stbischof · 2021-02-09T12:45:36Z

Because of your OSGi approach is not the way how I would expect to handle OSGi and Vaadin. It is kind of "Works with OSGi" and will fit most of people. But really working with Services is in a deep way would look other.

And your OSGi bundles also have the wrong license.

If you allow others to having an own VaadinService i will add test that force and check ClassCastExceptions.

pleku · 2021-02-10T07:01:42Z

Because of your OSGi approach is not the way how I would expect to handle OSGi and Vaadin. It is kind of "Works with OSGi" and will fit most of people. But really working with Services is in a deep way would look other.

I take it you mean with this that there would be a better way for this to work by utilizing declarative services in OSGi. I think this is a direction that we would want to take too and would gladly accept those contributions, but this direction should be taken by first discussing and agreeing on the steps in issues before jumping into implementation - it will be more smooth that way as then we know what is incoming and are more likely to help you get it in instead of need to start by questioning that "why is this code needed". From our perspective, we have to start that way as we're promising to maintain the code for 15 years to users who need that.

And your OSGi bundles also have the wrong license.

Just to be sure - do you mean the commercial license in vaadin/osgi or something else ?

If you allow others to having an own VaadinService i will add test that force and check ClassCastExceptions.

I don't have any objections to this now that I understand the goal. Though, I would rather work together to improve our OSGi support for all instead of enabling people to build their own support. We're gladly willing to give out licenses to those who contribute to it.

stbischof · 2021-02-10T07:37:35Z

I broken all my contribution in the moment where i had seen that you moved the osgi part to commercial license in vaadin/osgi.
Also parts that had been under the normal license.

Because this was just an private interest a commercial license would not be an option for me and some guyes with the same interests. I will discuss this with them.

pleku · 2021-02-17T12:26:44Z

To clarify to myself for later times, this is still waiting for a test to be added that would prevent breaking the "feature" that easily.

flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java

stbischof · 2021-02-18T11:49:08Z

i added the wished tests: OtherImplementationsTest.java

pleku · 2021-02-18T14:43:21Z

The build failed due to unrelated build agent issues, I'll retrigger it tomorrow and take a look at the test.

stbischof · 2021-02-24T18:00:29Z

@pleku Could you please retrigger it?

flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java

stbischof · 2021-02-25T08:03:44Z

forced push to trigger rerun of tests, (last one also did not finished

flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java

Signed-off-by: Stefan Bischof <stbischof@bipolis.org>

pleku

Sorry for the long delay, I was on winter vacation last week ⛷️

Only couple of small comments and then this would be 🟢

flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java

pleku · 2021-03-02T08:06:34Z

flow-server/src/test/java/com/vaadin/flow/server/OtherImplementationsTest.java

+
+public class OtherImplementationsTest {
+    @Test
+    public void StaticFileServer_Constructor_uses_VadinService()


non-blocking nit: typoed Vaadin here and c/p the error to other method names too ...

changed into Vaadin

pleku · 2021-03-02T08:07:58Z

flow-server/src/test/java/com/vaadin/flow/server/OtherImplementationsTest.java

+
+import com.vaadin.flow.server.communication.PushRequestHandler;
+
+public class OtherImplementationsTest {


I get the test, but at a glance probably most developers will not. Please change the name to CustomVaadinServiceImplementationTest and maybe add a comment line "Makes sure that a custom vaadin service that is not vaadin servlet service can be used in when desired".

Signed-off-by: Stefan Bischof <stbischof@bipolis.org>

vaadin-bot · 2021-03-04T08:53:10Z

SonarQube analysis reported 7 issues

2 critical
1 major
4 minor

Watch the comments in this conversation to review them.

5 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

VaadinService.java#L426: Rename "instantiator" which hides the field declared at line 193.
VaadinService.java#L429: Remove this use of "init"; it is deprecated.
VaadinService.java#L443: Remove this use of "init"; it is deprecated.
VaadinServlet.java#L194: Remove the declaration of thrown exception 'javax.servlet.ServletException', as it cannot be thrown from method's body.
VaadinServlet.java#L543: Replace this lambda with a method reference.

Legioth · 2021-04-14T07:56:58Z

This change causes massive backwards binary compatibility issues with e.g. add-ons compiled against older versions of Vaadin. This happens because method invocations in bytecode are tied to the method return type and not only the method name and parameter types.

I would suggest reverting to avoid those issues.

Furthermore, I don't even understand the whole point of making this change. If you want to use a custom service implementation together with VaadinServlet, then why wouldn't you extend VaadinServletService instead of the generic VaadinService class?

Legioth · 2021-04-15T15:51:06Z

To further elaborate on my previous message, the point is that VaadinServlet and VaadinServletService should really be seen as a single unit that just happens to be split up into two separate classes because Java doesn't offer multiple inheritance.

They're not supposed to be separated from each other. A proper separation would require more thorough architectural considerations rather than only tweaking some return types that happened to get in the way when trying to do something that you're not supposed to even try.

pleku · 2021-04-26T14:45:28Z

I don't know why the bot has added the label Released with Platform 19.0.0 as it is not true - @ZheSun88 ?

…ssible (#9699)" This reverts commit c581668, except intentionally leaves out part in PushHandler which accepts VaadinService and the protected API introduced to StaticFileServer to get a StaticFileServer resource, so it may be overridden to use something else than VaadinServletService.

ZheSun88 · 2021-04-26T15:31:05Z

Thanks@pleku , there was a wrong tag in platform repo, so the bot caught the wrong flow release for V19.0.0. i have fixed the issue.

…)" (#10812) This reverts commit c581668, except intentionally leaves out part of the protected API introduced to StaticFileServer to get a StaticFileServer resource, so it may be overridden to use something else than VaadinServletService.

vaadin-bot added the +1.0.0 label Dec 20, 2020

stbischof force-pushed the ServletRequestWrapper branch 5 times, most recently from 8754129 to 4803667 Compare December 21, 2020 02:10

vaadin-bot reviewed Dec 21, 2020

View reviewed changes

stbischof force-pushed the ServletRequestWrapper branch from 4803667 to ae688b9 Compare December 21, 2020 12:12

pleku suggested changes Feb 4, 2021

View reviewed changes

pleku reviewed Feb 9, 2021

View reviewed changes

flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java Show resolved Hide resolved

pleku changed the title ~~use superclass of VadinServlet/Request if possible~~ feat: use superclass of VadinServlet/Request if possible Feb 17, 2021

pleku added the waiting for author label Feb 17, 2021

stbischof force-pushed the ServletRequestWrapper branch 2 times, most recently from 6246f58 to c4070f7 Compare February 18, 2021 10:57

knoobie reviewed Feb 18, 2021

View reviewed changes

flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java Show resolved Hide resolved

knoobie reviewed Feb 24, 2021

View reviewed changes

flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java Outdated Show resolved Hide resolved

stbischof force-pushed the ServletRequestWrapper branch from 5d8a31a to 4bf8142 Compare February 25, 2021 08:01

vaadin-bot reviewed Feb 25, 2021

View reviewed changes

flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java Show resolved Hide resolved

use superclass of VadinServlet/Request if possible

152aace

Signed-off-by: Stefan Bischof <stbischof@bipolis.org>

stbischof force-pushed the ServletRequestWrapper branch from 4bf8142 to 152aace Compare February 25, 2021 09:47

pleku suggested changes Mar 2, 2021

View reviewed changes

Review fixes

9f7a35c

Signed-off-by: Stefan Bischof <stbischof@bipolis.org>

pleku approved these changes Mar 4, 2021

View reviewed changes

pleku enabled auto-merge (squash) March 4, 2021 08:37

pleku merged commit c581668 into vaadin:master Mar 4, 2021

vaadin-bot added the Released with Platform 19.0.0 label Mar 16, 2021

pleku removed the Released with Platform 19.0.0 label Apr 26, 2021

vaadin-bot added the Released with Platform 19.0.0 label Apr 26, 2021

ZheSun88 removed the Released with Platform 19.0.0 label Apr 26, 2021

vaadin deleted a comment from vaadin-bot Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use superclass of VadinServlet/Request if possible #9699

feat: use superclass of VadinServlet/Request if possible #9699

stbischof commented Dec 20, 2020 •

edited by pleku

vaadin-bot Dec 21, 2020

pleku left a comment

pleku left a comment •

edited

stbischof commented Feb 9, 2021 •

edited

pleku commented Feb 10, 2021

stbischof commented Feb 10, 2021 •

edited

pleku commented Feb 17, 2021

stbischof commented Feb 18, 2021

pleku commented Feb 18, 2021

stbischof commented Feb 24, 2021 •

edited

stbischof commented Feb 25, 2021

pleku left a comment

pleku Mar 2, 2021

stbischof Mar 2, 2021

pleku Mar 2, 2021

stbischof Mar 2, 2021

vaadin-bot commented Mar 4, 2021

Legioth commented Apr 14, 2021

Legioth commented Apr 15, 2021

pleku commented Apr 26, 2021

ZheSun88 commented Apr 26, 2021 •

edited


		import com.vaadin.flow.server.communication.PushRequestHandler;

		public class OtherImplementationsTest {

feat: use superclass of VadinServlet/Request if possible #9699

feat: use superclass of VadinServlet/Request if possible #9699

Conversation

stbischof commented Dec 20, 2020 • edited by pleku

vaadin-bot Dec 21, 2020

Choose a reason for hiding this comment

pleku left a comment

Choose a reason for hiding this comment

pleku left a comment • edited

Choose a reason for hiding this comment

stbischof commented Feb 9, 2021 • edited

pleku commented Feb 10, 2021

stbischof commented Feb 10, 2021 • edited

pleku commented Feb 17, 2021

stbischof commented Feb 18, 2021

pleku commented Feb 18, 2021

stbischof commented Feb 24, 2021 • edited

stbischof commented Feb 25, 2021

pleku left a comment

Choose a reason for hiding this comment

pleku Mar 2, 2021

Choose a reason for hiding this comment

stbischof Mar 2, 2021

Choose a reason for hiding this comment

pleku Mar 2, 2021

Choose a reason for hiding this comment

stbischof Mar 2, 2021

Choose a reason for hiding this comment

vaadin-bot commented Mar 4, 2021

5 extra issues

Legioth commented Apr 14, 2021

Legioth commented Apr 15, 2021

pleku commented Apr 26, 2021

ZheSun88 commented Apr 26, 2021 • edited

stbischof commented Dec 20, 2020 •

edited by pleku

pleku left a comment •

edited

stbischof commented Feb 9, 2021 •

edited

stbischof commented Feb 10, 2021 •

edited

stbischof commented Feb 24, 2021 •

edited

ZheSun88 commented Apr 26, 2021 •

edited