Make classes serializable #4148

elmot · 2018-05-21T14:33:51Z

Partial fix for vaadin/platform#173
Related to #4105

This change is

vaadin-bot · 2018-05-21T14:49:04Z

flow-server/src/main/java/com/vaadin/flow/component/HasValue.java


    /**
     * An event fired when the value of a {@code HasValue} changes.
     *
     * @param <V>
     *            the value type
     */
-    interface ValueChangeEvent<V> {
+    interface ValueChangeEvent<V> extends Serializable {
        HasValue<?, V> getHasValue();


Remove usage of generic wildcard type.

caalador · 2018-05-22T06:32:23Z

JavaDoc fails build.

denis-anisimov · 2018-05-22T06:34:36Z

Reviewed 47 of 54 files at r1, 9 of 10 files at r2.
Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

flow-server/src/main/java/com/vaadin/flow/dom/ElementFactory.java, line 28 at r2 (raw file):

ElementFactory

This is a mix-in without any single instance method.
Technically it's just a util class. So doesn't have to be serializable.
But it doesn't hurt.

flow-server/src/main/java/com/vaadin/flow/dom/NodeVisitor.java, line 28 at r2 (raw file):

 extends Serializable

That may be wrong......
I'm not sure.
Visitor is not intended to be stored in any state. It's temporary logic which is applied during request handling. So it's too much to require it to be serializable.

flow-server/src/main/java/com/vaadin/flow/dom/impl/ThemeListImpl.java, line 18 at r2 (raw file):

import java.io.Serializable;

No other changes in this class.
Is it a mistake or something is missed ?

flow-server/src/main/java/com/vaadin/flow/router/RouteNotFoundError.java, line 118 at r2 (raw file):

        private LazyInit() {
        }

Why ?
The class is inner private.
What's the benefit to have explicit CTOR here ?

flow-server/src/main/java/com/vaadin/flow/templatemodel/PropertyMapBuilder.java, line 131 at r2 (raw file):

SerializablePredicate<String>

This is not really needed.
This predicate is not stored anywhere. It's used only at the creation time to filter the data and then it's lost.
But it looks this filter is our own filter which we may force to be serializable.

flow-server/src/main/java/com/vaadin/flow/templatemodel/TemplateModel.java, line 188 at r2 (raw file):

SerializablePredicate<String>

I would say this is a mistake here to require Serializable.
Looks like this filter is used only to filter the data during other data creation and then it's not stored anywhere.
So it's too restrictive to require serializable here.

Comments from Reviewable

elmot · 2018-05-22T08:28:02Z

Review status: 47 of 53 files reviewed at latest revision, 19 unresolved discussions.

flow-server/src/main/java/com/vaadin/flow/dom/ElementFactory.java, line 28 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

ElementFactory 
This is a mix-in without any single instance method.
Technically it's just a util class. So doesn't have to be serializable.
But it doesn't hurt.

Done.

flow-server/src/main/java/com/vaadin/flow/dom/NodeVisitor.java, line 28 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

 extends Serializable 
That may be wrong......
I'm not sure.
Visitor is not intended to be stored in any state. It's temporary logic which is applied during request handling. So it's too much to require it to be serializable.

Done.

flow-server/src/main/java/com/vaadin/flow/router/RouteNotFoundError.java, line 118 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

        private LazyInit() {
        }
Why ?
The class is inner private.
What's the benefit to have explicit CTOR here ?

Technically there is no need of the CTOR, it's added to make code self-documented.

flow-server/src/main/java/com/vaadin/flow/templatemodel/TemplateModel.java, line 188 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

SerializablePredicate<String>
I would say this is a mistake here to require Serializable.
Looks like this filter is used only to filter the data during other data creation and then it's not stored anywhere.
So it's too restrictive to require serializable here.

Are PropertyFilters stored anywhere? If yes, then it should be made of SerializablePredicates and all classes around should be serializable as well. If not, then they could be reverted back to non-serializable version

Comments from Reviewable

denis-anisimov · 2018-05-22T08:36:10Z

Reviewed 3 of 9 files at r3.
Review status: 50 of 53 files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

flow-server/src/main/java/com/vaadin/flow/router/RouteNotFoundError.java, line 118 at r2 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

Technically there is no need of the CTOR, it's added to make code self-documented.

Don't see how it makes self-documented code for the class which is nested and not visible from anywhere. Extra CTOR just adds more lines and what's bad: this change has no any relation to the main purpose of the PR.

But it's minor.

flow-server/src/main/java/com/vaadin/flow/templatemodel/TemplateModel.java, line 188 at r2 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

Are PropertyFilters stored anywhere? If yes, then it should be made of SerializablePredicates and all classes around should be serializable as well. If not, then they could be reverted back to non-serializable version

That's the question.
The filter is stored inside an instance of PropertyFilter. But the PropertyFilter instance is not stored anywhere. It's temporary instance.
I didn't find any code which stores this instance.
And if PropertyFilter also requires serializable filter in its CTOR it also should be reverted.

Comments from Reviewable

denis-anisimov · 2018-05-22T08:39:15Z

Reviewed 6 of 9 files at r3.
Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

flow-server/src/test/java/com/vaadin/tests/server/ClassesSerializableTest.java, line 213 at r3 (raw file):

test

useless prefix. But it's minor.

Comments from Reviewable

elmot · 2018-05-22T10:23:42Z

Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

flow-server/src/main/java/com/vaadin/flow/templatemodel/TemplateModel.java, line 188 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

That's the question.
The filter is stored inside an instance of PropertyFilter. But the PropertyFilter instance is not stored anywhere. It's temporary instance.
I didn't find any code which stores this instance.
And if PropertyFilter also requires serializable filter in its CTOR it also should be reverted.

Ok, I am reverting both.

flow-server/src/main/java/com/vaadin/flow/component/internal/ComponentMetaData.java, line 58 at r1 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Make "javaScripts" transient or serializable.

Done.

flow-server/src/main/java/com/vaadin/flow/component/internal/ComponentMetaData.java, line 59 at r1 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Make "styleSheets" transient or serializable.

Done.

Comments from Reviewable

denis-anisimov · 2018-05-22T11:06:00Z

Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

flow-server/src/main/java/com/vaadin/flow/templatemodel/TemplateModel.java, line 188 at r2 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

Ok, I am reverting both.

No changes.
Not committed ?

Comments from Reviewable

denis-anisimov · 2018-05-23T05:40:15Z

Reviewed 4 of 7 files at r4.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks broke.

Comments from Reviewable

elmot · 2018-05-23T07:27:51Z

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks broke.

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/TemplateDataAnalyzer.java, line 107 at r1 (raw file):

        private final Map<String, String> tagById;
        private final Map<Field, String> idByField;

Done.

Comments from Reviewable

elmot · 2018-05-23T08:31:16Z

Review status: 42 of 48 files reviewed at latest revision, 5 unresolved discussions.

flow-server/src/main/java/com/vaadin/flow/dom/impl/ThemeListImpl.java, line 18 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

import java.io.Serializable;
No other changes in this class.
Is it a mistake or something is missed ?

Done. Now there are changes

flow-server/src/main/java/com/vaadin/flow/router/RouteNotFoundError.java, line 118 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

Don't see how it makes self-documented code for the class which is nested and not visible from anywhere. Extra CTOR just adds more lines and what's bad: this change has no any relation to the main purpose of the PR.

But it's minor.

Done.

flow-server/src/main/java/com/vaadin/flow/templatemodel/PropertyMapBuilder.java, line 131 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

SerializablePredicate<String>
This is not really needed.
This predicate is not stored anywhere. It's used only at the creation time to filter the data and then it's lost.
But it looks this filter is our own filter which we may force to be serializable.

Done.

Comments from Reviewable

vaadin-bot · 2018-05-23T09:18:09Z

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java

+        ArrayList<Field> nonSerializableFunctionFields = new ArrayList<>();
+
+        List<Class<?>> nonSerializableClasses = new ArrayList<>();
+        for (String className : classes) {


Reduce the total number of break and continue statements in this loop to use at most one.

denis-anisimov · 2018-05-23T09:59:20Z

Reviewed 1 of 8 files at r4, 1 of 3 files at r5, 7 of 10 files at r6.
Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks broke.

flow-test-generic/pom.xml, line 22 at r6 (raw file):

</project>

new line

Comments from Reviewable

elmot · 2018-05-23T10:14:43Z

Review status: 47 of 49 files reviewed at latest revision, 28 unresolved discussions.

flow-test-generic/pom.xml, line 22 at r6 (raw file):

Previously, denis-anisimov (Denis) wrote…

new line

Done.

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java, line 36 at r6 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Document this public class by adding an explicit description.

Done.

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java, line 117 at r6 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Remove this empty statement.

Done.

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java, line 119 at r6 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Document this public method by adding an explicit description.
Refactor this method to throw at most one checked exception instead of: java.io.IOException, java.lang.ClassNotFoundException

Done.

Comments from Reviewable

denis-anisimov · 2018-05-23T10:30:09Z

Reviewed 2 of 3 files at r7.
Review status: all files reviewed at latest revision, 27 unresolved discussions.

Comments from Reviewable

vaadin-bot · 2018-05-23T13:01:51Z

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java

+        } else if (getJarPattern().matcher(file.getName()).matches()) {
+            classes = findClassesInJar(file);
+        } else {
+            logger.debug("Ignoring " + classpathEntry);


Use the built-in formatting to construct this argument.

denis-anisimov · 2018-05-24T05:46:26Z

Reviewed 1 of 2 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved.

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java, line 315 at r9 (raw file):

if (iface == Runnable.class || iface == Comparator.class) {

So if I implement a Runnable or Comparator class myself which is a real class it will be skipped?

I think it should check something isAnonymousClass() or isSynthetic() or whatever.

And isSkippable is a bad name. It should say why it may be ignored (or just canIgnore ).

Comments from Reviewable

elmot · 2018-05-24T07:56:46Z

Review status: 48 of 49 files reviewed at latest revision, 1 unresolved discussion.

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java, line 315 at r9 (raw file):

Previously, denis-anisimov (Denis) wrote…

if (iface == Runnable.class || iface == Comparator.class) {
So if I implement a Runnable or Comparator class myself which is a real class it will be skipped?

I think it should check something isAnonymousClass() or isSynthetic() or whatever.

And isSkippable is a bad name. It should say why it may be ignored (or just canIgnore ).

I also had some concerns about this code. Removed.

Comments from Reviewable

vaadin-bot · 2018-05-24T08:10:36Z

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java

+     * @throws Throwable serialization goes wrong
+     */
+    @Test
+    public void classesSerializable() throws Throwable {


Define and throw a dedicated exception instead of using a generic one.

vaadin-bot · 2018-05-24T08:10:37Z

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java

+            // report non-serializable classes and interfaces
+            if (!Serializable.class.isAssignableFrom(cls)) {
+                nonSerializableClasses.add(cls);
+                // TODO easier to read when testing


Complete the task associated to this TODO comment.

vaadin-bot · 2018-05-24T08:10:37Z

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java

+            if (!Serializable.class.isAssignableFrom(cls)) {
+                nonSerializableClasses.add(cls);
+                // TODO easier to read when testing
+                // System.err.println(cls);


This block of commented-out lines of code should be removed.

elmot · 2018-05-24T08:17:00Z

Review status: 48 of 49 files reviewed at latest revision, 2 unresolved discussions.

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java, line 148 at r10 (raw file):

serializeAndDeserialize

Comments from Reviewable

vaadin-bot · 2018-05-24T08:31:03Z

flow-test-generic/src/main/java/com/vaadin/flow/testutil/ClassesSerializableTest.java

+     */
+    @SuppressWarnings({"UnusedReturnValue", "WeakerAccess"})
+    public <T> T serializeAndDeserialize(T instance)
+            throws Throwable {


Define and throw a dedicated exception instead of using a generic one.

vaadin-bot · 2018-05-24T08:31:06Z

SonarQube analysis reported 16 issues

13 major
2 minor
1 info

Watch the comments in this conversation to review them.

9 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:

EventDataCache.java#L1: Class com.vaadin.flow.component.internal.EventDataCache defines non-transient non-serializable instance field eventConstructors
ElementListenerMap.java#L368: Rename "listeners" which hides the field declared at line 58.
InitialPageSettings.java#L1: Class com.vaadin.flow.server.InitialPageSettings defines non-transient non-serializable instance field elements
InitialPageSettings.java#L1: Class com.vaadin.flow.server.InitialPageSettings defines non-transient non-serializable instance field request
InitialPageSettings.java#L55: Make "request" transient or serializable.
InitialPageSettings.java#L65: Make "elements" transient or serializable.
RouteRegistry.java#L575: A "NullPointerException" could be thrown; "navigationTarget" is nullable here.
ReflectionCache.java#L82: Remove this unnecessary cast to "Function".
ElementListenerMap.java#L327: Move this method into "DomEventListenerWrapper".

denis-anisimov · 2018-05-24T08:59:15Z

Reviewed 1 of 2 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

vaadin-bot reviewed May 21, 2018

View reviewed changes

caalador added the breaking change label May 22, 2018

caalador assigned elmot May 22, 2018

elmot added 5 commits May 22, 2018 16:45

Make classes serializable

14956bc

Sponarqube fixes

5c83e4a

Fixes

376e5c1

Fixes

8420ac0

Fixes

e677f0e

elmot force-pushed the serialization branch from 55cb376 to e677f0e Compare May 22, 2018 14:13

elmot added 3 commits May 23, 2018 10:57

Fixes

affca6f

Fixes

95182a1

Refactoring and optimization

73d72ca

elmot added 2 commits May 23, 2018 11:37

Small fix

82aa7e6

SplitPackagesTest ignores flow-test-generic

865a6e5

vaadin-bot reviewed May 23, 2018

View reviewed changes

Make sonarqube happy

e1f45d7

Use sl4j

6bbab68

elmot added 3 commits May 23, 2018 13:35

Add a copyright

0d4d0be

Optimize, fix sonarqube issues

239a1e5

Make the test friendly for the depended projects

3e69650

vaadin-bot reviewed May 23, 2018

View reviewed changes

After review fix, small improvements

2516547

vaadin-bot reviewed May 24, 2018

View reviewed changes

elmot added 2 commits May 24, 2018 11:10

Small improvements

e339097

Javadoc

37365be

vaadin-bot reviewed May 24, 2018

View reviewed changes

denis-anisimov merged commit bad717d into master May 24, 2018

denis-anisimov deleted the serialization branch May 24, 2018 08:59

pekam added this to the 1.0.0.beta12 milestone May 24, 2018

elmot mentioned this pull request May 29, 2018

Cannot construct complex predicates with AND and OR #3969

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make classes serializable #4148

Make classes serializable #4148

elmot commented May 21, 2018 •

edited by SomeoneToIgnore

Loading

vaadin-bot May 21, 2018

caalador commented May 22, 2018

denis-anisimov commented May 22, 2018

elmot commented May 22, 2018

denis-anisimov commented May 22, 2018

denis-anisimov commented May 22, 2018

elmot commented May 22, 2018

denis-anisimov commented May 22, 2018

denis-anisimov commented May 23, 2018

elmot commented May 23, 2018

elmot commented May 23, 2018

vaadin-bot May 23, 2018

denis-anisimov commented May 23, 2018

elmot commented May 23, 2018

denis-anisimov commented May 23, 2018

vaadin-bot May 23, 2018

denis-anisimov commented May 24, 2018

elmot commented May 24, 2018

vaadin-bot May 24, 2018

vaadin-bot May 24, 2018

vaadin-bot May 24, 2018

elmot commented May 24, 2018

vaadin-bot May 24, 2018

vaadin-bot commented May 24, 2018

denis-anisimov commented May 24, 2018

Make classes serializable #4148

Make classes serializable #4148

Conversation

elmot commented May 21, 2018 • edited by SomeoneToIgnore Loading

vaadin-bot May 21, 2018

Choose a reason for hiding this comment

caalador commented May 22, 2018

denis-anisimov commented May 22, 2018

elmot commented May 22, 2018

denis-anisimov commented May 22, 2018

denis-anisimov commented May 22, 2018

elmot commented May 22, 2018

denis-anisimov commented May 22, 2018

denis-anisimov commented May 23, 2018

elmot commented May 23, 2018

elmot commented May 23, 2018

vaadin-bot May 23, 2018

Choose a reason for hiding this comment

denis-anisimov commented May 23, 2018

elmot commented May 23, 2018

denis-anisimov commented May 23, 2018

vaadin-bot May 23, 2018

Choose a reason for hiding this comment

denis-anisimov commented May 24, 2018

elmot commented May 24, 2018

vaadin-bot May 24, 2018

Choose a reason for hiding this comment

vaadin-bot May 24, 2018

Choose a reason for hiding this comment

vaadin-bot May 24, 2018

Choose a reason for hiding this comment

elmot commented May 24, 2018

vaadin-bot May 24, 2018

Choose a reason for hiding this comment

vaadin-bot commented May 24, 2018

9 extra issues

denis-anisimov commented May 24, 2018

elmot commented May 21, 2018 •

edited by SomeoneToIgnore

Loading