Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix conversion by annotation and WW-4906 #199

Merged
merged 5 commits into from
Jan 15, 2018
Merged

fix conversion by annotation and WW-4906 #199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
faf5018
add conversion by annotation missing tests
yasserzamani Jan 7, 2018
77cbafb
fix conversion by annotation
yasserzamani Jan 7, 2018
ff903e7
add tests if conversion can get annotation from supers
yasserzamani Jan 7, 2018
4394238
un-deprecate TypeConversion.converter plus some improvements
yasserzamani Jan 7, 2018
9739291
change tests to cover more
yasserzamani Jan 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions core/src/main/java/com/opensymphony/xwork2/conversion/annotations/TypeConversion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,15 @@
* </tr>
* <tr>
* <td>converter</td>
* <td>DEPRECATED: either this or value</td>
* <td>either this or value</td>
* <td>&nbsp;</td>
* <td>The class name of the TypeConverter to be used as converter.</td>
* <td>The class or bean name of the TypeConverter to be used as converter.</td>
* </tr>
* <tr>
* <td>converterClass</td>
* <td>either this or value</td>
* <td>&nbsp;</td>
* <td>The class of the TypeConverter to be used as converter. XWorkBasicConverter by default.</td>
* <td>XWorkBasicConverter</td>
* <td>The class of the TypeConverter to be used as converter.</td>
* </tr>
* <tr>
* <td>value</td>
Expand Down Expand Up @@ -181,14 +181,13 @@
ConversionRule rule() default ConversionRule.PROPERTY;

/**
* The class of the TypeConverter to be used as converter.
* The class or bean name of the TypeConverter to be used as converter.
*
* Note: This can not be used with ConversionRule.KEY_PROPERTY!
*
* @return class of the TypeConverter to be used as converter
* @deprecated user {@link #converterClass()} instead
* @return class or bean name of the TypeConverter to be used as converter
* @see {@link #converterClass()}
*/
@Deprecated
String converter() default "";

/**
Expand Down
19 changes: 12 additions & 7 deletions ...in/java/com/opensymphony/xwork2/conversion/impl/DefaultConversionAnnotationProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.opensymphony.xwork2.conversion.annotations.ConversionType;
import com.opensymphony.xwork2.conversion.annotations.TypeConversion;
import com.opensymphony.xwork2.inject.Inject;
import com.opensymphony.xwork2.util.ClassLoaderUtil;
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (StringUtils.isNoneEmpty(tc.converter())) {
mapping.put(key, converterCreator.createTypeConverter(tc.converter()));
} else {
Expand All @@ -80,18 +81,22 @@ else if (tc.rule() != ConversionRule.ELEMENT || tc.rule() == ConversionRule.KEY
else if (tc.rule() == ConversionRule.KEY) {
Class<?> converterClass;
if (StringUtils.isNoneEmpty(tc.converter())) {
converterClass = Thread.currentThread().getContextClassLoader().loadClass(tc.converter());
//check if the converter is a type converter if it is one
//then just put it in the map as is. Otherwise
//put a value in for the type converter of the class
converterClass = ClassLoaderUtil.loadClass(tc.converter(), this.getClass());
} else {
converterClass = tc.converterClass();
}

LOG.debug("Converter class: [{}]", converterClass);

//check if the converter is a type converter if it is one
//then just put it in the map as is. Otherwise
//put a value in for the type converter of the class
if (converterClass.isAssignableFrom(TypeConverter.class)) {
mapping.put(key, converterCreator.createTypeConverter(tc.converter()));
if (StringUtils.isNoneEmpty(tc.converter())) {
mapping.put(key, converterCreator.createTypeConverter(tc.converter()));
} else {
mapping.put(key, converterCreator.createTypeConverter(tc.converterClass()));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead this if statement, using converterClass only should be sufficient

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
mapping.put(key, converterClass);
LOG.debug("Object placed in mapping for key [{}] is [{}]", key, mapping.get(key));
Expand All @@ -100,7 +105,7 @@ else if (tc.rule() == ConversionRule.KEY) {
//elements(values) of maps / lists
else {
if (StringUtils.isNoneEmpty(tc.converter())) {
mapping.put(key, Thread.currentThread().getContextClassLoader().loadClass(tc.converter()));
mapping.put(key, ClassLoaderUtil.loadClass(tc.converter(), this.getClass()));
} else {
mapping.put(key, tc.converterClass());
}
Expand Down
4 changes: 2 additions & 2 deletions ...src/main/java/com/opensymphony/xwork2/conversion/impl/DefaultConversionFileProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ else if (!(key.startsWith(DefaultObjectTypeDeterminer.ELEMENT_PREFIX) ||
//for keys of Maps
else if (key.startsWith(DefaultObjectTypeDeterminer.KEY_PREFIX)) {

Class converterClass = Thread.currentThread().getContextClassLoader().loadClass((String) entry.getValue());
Class converterClass = ClassLoaderUtil.loadClass((String) entry.getValue(), this.getClass());

//check if the converter is a type converter if it is one
//then just put it in the map as is. Otherwise
Expand All @@ -102,7 +102,7 @@ else if (key.startsWith(DefaultObjectTypeDeterminer.KEY_PREFIX)) {
}
//elements(values) of maps / lists
else {
Class _c = Thread.currentThread().getContextClassLoader().loadClass((String) entry.getValue());
Class _c = ClassLoaderUtil.loadClass((String) entry.getValue(), this.getClass());
LOG.debug("\t{}:{} [treated as Class {}]", key, entry.getValue(), _c);
mapping.put(key, _c);
}
Expand Down
25 changes: 21 additions & 4 deletions core/src/main/java/com/opensymphony/xwork2/conversion/impl/XWorkConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,15 +500,32 @@ protected void addConverterMapping(Map<String, Object> mapping, Class clazz) {
for (Annotation annotation : annotations) {
if (annotation instanceof TypeConversion) {
TypeConversion tc = (TypeConversion) annotation;
if (mapping.containsKey(tc.key())) {
break;
}
String key = tc.key();
// Default to the property name
// Default to the property name with prefix
if (StringUtils.isEmpty(key)) {
key = AnnotationUtils.resolvePropertyName(method);
switch (tc.rule()) {
case COLLECTION:
key = DefaultObjectTypeDeterminer.DEPRECATED_ELEMENT_PREFIX + key;
break;
case CREATE_IF_NULL:
key = DefaultObjectTypeDeterminer.CREATE_IF_NULL_PREFIX + key;
break;
case ELEMENT:
key = DefaultObjectTypeDeterminer.ELEMENT_PREFIX + key;
break;
case KEY:
key = DefaultObjectTypeDeterminer.KEY_PREFIX + key;
break;
case KEY_PROPERTY:
key = DefaultObjectTypeDeterminer.KEY_PROPERTY_PREFIX + key;
break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LOG.debug("Retrieved key [{}] from method name [{}]", key, method.getName());
}
if (mapping.containsKey(key)) {
break;
}
annotationProcessor.process(mapping, tc, key);
}
}
Expand Down
13 changes: 13 additions & 0 deletions core/src/test/java/com/opensymphony/xwork2/AnnotatedTestBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
*/
package com.opensymphony.xwork2;

import com.opensymphony.xwork2.conversion.annotations.TypeConversion;
import com.opensymphony.xwork2.conversion.impl.FooBarConverter;
import com.opensymphony.xwork2.util.Bar;
import com.opensymphony.xwork2.validator.annotations.IntRangeFieldValidator;
import com.opensymphony.xwork2.validator.annotations.RequiredStringValidator;
import com.opensymphony.xwork2.validator.annotations.Validations;
Expand All @@ -37,6 +40,7 @@ public class AnnotatedTestBean {
private Date birth;
private String name;
private int count;
private Bar bar;

//~ Constructors ///////////////////////////////////////////////////////////

Expand Down Expand Up @@ -76,4 +80,13 @@ public void setName(String name) {
public String getName() {
return name;
}

public Bar getSupperBarObj() {
return bar;
}

@TypeConversion(converter = "com.opensymphony.xwork2.conversion.impl.FooBarConverter")
public void setSupperBarObj(Bar b) {
bar = b;
}
}
10 changes: 9 additions & 1 deletion core/src/test/java/com/opensymphony/xwork2/conversion/impl/AnnotationXWorkConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ public void testFindConversionErrorMessage() {
assertEquals("Invalid field value for field \"foo\".", message);
}

public void testFindConversionMappingForInterface() {
public void testFindConversionMappingForInterfaceAndSuperclass() {
ModelDrivenAnnotationAction2 action = new ModelDrivenAnnotationAction2();
ValueStack stack = ActionContext.getContext().getValueStack();
stack.push(action);
Expand All @@ -193,6 +193,14 @@ public void testFindConversionMappingForInterface() {

Bar b = (Bar) o;
assertEquals(value, b.getTitle() + ":" + b.getSomethingElse());

String value2 = "qwer:456";
Object o2 = converter.convertValue(ognlStackContext, action.getModel(), null, "supperBarObj", value2, Bar.class);
assertNotNull(o2);
assertTrue("class is: " + o.getClass(), o2 instanceof Bar);

Bar b2 = (Bar) o2;
assertEquals(value2, b2.getTitle() + ":" + b2.getSomethingElse());
}

public void testLocalizedDateConversion() throws Exception {
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why there is 2 more beans?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assertEquals("Hello Santa", beans.get("title"));
assertEquals(new Long("123"), beans.get("ALong"));
assertEquals(new Integer("44"), beans.get("number"));
Expand Down
13 changes: 13 additions & 0 deletions core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,11 +733,24 @@ public void testSetNullList() {
assertEquals("Cat One", ((Cat) foo.getCats().get(0)).getName());
assertEquals("Cat Two", ((Cat) foo.getCats().get(1)).getName());

vs.setValue("annotatedCats[0].name", "Cat One By Annotation");
vs.setValue("annotatedCats[1].name", "Cat Two By Annotation");
assertNotNull(foo.getAnnotatedCats());
assertEquals(2, foo.getAnnotatedCats().size());
assertEquals("Cat One By Annotation", ((Cat) foo.getAnnotatedCats().get(0)).getName());
assertEquals("Cat Two By Annotation", ((Cat) foo.getAnnotatedCats().get(1)).getName());

vs.setValue("cats[0].foo.cats[1].name", "Deep null cat");
assertNotNull(((Cat) foo.getCats().get(0)).getFoo());
assertNotNull(((Cat) foo.getCats().get(0)).getFoo().getCats());
assertNotNull(((Cat) foo.getCats().get(0)).getFoo().getCats().get(1));
assertEquals("Deep null cat", ((Cat) ((Cat) foo.getCats().get(0)).getFoo().getCats().get(1)).getName());

vs.setValue("annotatedCats[0].foo.annotatedCats[1].name", "Deep null cat by annotation");
assertNotNull(((Cat) foo.getAnnotatedCats().get(0)).getFoo());
assertNotNull(((Cat) foo.getAnnotatedCats().get(0)).getFoo().getAnnotatedCats());
assertNotNull(((Cat) foo.getAnnotatedCats().get(0)).getFoo().getAnnotatedCats().get(1));
assertEquals("Deep null cat by annotation", ((Cat) ((Cat) foo.getAnnotatedCats().get(0)).getFoo().getAnnotatedCats().get(1)).getName());
}

public void testSetMultiple() {
Expand Down
39 changes: 38 additions & 1 deletion core/src/test/java/com/opensymphony/xwork2/ognl/SetPropertiesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ public void doTestAddingAndModifyingCollectionWithObjects(Collection barColl) {
Bar bar2 = new Bar();
bar2.setId(new Long(22));
barColl.add(bar2);
foo.setAnnotatedBarCollection(barColl);
//try modifying bar1 and bar2
//check the logs here to make sure
//the Map is being created
Expand All @@ -262,13 +263,31 @@ public void doTestAddingAndModifyingCollectionWithObjects(Collection barColl) {
assertEquals(bar1Title, next.getTitle());
}
}
Bar bar3 = new Bar();
bar3.setId(new Long(33));
barColl.add(bar3);
Bar bar4 = new Bar();
bar4.setId(new Long(44));
barColl.add(bar4);
String bar1TitleByAnnotation = "The Phantom Menace By Annotation";
String bar2TitleByAnnotation = "The Clone Wars By Annotation";
vs.setValue("annotatedBarCollection(44).title", bar2TitleByAnnotation);
vs.setValue("annotatedBarCollection(33).title", bar1TitleByAnnotation);
for (Object aBarColl : barColl) {
Bar next = (Bar) aBarColl;
if (next.getId().intValue() == 44) {
assertEquals(bar2TitleByAnnotation, next.getTitle());
} else if (next.getId().intValue() == 33) {
assertEquals(bar1TitleByAnnotation, next.getTitle());
}
}
//now test adding to a collection
String bar3Title = "Revenge of the Sith";
String bar4Title = "A New Hope";
vs.setValue("barCollection.makeNew[4].title", bar4Title, true);
vs.setValue("barCollection.makeNew[0].title", bar3Title, true);

assertEquals(4, barColl.size());
assertEquals(6, barColl.size());

for (Object aBarColl : barColl) {
Bar next = (Bar) aBarColl;
Expand All @@ -279,6 +298,24 @@ public void doTestAddingAndModifyingCollectionWithObjects(Collection barColl) {
}
}

//now test adding to a collection by annotation
String bar3TitleByAnnotation = "Revenge of the Sith By Annotation";
String bar4TitleByAnnotation = "A New Hope By Annotation";
vs.setValue("annotatedBarCollection.makeNew[5].title", bar4TitleByAnnotation, true);
vs.setValue("annotatedBarCollection.makeNew[1].title", bar3TitleByAnnotation, true);

assertEquals(8, barColl.size());

for (Object aBarColl : barColl) {
Bar next = (Bar) aBarColl;
if (next.getId() == null) {
assertNotNull(next.getTitle());
assertTrue(next.getTitle().equals(bar4TitleByAnnotation)
|| next.getTitle().equals(bar3TitleByAnnotation)
|| next.getTitle().equals(bar4Title)
|| next.getTitle().equals(bar3Title));
}
}
}
public void testAddingToCollectionBasedOnPermission() {
final MockObjectTypeDeterminer determiner = new MockObjectTypeDeterminer(Long.class,Bar.class,"id",true);
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/com/opensymphony/xwork2/test/annotations/PersonAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
converterClass=AddressTypeConverter.class),
@TypeConversion(type=ConversionType.APPLICATION,
key="com.opensymphony.xwork2.test.annotations.Person",
converterClass=PersonTypeConverter.class)})
converter="com.opensymphony.xwork2.test.annotations.PersonTypeConverter")})
public class PersonAction {
List<Person> users;
private List<Address> address;
Expand Down
25 changes: 25 additions & 0 deletions core/src/test/java/com/opensymphony/xwork2/util/Foo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
*/
package com.opensymphony.xwork2.util;

import com.opensymphony.xwork2.conversion.annotations.ConversionRule;
import com.opensymphony.xwork2.conversion.annotations.ConversionType;
import com.opensymphony.xwork2.conversion.annotations.TypeConversion;

import java.util.*;


Expand All @@ -34,9 +38,11 @@ public class Foo {
Date meeting;
Foo child;
List cats;
List annotatedCats;
List moreCats;
List strings;
Collection barCollection;
Collection annotatedBarCollection;
Map catMap;
Map anotherCatMap;
String title;
Expand Down Expand Up @@ -96,6 +102,15 @@ public List getCats() {
return cats;
}

public void setAnnotatedCats(List annotatedCats) {
this.annotatedCats = annotatedCats;
}

@TypeConversion(rule = ConversionRule.ELEMENT, converterClass = Cat.class)
public List getAnnotatedCats() {
return annotatedCats;
}

public void setChild(Foo child) {
this.child = child;
}
Expand Down Expand Up @@ -154,6 +169,16 @@ public void setBarCollection(Collection barCollection) {
this.barCollection = barCollection;
}

@TypeConversion(rule = ConversionRule.KEY_PROPERTY, value = "id")
public void setAnnotatedBarCollection(Collection annotatedBarCollection) {
this.annotatedBarCollection = annotatedBarCollection;
}

@TypeConversion(rule = ConversionRule.ELEMENT, converter = "com.opensymphony.xwork2.util.Bar")
public Collection getAnnotatedBarCollection() {
return annotatedBarCollection;
}

public void setPoints(long[] points) {
this.points = points;
}
Expand Down
Loading