fix conversion by annotation and WW-4906 #199

yasserzamani · 2018-01-07T12:22:22Z

No description provided.

See WW-4906

coveralls · 2018-01-07T12:39:08Z

Coverage increased (+0.02%) to 46.336% when pulling 4394238 on yasserzamani:WW-4906 into a66f92c on apache:master.

Note: Also will delete CLASS from annotation type because it is already the default value

coveralls · 2018-01-07T14:02:56Z

Coverage increased (+0.04%) to 46.349% when pulling 9739291 on yasserzamani:WW-4906 into a66f92c on apache:master.

yasserzamani · 2018-01-07T14:31:37Z

For easier review, please see all commits together at once , and at one column (not side by side)

lukaszlenart · 2018-01-10T06:15:44Z

So this PR fixes two issues, right? If so, I would register another ticket in JIRA to cover the second issue.

lukaszlenart · 2018-01-10T06:23:40Z

core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java

@@ -582,7 +582,7 @@ public void testGetBeanMap() throws Exception {
        // just do some of the 15 tests
        Map beans = ognlUtil.getBeanMap(foo);
        assertNotNull(beans);
-        assertEquals(19, beans.size());
+        assertEquals(21, beans.size());


Why there is 2 more beans?

I added two more beans to Foo to test conversion by annotation and cover all possible usages :)

lukaszlenart · 2018-01-10T06:49:38Z

.../main/java/com/opensymphony/xwork2/conversion/impl/DefaultConversionAnnotationProcessor.java

@@ -69,7 +70,7 @@ public void process(Map<String, Object> mapping, TypeConversion tc, String key)
                    mapping.put(key, tc.value());
                }
                //for properties of classes
-                else if (tc.rule() != ConversionRule.ELEMENT || tc.rule() == ConversionRule.KEY || tc.rule() == ConversionRule.COLLECTION) {
+                else if (tc.rule() != ConversionRule.ELEMENT && tc.rule() != ConversionRule.KEY && tc.rule() != ConversionRule.COLLECTION) {


Not sure if I understand this change, we expected KEY or COLLECTION here but now we handle none of them which means we will handle KEY_PROPERTY or CREATE_IF_NULL

This is a fix for that issue which I discussed at dev list. I suspected it should be wrong because this else if then does not require || tc.rule() == ConversionRule.KEY || tc.rule() == ConversionRule.COLLECTION because it's always true when tc.rule() != ConversionRule.ELEMENT! Also, this else if was handling all tc.rule() != ConversionRule.ELEMENT and execution never reached it's next else if, tc.rule() == ConversionRule.KEY.

So i asked at dev list and your reply directed me that this code is copied from DefaultConversionFileProcessor.java#78. Please see there that a NOT is applied to all statement but the copier of code removed the NOT but did not applied on all but only on first. This is that second issue which is fixed here side by side WW-4906 :)

lukaszlenart · 2018-01-10T06:53:25Z

.../main/java/com/opensymphony/xwork2/conversion/impl/DefaultConversionAnnotationProcessor.java

+                            mapping.put(key, converterCreator.createTypeConverter(tc.converter()));
+                        } else {
+                            mapping.put(key, converterCreator.createTypeConverter(tc.converterClass()));
+                        }


Instead this if statement, using converterClass only should be sufficient

I afraid if user e.g. has a spring bean with name "java.lang.String" but with another type than string. In such cases, converterCreator.createTypeConverter("java.lang.String") and converterCreator.createTypeConverter(java.lang.String.class) will have different results :)

Not sure if I understand your example with String.class - in this place there be only TypeConverter.class, not a string. The .converter() value was used above to load a class (line 84), on line 94 we check if the class is TypeConverter.class and in such case we create the converter. So in both case it must be an instance of TypeConverter class.

Thanks! You're right. Please replace java.lang.String with com.opensymphony.xwork2.conversion.TypeConverter in my above comment. This is an odd thing :) but please consider a spring bean with name "com.opensymphony.xwork2.conversion.TypeConverter" and that bean type is me.yasser.MyTypeConverter. In such case, converterCreator.createTypeConverter("com.opensymphony.xwork2.conversion.TypeConverter") and converterCreator.createTypeConverter(com.opensymphony.xwork2.conversion.TypeConverter.class) will have different results.

At bottom, I think this was not working already before these and previous changes. Because DefaultObjectTypeDeterminer.getKeyClass always casts it to Class and will fail at this case when it's TypeConverter.

At bottom, I think this was not working already before these and previous changes. Because DefaultObjectTypeDeterminer.getKeyClass always casts it to Class and will fail at this case when it's TypeConverter.

FYI: I knew this piece of code was not working already before these and previous changes. But I just kept it unchanged as maybe I don't know something.

Hm... still not sure if I follow (not a Spring fan) but it's safer to keep it as is if you still have some objections and corner cases.

lukaszlenart · 2018-01-10T06:58:24Z

core/src/main/java/com/opensymphony/xwork2/conversion/impl/XWorkConverter.java

+                            case KEY_PROPERTY:
+                                key = DefaultObjectTypeDeterminer.KEY_PROPERTY_PREFIX + key;
+                                break;
+                        }


but in this case it won't be possible to use per property conversion, it will always be prefixed. I think there should be a check mapping,containsKey(propertyName) that breaks the chain.

No, it will be prefixed if was empty (i.e. is not specified by user explicitly) and not PROPERTY (i.e. allows per property conversion).

I meant I didn't add all possible cases in switch-case. My added tests show it works.

lukaszlenart · 2018-01-10T07:03:46Z

core/src/test/java/com/opensymphony/xwork2/util/MyBeanActionTest.java

+            assertEquals(MyBean.class.getName(), proxy.getInvocation().getStack().findValue("annotatedBeanList.get(1)").getClass().getName());
+
+            assertEquals("This is the bla bean by annotation", proxy.getInvocation().getStack().findValue("annotatedBeanList.get(0).name"));
+            assertEquals(new Long(1234567890), Long.valueOf(proxy.getInvocation().getStack().findValue("annotatedBeanList.get(0).id").toString()));


new Long?

I just copied it's above already test lines 😉

lukaszlenart · 2018-01-10T07:05:27Z

core/src/test/java/com/opensymphony/xwork2/util/MyBeanActionTest.java

+            assertEquals(2, Integer.parseInt(proxy.getInvocation().getStack().findValue("annotatedBeanMap.size").toString()));
+
+            assertEquals(true, action.getAnnotatedBeanMap().containsKey(1234567890L));
+            assertEquals(true, action.getAnnotatedBeanMap().containsKey(1234567891L));


I think it would be good to check values of these keys.

I see they are already checked in it's following lines using findValue :)

hm... findValue() performs a scan over the stack so it can find other values than expected but I think in this case it's ok.

yasserzamani · 2018-01-10T10:53:59Z

Thanks! To document second fix, I created WW-4908. I hope this PR did not confuse you. The fix itself is small as I only made a few changes into only two main files, DefaultConversionAnnotationProcessor.java and XWorkConverter.java. Rest are only heavy tests 😉

lukaszlenart · 2018-01-15T10:50:00Z

LGTM 👍

yasserzamani added 4 commits January 7, 2018 12:34

add conversion by annotation missing tests

faf5018

fix conversion by annotation

77cbafb

add tests if conversion can get annotation from supers

ff903e7

un-deprecate TypeConversion.converter plus some improvements

4394238

See WW-4906

change tests to cover more

9739291

Note: Also will delete CLASS from annotation type because it is already the default value

yasserzamani added the Ready For Merge label Jan 7, 2018

lukaszlenart requested changes Jan 10, 2018

View reviewed changes

lukaszlenart approved these changes Jan 15, 2018

View reviewed changes

lukaszlenart merged commit 29e1847 into apache:master Jan 15, 2018

yasserzamani deleted the WW-4906 branch January 16, 2018 07:58

fix conversion by annotation and WW-4906 #199

fix conversion by annotation and WW-4906 #199

Uh oh!

Conversation

yasserzamani commented Jan 7, 2018

Uh oh!

coveralls commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yasserzamani commented Jan 7, 2018

Uh oh!

lukaszlenart commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yasserzamani commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszlenart commented Jan 15, 2018

Uh oh!

Uh oh!

coveralls commented Jan 7, 2018 •

edited

Loading

coveralls commented Jan 7, 2018 •

edited

Loading

yasserzamani commented Jan 10, 2018 •

edited

Loading