You're correct. I have the setter part fixed too. If no one objects I'll file a JIRA issue and commit this today.
On Thu, Jul 30, 2009 at 11:54 PM, Cassie <[email protected]> wrote: > This looks great for the getter half, but doesn't the setter half need to > be > fixed too? > Otherwise, a getter may be called with too many parameters, right? > > - Cassie > > > On Thu, Jul 30, 2009 at 2:58 PM, Paul Lindner <[email protected]> wrote: > > > I think this should do it: > > diff --git > > a/java/common/src/main/java/org/apache/shindig/common/JsonUtil.java > > b/java/common/src/main/java/org/apache/shindig/common/JsonUtil.java > > index 9e392a8..b650178 100644 > > --- a/java/common/src/main/java/org/apache/shindig/common/JsonUtil.java > > +++ b/java/common/src/main/java/org/apache/shindig/common/JsonUtil.java > > @@ -84,9 +84,11 @@ public class JsonUtil { > > methods = Maps.newHashMap(); > > > > for (Method method : clazz.getMethods()) { > > - String name = getPropertyName(method); > > - if (name != null) { > > - methods.put(name, method); > > + if (method.getParameterTypes().length == 0) { > > + String name = getPropertyName(method); > > + if (name != null) { > > + methods.put(name, method); > > + } > > } > > } > > > > diff --git > > > > > a/java/common/src/test/java/org/apache/shindig/common/JsonSerializerTest.java > > > > > b/java/common/src/test/java/org/apache/shindig/common/JsonSerializerTest.java > > index 57145a5..7715d52 100644 > > --- > > > > > a/java/common/src/test/java/org/apache/shindig/common/JsonSerializerTest.java > > +++ > > > > > b/java/common/src/test/java/org/apache/shindig/common/JsonSerializerTest.java > > @@ -136,6 +136,10 @@ public class JsonSerializerTest { > > public Object getNullValue() { > > return null; > > } > > + @JsonProperty("simple!") > > + public void setSimpleName(int foo) { > > + > > + } > > } > > > > @Test > > > > > > On Thu, Jul 30, 2009 at 2:44 PM, Kevin Brown <[email protected]> wrote: > > > > > On Thu, Jul 30, 2009 at 2:29 PM, Paul Lindner <[email protected]> > wrote: > > > > > > > How important is it to support non-bean style getters/setter method > > > names? > > > > If it's not a problem then we can just check for get/set before > > checking > > > > for JsonProperty annotations. > > > > > > > > Another thing we could do is check the results of > > > Method#getParameterTypes > > > > or Method#getReturnType calls to insure we have something that looks > > like > > > a > > > > getter/setter > > > > > > > > > I think it's pretty trivial: > > > > > > - if it takes no arguments, it's a getter. > > > - if it does take arguments, it's a setter. > > > > > > looking at return types doesn't work because it can be ambiguous. > > Consider > > > this class: > > > > > > class Thingamjig { > > > > > > @JsonProperty("Foo") > > > Thingamajig setFoo(Foo foo) { > > > this.foo = foo; > > > return this; > > > } > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Jul 29, 2009 at 3:47 PM, Cassie <[email protected]> wrote: > > > > > > > > > That makes sense, but unfortunately, the code is already using it > in > > > > > deserialization... in BeanJsonConverter, line 89-119ish, the > > getSetter > > > > > method calls getPropertyName which uses the annotation. > > > > > > > > > > So, can this be safely changed to check for a different annotation > > > name, > > > > > like JsonSetterProperty, while leaving the serialization code the > > same? > > > > > Because as it stands, its sorta broken. If no one is supposed to be > > > using > > > > > it, then I suppose it shouldn't affect anyone. > > > > > > > > > > - Cassie > > > > > > > > > > > > > > > On Wed, Jul 29, 2009 at 3:39 PM, Kevin Brown <[email protected]> > > wrote: > > > > > > > > > > > JsonProperty was originally only added for serialization (as > > > indicated > > > > by > > > > > > the documentation). It could certainly be used for > deserialization, > > > but > > > > > the > > > > > > BeanJsonConverter and JsonSerializer both need to be updated > > > > accordingly. > > > > > > > > > > > > On Wed, Jul 29, 2009 at 3:28 PM, Cassie <[email protected]> wrote: > > > > > > > > > > > > > Hey everyone - > > > > > > > > > > > > > > I have some legacy objects in my code base that I wanted to > > convert > > > > to > > > > > > and > > > > > > > from json with the BeanJsonConverter and JsonProperty. Problem > > is, > > > it > > > > > > seems > > > > > > > that the same logic/annotation is being used to find getters > and > > > > > setters > > > > > > on > > > > > > > my pojos. So when I try to convert my pojo to a string, it ends > > up > > > > > trying > > > > > > > to > > > > > > > call setters, and not getters (or vice versa). > > > > > > > > > > > > > > For example, I have a class Post.java with two fields "title" > and > > > > > "body". > > > > > > > Add setters and getters for these fields and things work dandy. > > > > > However, > > > > > > I > > > > > > > need my json to use capital case: "Title" and "Body". To do > this > > I > > > > > figure > > > > > > I > > > > > > > need JsonProperty. So I go and add the JsonProperty("Title") > and > > > the > > > > > > other > > > > > > > annotation to the setters and who hoo I can take my json into a > > > pojo > > > > > with > > > > > > > capital case. But then - on the way from pojo to json the code > > > grabs > > > > > all > > > > > > > methods, and if they have a jsonProperty - it puts them in the > > > getter > > > > > > map! > > > > > > > Unfortunately, this causes an IllegalArgException because of > > course > > > > my > > > > > > > setters take params, and the code is expecting them to be > > > getters... > > > > > > > > > > > > > > So, I'm thinking we either need a JsonGetProperty and > > > JsonSetProperty > > > > > > > annotation. Or, the code needs to check that a property exists > > > -and- > > > > > that > > > > > > > the method starts with "get" or "set" or maybe there is > something > > > > > > better... > > > > > > > > > > > > > > Anyway, did I completely screw something up here? Is someone > > > getting > > > > > this > > > > > > > to > > > > > > > work correctly? > > > > > > > If not, how should we fix it? > > > > > > > Thanks! > > > > > > > > > > > > > > - Cassie > > > > > > > > > > > > > > > > > > > > > > > > > > > >
