Return bean proxy for model. #911

denis-anisimov · 2016-06-03T10:12:56Z

Provides a proxy for the given part of the model whose changes are
reflected back to the model

Fixes #731.

This change is

Fixes #880

Provides a proxy for the given part of the model whose changes are reflected back to the model Fixes #731.

Conflicts: hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModel.java

Conflicts: hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModel.java hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelBeanUtil.java hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelProxyHandler.java hummingbird-server/src/test/java/com/vaadin/hummingbird/template/model/TemplateModelTest.java

vaadin-tc · 2016-06-03T11:57:51Z

...rd-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelProxyHandler.java

+                        modelType.getName()), e);
+            } catch (IllegalAccessException e) {
+                // this should not happen
+                throw new RuntimeException(String.format(


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

vaadin-tc · 2016-06-03T12:39:10Z

...rd-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelProxyHandler.java


-        throw new UnsupportedOperationException(
-                getUnsupportedMethodMessage(method, args));
+    private static boolean isAccessor(MethodDescription method) {


Private method 'isAccessor' is never used.

Conflicts: hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModel.java hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelBeanUtil.java hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelProxyHandler.java

Artur- · 2016-06-06T07:46:11Z

Reviewed 2 of 6 files at r2, 3 of 4 files at r4, 1 of 2 files at r5.
Review status: 6 of 7 files reviewed at latest revision, 8 unresolved discussions.

hummingbird-server/pom.xml, line 102 [r5] (raw file):

      <dependency>
          <groupId>net.bytebuddy</groupId>
          <artifactId>byte-buddy</artifactId>

Does byte-buddy work with OSGi?

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModel.java, line 125 [r5] (raw file):

    default <T> T getProxy(String modelPath, Class<T> beanType) {
        // The method is handled by proxy handler
        return null;

Should you rather throw an exception if you for whatever reason reach this code?

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelBeanUtil.java, line 141 [r5] (raw file):

        StateNode node = stateNode;
        StringTokenizer tokenizer = new StringTokenizer(modelPath, ".");

FYI If you use String.split you do not have to loop an extra round and then undo the last loop using node.getParent() and last

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelProxyHandler.java, line 130 [r5] (raw file):

                        ClassLoadingStrategy.Default.WRAPPER)
                .getLoaded();
        Optional<Constructor<?>> defaultCtor = Stream

Constructor defaultCtor = modelType.getConstructor() ?

Comments from Reviewable

denis-anisimov · 2016-06-06T08:21:22Z

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

hummingbird-server/pom.xml, line 102 [r5] (raw file):

Previously, Artur- (Artur) wrote…

Does byte-buddy work with OSGi?

Should work. Didn't find any known issues. It's more about how it's used to be compatible with OSGi: is current implementation is good for OSGi ? There are two possible issues: correct class loader which is used to load proxy class and instantiation using reflection (see the code). But that's a bit different topic.

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModel.java, line 125 [r5] (raw file):

Previously, Artur- (Artur) wrote…

Should you rather throw an exception if you for whatever reason reach this code?

Done.

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelBeanUtil.java, line 141 [r5] (raw file):

Previously, Artur- (Artur) wrote…

FYI If you use String.split you do not have to loop an extra round and then undo the last loop using node.getParent() and last

With a split it will look like this:

modelPath.split("\\.'");

So the argument is 3 characters string and split will use real regexp to do split (for one/two characters string simplified logic is used).
It has no sense to use regexp here for such simple case. Although the code becomes shorter of course..

So it longer code vs unnecessary regexp usage : I\m not sure what's better.

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelProxyHandler.java, line 130 [r5] (raw file):

Previously, Artur- (Artur) wrote…

Constructor defaultCtor = modelType.getConstructor() ?

The `modelType.getConstructor()` method throws an exception if there is no default CTOR (as almost any method in reflection API).

Comments from Reviewable

Artur- · 2016-06-06T12:45:19Z

Reviewed 1 of 1 files at r6.
Review status: 6 of 7 files reviewed at latest revision, 6 unresolved discussions.

hummingbird-server/pom.xml, line 102 [r5] (raw file):

Previously, denis-anisimov wrote…

Should work. Didn't find any known issues.
It's more about how it's used to be compatible with OSGi: is current implementation is good for OSGi ?
There are two possible issues: correct class loader which is used to load proxy class and instantiation using reflection (see the code).
But that's a bit different topic.

Added comment to OSGi issue https://github.com//issues/455

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelBeanUtil.java, line 141 [r5] (raw file):

Previously, denis-anisimov wrote…

With a split it will look like this:
modelPath.split("\\.'");
So the argument is 3 characters string and split will use real regexp to do split (for one/two characters string simplified logic is used).
It has no sense to use regexp here for such simple case. Although the code becomes shorter of course..

So it longer code vs unnecessary regexp usage : I\m not sure what's better.

"StringTokenizer is a legacy class that is retained for compatibility reasons although its use is discouraged in new code. It is recommended that anyone seeking this functionality use the split method of String or the java.util.regex package instead."

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelProxyHandler.java, line 130 [r5] (raw file):

Previously, denis-anisimov wrote…

The modelType.getConstructor() method throws an exception if there is no default CTOR (as almost any method in reflection API).

Which is what you also want to do here if there is no public constructor?

Comments from Reviewable

denis-anisimov · 2016-06-06T14:11:43Z

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

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelBeanUtil.java, line 141 [r5] (raw file):

Previously, Artur- (Artur) wrote…

"StringTokenizer is a legacy class that is retained for compatibility reasons although its use is discouraged in new code. It is recommended that anyone seeking this functionality use the split method of String or the java.util.regex package instead."

Would be better if they just deprecated the class.

And by the way I cannot avoid extra loop and using node.getParent() because I need to call resolveStateNode method to satisfy getModelValue method which checks the value type.

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelProxyHandler.java, line 130 [r5] (raw file):

Previously, Artur- (Artur) wrote…

Which is what you also want to do here if there is no public constructor?

Several reasons: 1) getConstructor() throws generic reflection checked exception. Which means it has to be caught and rethrown. 2) The constructor may be available but is not public. As a result the logic will contain one more catch that just rethrows an exception and still requires check for public modifier.

The easier way would be to avoid check for CTOR at all and just do proxy.newInstance();. But then messages in rethrown exceptions are not explanatory enough.

Comments from Reviewable

Artur- · 2016-06-07T09:43:02Z

Reviewed 1 of 2 files at r5, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelBeanUtil.java, line 293 [r7] (raw file):

                        + getSupportedTypesString());
    }
}

Newline

hummingbird-server/src/test/java/com/vaadin/hummingbird/template/model/TemplateModelTest.java, line 674 [r7] (raw file):

    }

    private void verifyBeanValue(ModelMap feature, Supplier<Object> getter,

verifyBeanValue sounds like it verifies a value in a bean but in reality it sets a property in a model map and validates that the given getter returns that value?

hummingbird-server/src/test/java/com/vaadin/hummingbird/template/model/TemplateModelTest.java, line 683 [r7] (raw file):

            String beanPath1, String beanPath2, String property,
            Serializable expected) {
        Serializable bean = template.getElement().getNode()

FYI Most of the three last verify* methods are about fetching the model map for a given path, which could be extracted to a helper taking a dot separated path

Comments from Reviewable

denis-anisimov · 2016-06-07T09:58:33Z

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

hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelBeanUtil.java, line 293 [r7] (raw file):

Previously, Artur- (Artur) wrote…

Newline

Done.

hummingbird-server/src/test/java/com/vaadin/hummingbird/template/model/TemplateModelTest.java, line 674 [r7] (raw file):

Previously, Artur- (Artur) wrote…

verifyBeanValue sounds like it verifies a value in a bean but in reality it sets a property in a model map and validates that the given getter returns that value?

Yes, and what do you suggest as a better name here ?

Comments from Reviewable

Artur- · 2016-06-07T10:01:08Z

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

hummingbird-server/src/test/java/com/vaadin/hummingbird/template/model/TemplateModelTest.java, line 674 [r7] (raw file):

Previously, denis-anisimov wrote…

Yes, and what do you suggest as a better name here ?

setModelAndVerifyUsingGetter? setModelPropertyAndVerifyGetter?

Comments from Reviewable

denis-anisimov · 2016-06-07T10:11:05Z

Review status: 5 of 7 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

hummingbird-server/src/test/java/com/vaadin/hummingbird/template/model/TemplateModelTest.java, line 674 [r7] (raw file):

Previously, Artur- (Artur) wrote…

setModelAndVerifyUsingGetter?
setModelPropertyAndVerifyGetter?

Done.

hummingbird-server/src/test/java/com/vaadin/hummingbird/template/model/TemplateModelTest.java, line 683 [r7] (raw file):

Previously, Artur- (Artur) wrote…

FYI Most of the three last verify* methods are about fetching the model map for a given path, which could be extracted to a helper taking a dot separated path

I'm not sure that in the tests it's the best approach : it's absolutely clear what's going on with the model without cycle. But let it be.

Comments from Reviewable

Artur- · 2016-06-07T10:31:20Z

Reviewed 1 of 2 files at r8, 3 of 3 files at r9.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

hummingbird-server/src/test/java/com/vaadin/hummingbird/template/model/TemplateModelTest.java, line 683 [r7] (raw file):

Previously, denis-anisimov wrote…

I'm not sure that in the tests it's the best approach : it's absolutely clear what's going on with the model without cycle.
But let it be.

The test is easier to understand for me at least when there is one parameter for the path.

Now you just need to extract the duplicated code to a method like getModelMap(String beanPath)

Comments from Reviewable

Conflicts: hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelProxyHandler.java

denis-anisimov · 2016-06-07T10:39:38Z

Review status: 6 of 7 files reviewed at latest revision, 4 unresolved discussions.

hummingbird-server/src/test/java/com/vaadin/hummingbird/template/model/TemplateModelTest.java, line 683 [r7] (raw file):

Previously, Artur- (Artur) wrote…

The test is easier to understand for me at least when there is one parameter for the path.

Now you just need to extract the duplicated code to a method like getModelMap(String beanPath)

Done.

Comments from Reviewable

vaadin-tc · 2016-06-07T10:48:23Z

SonarQube analysis reported 4 issues

4 major

Watch the comments in this conversation to review them.

1 extra issue

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:

TemplateModelBeanUtil.java#L246: The Cyclomatic Complexity of this method "getModelValueBasicType" is 11 which is greater than 10 authorized.

Artur- · 2016-06-07T10:48:46Z

Reviewed 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

pleku and others added 3 commits June 2, 2016 10:59

Import nested beans into model

dbb9020

Fixes #880

Return bean proxy for model.

d95a17c

Provides a proxy for the given part of the model whose changes are reflected back to the model Fixes #731.

Merge branch '880-nested-beans' into 731-proxy-bean

c1cdb43

Conflicts: hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModel.java

denis-anisimov added the in progress label Jun 3, 2016

denis-anisimov force-pushed the 731-proxy-bean branch from e1fef00 to eb8a612 Compare June 3, 2016 10:58

vaadin-tc reviewed Jun 3, 2016
View reviewed changes

denis-anisimov force-pushed the 731-proxy-bean branch from e841749 to fa06aa5 Compare June 3, 2016 12:29

vaadin-tc reviewed Jun 3, 2016
View reviewed changes

denis-anisimov force-pushed the 731-proxy-bean branch from fa06aa5 to 4c41458 Compare June 3, 2016 13:57

denis-anisimov force-pushed the 731-proxy-bean branch from b7b621c to 7199a72 Compare June 6, 2016 08:21

denis-anisimov force-pushed the 731-proxy-bean branch from 7199a72 to e4f9fb8 Compare June 6, 2016 14:11

Merge branch 'master' into 731-proxy-bean

b811c87

denis-anisimov force-pushed the 731-proxy-bean branch from e4f9fb8 to b811c87 Compare June 7, 2016 10:09

denis-anisimov force-pushed the 731-proxy-bean branch from c69fb05 to d71078b Compare June 7, 2016 10:39

Merge branch 'master' into 731-proxy-bean

d71078b

Conflicts: hummingbird-server/src/main/java/com/vaadin/hummingbird/template/model/TemplateModelProxyHandler.java

Artur- merged commit 8694dad into master Jun 7, 2016

Artur- deleted the 731-proxy-bean branch June 7, 2016 10:49

Artur- removed the in progress label Jun 7, 2016

Artur- modified the milestone: 0.0.9 Jun 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return bean proxy for model. #911

Return bean proxy for model. #911

denis-anisimov commented Jun 3, 2016 •

edited by Artur-

vaadin-tc Jun 3, 2016

vaadin-tc Jun 3, 2016

Artur- commented Jun 6, 2016

denis-anisimov commented Jun 6, 2016

Artur- commented Jun 6, 2016

denis-anisimov commented Jun 6, 2016

Artur- commented Jun 7, 2016

denis-anisimov commented Jun 7, 2016

Artur- commented Jun 7, 2016

denis-anisimov commented Jun 7, 2016

Artur- commented Jun 7, 2016

denis-anisimov commented Jun 7, 2016

vaadin-tc commented Jun 7, 2016

Artur- commented Jun 7, 2016

Return bean proxy for model. #911

Return bean proxy for model. #911

Conversation

denis-anisimov commented Jun 3, 2016 • edited by Artur-

vaadin-tc Jun 3, 2016

Choose a reason for hiding this comment

vaadin-tc Jun 3, 2016

Choose a reason for hiding this comment

Artur- commented Jun 6, 2016

denis-anisimov commented Jun 6, 2016

Artur- commented Jun 6, 2016

denis-anisimov commented Jun 6, 2016

Artur- commented Jun 7, 2016

denis-anisimov commented Jun 7, 2016

Artur- commented Jun 7, 2016

denis-anisimov commented Jun 7, 2016

Artur- commented Jun 7, 2016

denis-anisimov commented Jun 7, 2016

vaadin-tc commented Jun 7, 2016

1 extra issue

Artur- commented Jun 7, 2016

denis-anisimov commented Jun 3, 2016 •

edited by Artur-