Nested Properties proposition.
Benjamin Lerman
I have a patch of pectin that allows to use nested properties.
http://www.ambre.net/misc/gwt-pectin-nested-20100617.patch
The idea is:
# Have a factory (BeanModelProviderFactory) that is able to generate
BeanIntrospector given a Class (without using GWT.create)
# Have an annotation (@Beans) that have a parameter that takes a
number of Class
# Have a rebind strategy for my factory that
## explore all type in the oracle
## retrieve all those that are annotated with @Beans
## retrieve all parameters of thoses annotations (those are my beans
classes)
## create an implementation of BeanIntrospector for each of those
beans
## implements a factory method that takes a class as a parameter, and
returns the implementation of BeanIntrospector that corresponds if the
class is a parameter of the annotation somewhere.
Then, because you can retrieve a BeanIntrospector without going
through GWT.create, you can have working nested properties for all
type that are declared as parameter of the @Beans annotation.
I write a test (BeanModelProviderFactoryTest) that you can look out
to see how it is used, here an extract:
/* See the use of the BeanModelProviderFactory to retrieve a
BeanModelProvider without needing to declare an abstract class */
BeanModelProvider<TestBean> provider =
BeanModelProviderFactory.get().getBeanModelProvider(TestBean.class);
/* Here is a nested property */
MutableValueModel<String> stringValueModel =
provider.getValueModel("nestedBean.string", String.class);
TestBean tb = new TestBean();
AnotherBean ab = new AnotherBean();
ab.setString("value");
tb.setNestedBean(ab);
provider.setValue(tb);
/* Let's read some value */
assertEquals("value", stringValueModel.getValue());
/* then write some */
stringValueModel.setValue("hello");
provider.commit();
assertEquals("hello", ab.getString());
What do you thing of this.
Andrew: I had some problems because of the way you started to
implement nested properties. I'm handling the parsing of the nested
property in the BeanIntrospector (in NestedBeanIntrospector to be
exact), while you use PropertyKey for this. That means that I had to
returns a fake PropertyKey on my MutableValueModel that always act as
if the property was a simple one, and I let the NestedBeanIntrospector
do the parsing work.
Moreover, I might be stupid, but I do not see what is the point of
having a parameter in BeanIntrospector#getBeanAccessorForPropertyPath.
This parameter is only used for an exception on your implementation of
the generator, and it is quite confusing.
--
Benjamin
Andrew
Note to others, Benjamin and I have discussed this briefly over email
so appologies if some context has been lost.
key changes are (please correct me if I'm wrong):
1. Removes the need to annotate the beans themselves which is
important if you can't access the source or don't want to polute them
with a pectin dependency. It does this by scanning for any class
annotated with @Beans({BeanA.clas, BeanB.class}).
2. It solves the recursion problems of the current provider where a
nested bean returns an instance of the same type as itself. It does
so by processing property paths at run time.
3. Removes the need to use GWT.create.
4. Uses runtime property path parsing which both solves 2. but
probably (almost certainly) limits optimisation due to the use of
String.split (while noting that the current provider also suffers the
same fate although I think I can fix that).
The issues addressed by 1. and 2. are important and definitely need to
be fixed either way.
I'm not sure if optimisation losses mentioned in 4. are significant in
real world usage. My ideas to solve 2. may be worse from a usability
point of view than any optimisation loss (see following).
In a quick attempt at solving 1. and have used your idea and added
support for annotations on the BeanModelProvider class declaration.
i.e.
@NestedBeans({Address.class, ContactDetails.class})
public class PersonProvider extends BeanModelProvider<Person>
The above usage however won't automatically handle the recursion
problem in 2. (and I would need ensure the rebind generator gives you
an meaningful/useful RecusiveBeanPathException if this case occurs).
To handle the recursion issues I'd had been considering an annotation
as follows:
@NestedBeanPaths({"address", "contactDetails", "some.other.path"})
public class PersonProvider extends BeanModelProvider<Person>
This wouldn't suffer from the recursion problem but uses type-unsafe
strings for property names. It also might be a little confusing as
while it lists "address" it's actually making paths like
"address.postcode" etc available.
So in summary for me (at this stage):
* I think I prefer annotated providers since it keeps the beans that
the provider exposes with the provider declaration. i.e. if I'm using
the factory I'd need to go and find what/where to annotate additional
beans. The flip side is that you'll have to annotate every provider.
There may be some inbetween cas.
* If optimisation isn't an issue, or if people find the multiple
annotations too complex/confusing then your implementation is better
since it's simpler to use and the recursion problem just goes away.
* If optimisation is an issue, or if people want the @NestedBeanPath
option then the current generator mechanism is probably worth
pursuing.
I'm interested in how people feel about the addition of both
@NestedBean & @NestedBeanPaths. Is it harming the usability of the
API (i.e. mutliply ways of doing the same thing, multiple annotations,
annotations that don't work in certain situations)?
And in summary summary... I think the recursion problem and
optimisation are mutually exclusive, i.e. any optimisable solution
that uses only types to declare nested beans will have the recursion
problem. Happy to be proved wrong.
Any thoughts welcome.
> Andrew: I had some problems because of the way you started to
> implement nested properties. I'm handling the parsing of the nested
> property in the BeanIntrospector (in NestedBeanIntrospector to be
> exact), while you use PropertyKey for this. That means that I had to
> returns a fake PropertyKey on my MutableValueModel that always act as
> if the property was a simple one, and I let the NestedBeanIntrospector
> do the parsing work.
BeanPropertyValueModel go it only needs to provide the property name
and full path so a simple PropertyKey(String fullPath, String
property) constructor is probably all that is required. If I'm to fix
the optimisation issue then I'll have to move all the code out of it
anyway.
> Moreover, I might be stupid, but I do not see what is the point of
> having a parameter in BeanIntrospector#getBeanAccessorForPropertyPath.
> This parameter is only used for an exception on your implementation of
> the generator, and it is quite confusing.
so then yes the path is only used to generate context for the "you
found a pectin bug" exception. I think it's even more confusing
because it's the first parameter and it's poorly named. I'll change
the signature to something like `getAccessorByType(Class type, String
pathForDebugInfo)`.
Cheers
Andrew
Benjamin Lerman
> I'm still going through the patch but as I understand it so far the
> key changes are (please correct me if I'm wrong):
No corrections.
> In a quick attempt at solving 1. and have used your idea and added
> support for annotations on the BeanModelProvider class declaration.
> i.e.
>
> @NestedBeans({Address.class, ContactDetails.class})
> public class PersonProvider extends BeanModelProvider<Person>
>
> The above usage however won't automatically handle the recursion
> problem in 2. (and I would need ensure the rebind generator gives you
> an meaningful/useful RecusiveBeanPathException if this case occurs).
My problem with that is that it can get really verbose. For example, if
I have 3 object A, B, C with link from A to B and B to C, I have to
annotate the BeanModelProvider of A with B and C, and the
BeanModelProvider of B with C. And if I add something to C, I then might
have to go back to all those providers and add the new class.
> To handle the recursion issues I'd had been considering an annotation
> as follows:
>
> @NestedBeanPaths({"address", "contactDetails", "some.other.path"})
> public class PersonProvider extends BeanModelProvider<Person>
>
> This wouldn't suffer from the recursion problem but uses type-unsafe
> strings for property names. It also might be a little confusing as
> while it lists "address" it's actually making paths like
> "address.postcode" etc available.
This one seems very hard to maintain. One has to explicitly declares all
properties path one will be using. If one is reusing the same
BeanModelProvider in more than one place, one has to declare all the
nested beans one will use.
> So in summary for me (at this stage):
> * I think I prefer annotated providers since it keeps the beans that
> the provider exposes with the provider declaration. i.e. if I'm using
> the factory I'd need to go and find what/where to annotate additional
> beans. The flip side is that you'll have to annotate every provider.
> There may be some inbetween cas.
Well, I do prefer the inverse :) I can have a single place where I
declare all the Beans that are usable with pectin, and I have to do only
once. I can still create a class by bean if I wish, but it is not
required anymore.
> * If optimisation isn't an issue, or if people find the multiple
> annotations too complex/confusing then your implementation is better
> since it's simpler to use and the recursion problem just goes away.
>
> * If optimisation is an issue, or if people want the @NestedBeanPath
> option then the current generator mechanism is probably worth
> pursuing.
>
> I'm interested in how people feel about the addition of both
> @NestedBean & @NestedBeanPaths. Is it harming the usability of the
> API (i.e. mutliply ways of doing the same thing, multiple annotations,
> annotations that don't work in certain situations)?
I'd really like to know how well optimization is working.
On the another hand (and as you proposed by mail), maybe one solution
would be to have a clear API for the reflection part of pectin, and
allows multiple implementations.
> And in summary summary... I think the recursion problem and
> optimisation are mutually exclusive, i.e. any optimisable solution
> that uses only types to declare nested beans will have the recursion
> problem. Happy to be proved wrong.
Well, I agree with your point, but I think it is not the only case where
optimization will not work. As soon as the user is not using constant
strings for the property (if it is constructing those in a not trivial
way), then the optimization will go away.
> > Moreover, I might be stupid, but I do not see what is the point of
> > having a parameter in BeanIntrospector#getBeanAccessorForPropertyPath.
> > This parameter is only used for an exception on your implementation of
> > the generator, and it is quite confusing.
>
> I presume you mean `getAccessorByType(String path, Class type)`? If
> so then yes the path is only used to generate context for the "you
> found a pectin bug" exception. I think it's even more confusing
> because it's the first parameter and it's poorly named. I'll change
> the signature to something like `getAccessorByType(Class type, String
> pathForDebugInfo)`.
Sorry, I did read the generated implementation of TestBeanProvider and I
get a better understanding of the parameter. But, from an API point of
view, it is quite weird. If I understand correctly, the only reason of
this parameter, is for handling nested properties. Now let's say I have a
BeanIntrospector for Object A, that as a b property to Object B, that has
a string property.
I will consider an object aObject of type A and the "b.string" nested property.
What I have to do, manually, with the BeanIntrospector as it is now, is:
# call getBeanAccessorForPropertyPath("b").readProperty(aObject, "b") and
stock it on bObject (note that I have more or less to give the same
parameter "b" on the first and second call)
# call getBeanAccessorForPropertyPath("b.string").readProperty(bObject,
"string") to get my String. Once again I have to have a strong
correlation between my first and second calls (mainly the "string")
I'll be a lot more confortable using something like
getBeanAccessorForPropertyPath().readProperty("b.string", aObject)
It will allows
# to have getBeanAccessorForPropertyPath returns a cached object, because
it does not depends anymore on the usage afterwards
# to prevent all parsing of the properties outside of the
BeanIntrospector, and if you use your idea of declaring all nested
properties that will be used, it allows not to use any parsing inside
the method as well, so it will help optimization (if indeed GWT is able
to optimize this case, which I do not know)
--
Benjamin Lerman
Benjamin Lerman
Here a new version of the patch that add another functionality:
polymorphism
http://www.ambre.net/misc/gwt-pectin-nested-20100618.patch
This won't work yet with a BeanModelProvider, ony with the
BeanPropertyAccessor, but I'm pretty certain I can make it work.
There is 2 problems to have this work with a BeanModelProvider
1. BeanModelProvider#getValueModel do a type check using
getPropertyType, and getPropertyType does not work yet for properties
that only exist on a sub-class. One solution for that is to remove this
check (but that's quite inelegant, and won't solve the next problem), or
to have getPropertyType take into account the hierarchy of the beans and
returns a type as soon as all descendant of a bean agrees on it.
2. BeanModelProvider do send event with the old value of all declared
ValueModel as soon as one set the value, and this will fail the first
time, because the old value is null and one cannot find the real
implementation of a polymorphic attribute in that case. One again it can
be resolved by the same solution.
I'll try to implements a BeanIntrospector that know about the type
hierarchy to resolve thoss problems.
--
Benjamin Lerman
Andrew
> My problem with that is that it can get really verbose. For example, if
> I have 3 object A, B, C with link from A to B and B to C, I have to
> annotate the BeanModelProvider of A with B and C, and the
> BeanModelProvider of B with C. And if I add something to C, I then might
> have to go back to all those providers and add the new class.
that would let you annotate globally in one place while addressing
some of the optimisation issues I'm concerned about.
> > To handle the recursion issues I'd had been considering an annotation
> > as follows:
>
> > @NestedBeanPaths({"address", "contactDetails", "some.other.path"})
> > public class PersonProvider extends BeanModelProvider<Person>
>
> > This wouldn't suffer from the recursion problem but uses type-unsafe
> > strings for property names. It also might be a little confusing as
> > while it lists "address" it's actually making paths like
> > "address.postcode" etc available.
>
> This one seems very hard to maintain. One has to explicitly declares all
> properties path one will be using. If one is reusing the same
> BeanModelProvider in more than one place, one has to declare all the
> nested beans one will use.
nested beans. I.e. BeanModelProvider<Person> will expose all
properties of the Person, adding the annotation
@NestedPropertyPath("contactDetails") will expose all properties of
person and all those of contactDetails. I'm not a massive fan of this
either but I'm not yet ready to sacrifice the possibility of
optimisation. Also this is only required to solve the "a.a.a...a"
case, in all other cases you can use types. Once again I'm not
convinced either way yet.
> > So in summary for me (at this stage):
> > * I think I prefer annotated providers since it keeps the beans that
> > the provider exposes with the provider declaration. i.e. if I'm using
> > the factory I'd need to go and find what/where to annotate additional
> > beans. The flip side is that you'll have to annotate every provider.
> > There may be some inbetween cas.
>
> Well, I do prefer the inverse :) I can have a single place where I
> declare all the Beans that are usable with pectin, and I have to do only
> once. I can still create a class by bean if I wish, but it is not
> required anymore.
@Bean combined with no optimisation could lead to problems. For
example, I have the case where I have multiple entry points in the one
project. If each entry point uses different beans (which they do) and
the optimiser can't strip out the unused introspectors then each entry
point will be bloated with code it doesn't need (and as a developer I
would have no choice or controll over this).
Perhaps a solution that allows developer to limits the code created
while allowing annotations to be defined in one place would be to use
an annotated factory? E.g.
// create a base factory implememtation that
// develoers extend and annotate to create a concrete instance.
public class AbstractBeanModelProviderFactory {
public <T> BeanModelProvider<T> getBeanModelProvider(Class<T>
beanType)
{...}
public <T> BeanModelProvider<T> getBeanModelProvider(Class<T>
beanType, boolean autoCommit)
{...}
}
Then if you're happy with a global namespace for your bean
introspectors then you create the following for you project:
// all my annotiations are here, and I can use the static get() if I
want or inject it.
@Beans({BeanA.class, BeanB.class})
public class DefaultBeanModelProviderFactory extends
AbstractBeanModelProviderFactory {
private static BeanModelProviderFactory instance;
public static BeanModelProviderFactory get() {
if (instance == null) {
if (GWT.isClient()) {
instance =
GWT.create(DefaultBeanModelProviderFactory.class);
}
}
return instance;
}
}
To me this has a couple of advantages, for one the bean declarations
are always on the factory so there's only one place to look for them
(easier for developer consistency), developers can control the scope
of what introspectors will be created for a given entry point in a
single project and it also works nicely with GIN while allowing those
who prefer a static getters to use some thing like the snippet above.
> I'd really like to know how well optimization is working.
> On the another hand (and as you proposed by mail), maybe one solution
> would be to have a clear API for the reflection part of pectin, and
> allows multiple implementations.
works for both of us would be well worth it. For what it's worth
FormModel only relies on the ValueModelProvider and ListModelProvider
interface types so you can use what ever you want, however
implementing something like BeanModelProvider from scratch is somewhat
of an onerous task.
> > And in summary summary... I think the recursion problem and
> > optimisation are mutually exclusive, i.e. any optimisable solution
> > that uses only types to declare nested beans will have the recursion
> > problem. Happy to be proved wrong.
>
> Well, I agree with your point, but I think it is not the only case where
> optimization will not work. As soon as the user is not using constant
> strings for the property (if it is constructing those in a not trivial
> way), then the optimization will go away.
still be very nervous of ditching any/all optimisation because it may
not work in some cases.
> Sorry, I did read the generated implementation of TestBeanProvider and I
> get a better understanding of the parameter. But, from an API point of
> view, it is quite weird. If I understand correctly, the only reason of
> this parameter, is for handling nested properties. Now let's say I have a
> BeanIntrospector for Object A, that as a b property to Object B, that has
> a string property.
really expecting anyone to be using this code so I haven't tried too
hard to polish it.
The getAccessorForPropertyPath(String path) is designed to take a full
path such as 'a.b.c' and return an introspector for the object of type
'b'. From here the provider creates a BeanPropertyValueModel for
`a.b.c` using the ValueModel for the path 'a.b' and a
BeanPropertyAccessor for type b. Using unprocessed strings gives the
change for optimisation (but I introduced the PropertyKey bug during
the changes to support nested properties).
I'm going to make some changes to PropertyKey to fix the optimisation
issues, and change getAccessorForType(String,Class) etc and check
those in. I'll let you know when I've done that.
There are also a couple of basic principles I'm trying to follow as
well which are probably worth outlining when you're generating ideas
and patches:
* What I can't catch at compile time I try as much as possbile to
catch at construction. I.e. provider.getValueModel("a.b.c",
String.class) must fail if the property doesn't exist, is the wrong
type, wrong element type etc.
* Exceptions need to be meaningful and helpful. I.e.
BeanModelProviders know are given the full path of the property they
use purely for meaningful exception messages. That way when they
throw a ReadOnlyPropertyException the can say "a.b.c.e.date" was read
only. Saying only the property "date" was readonly is the type of
exception that causes pain.
Cheers
Andrew
might be worth looking into in terms of the implications from an
implementation perspective. Don't know if it will be discussed
further.
https://wave.google.com/wave/?nouacheck&pli=1#minimized:search,restored:wave:googlewave.com%252Fw%252ByAUzf_-lA
Benjamin Lerman
> @Bean combined with no optimisation could lead to problems. For
> example, I have the case where I have multiple entry points in the one
> project. If each entry point uses different beans (which they do) and
> the optimiser can't strip out the unused introspectors then each entry
> point will be bloated with code it doesn't need (and as a developer I
> would have no choice or controll over this).
That one I did not think about, and you are completely right.
> Perhaps a solution that allows developer to limits the code created
> while allowing annotations to be defined in one place would be to use
> an annotated factory?
That would be fine with me. I'll need to have the getBeanIntrospector
methods also, but this one is cheap to add.
> To me this has a couple of advantages, for one the bean declarations
> are always on the factory so there's only one place to look for them
> (easier for developer consistency), developers can control the scope
> of what introspectors will be created for a given entry point in a
> single project and it also works nicely with GIN while allowing those
> who prefer a static getters to use some thing like the snippet above.
Fine with me.
> I think extracting an AbstractBeanModelProvider or some such that
> works for both of us would be well worth it. For what it's worth
> FormModel only relies on the ValueModelProvider and ListModelProvider
> interface types so you can use what ever you want, however
> implementing something like BeanModelProvider from scratch is somewhat
> of an onerous task.
Agreed. My patch relied almost entirely on your code.
> That's a usecase I've never come across. Do you have an example? I'd
> still be very nervous of ditching any/all optimisation because it may
> not work in some cases.
I don't have a real use case just now, but you can look at the test I
wrote for inheritance. It did construct the path dynamically. I can
imagine other case when one has a Bean A with 2 properties b1 and b2 to
a bean B and one wants to factorize the code by using a method that
takes a String parameter than can be either b1 or b2.
> Yep, it's sort of one level out and is a bit confusing. I wasn't
> really expecting anyone to be using this code so I haven't tried too
> hard to polish it.
Sorry :)
> The getAccessorForPropertyPath(String path) is designed to take a full
> path such as 'a.b.c' and return an introspector for the object of type
> 'b'. From here the provider creates a BeanPropertyValueModel for
> `a.b.c` using the ValueModel for the path 'a.b' and a
> BeanPropertyAccessor for type b. Using unprocessed strings gives the
> change for optimisation (but I introduced the PropertyKey bug during
> the changes to support nested properties).
Well, you'll always need to process string with this approach, because
you have to compute the c from 'a.b.c'. You need to call something like:
getBeanAccessorForPropertyPath('a.b.c').readProperty(o, 'c')
the second 'c' is always computed in your case.
What you could do, is removing the parameter of
getBeanAccessorForPropertyPath and have:
getBeanAccessorForPropertyPath().readProperty(o, 'a.b.c.')
Then you won't have any computation, but the method readProperty can be
quite long. You have to consider all the possible paths (and forget
about recursion, but I think we agreed that recursion won't be possible
without computation). Now, if the optimization do cut all unused path
and inline all calls, you have something very good, if it doesn't, then
you have a monstrous if else if else... that is quite inefficient.
> I'm going to make some changes to PropertyKey to fix the optimisation
> issues, and change getAccessorForType(String,Class) etc and check
> those in. I'll let you know when I've done that.
Ok.
> There are also a couple of basic principles I'm trying to follow as
> well which are probably worth outlining when you're generating ideas
> and patches:
> * What I can't catch at compile time I try as much as possbile to
> catch at construction. I.e. provider.getValueModel("a.b.c",
> String.class) must fail if the property doesn't exist, is the wrong
> type, wrong element type etc.
That's a good principle in general, but I have a problem here. Maybe
you can tell me what would be a good solution for it.
In my second path, I write the test classes AbtractRootBean, SonBean1
and SonBean2 with SonBean1 and SonBean2 extending AbtractRootBean. I
wanted to be able to use pectin in the case of polymorphism. My problem
is that I cannot check the property without having a real bean, because
when I have a property of type AbtractRootBean, it can in fact be a
SonBean1 or a SonBean2. Would you consider acceptable to lose a little
bit of check at construction to allow those case to work?
My idea was to enhance the BeanIntrospector to take inheritance into
account. That means that if I have a hierarchy, and that a property "a"
exist for some element in the hierarchy (but always with a compatible
type), I will consider that "a" is a property of all the parents. I'm
not sure to be clear so there is an example:
class A {
}
class A1 extends A {
String a;
String b;
}
class A2 extends A {
int b;
}
class A21 extends A2 {
String a;
}
class A22 extends A2 {
}
Than, I will have the BeanIntrospector for A, A1, A2 and A21 accept "a"
as a property for all methods that do not take an actual object. But
A won't accept "b", because the type is not coherent in the hierarchy.
That will break you principle, because there would be way to fail later
than construction, but that would allow to use polymorphism in some
case.
> * Exceptions need to be meaningful and helpful. I.e.
> BeanModelProviders know are given the full path of the property they
> use purely for meaningful exception messages. That way when they
> throw a ReadOnlyPropertyException the can say "a.b.c.e.date" was read
> only. Saying only the property "date" was readonly is the type of
> exception that causes pain.
On this ,I agree 100%, and I have no problem with the extra parameter
in getAccessorByType (it is a private, generated, method so not part of
any API). My question was there because I had not understood how you
were using getBeanAccessorForPropertyPath.
--
Benjamin
Andrew
> > path such as 'a.b.c' and return an introspector for the object of type
> > 'b'. From here the provider creates a BeanPropertyValueModel for
> > `a.b.c` using the ValueModel for the path 'a.b' and a
> > BeanPropertyAccessor for type b. Using unprocessed strings gives the
> > change for optimisation (but I introduced the PropertyKey bug during
> > the changes to support nested properties).
>
> Well, you'll always need to process string with this approach, because
> you have to compute the c from 'a.b.c'. You need to call something like:
>
> getBeanAccessorForPropertyPath('a.b.c').readProperty(o, 'c')
>
> the second 'c' is always computed in your case.
>
> What you could do, is removing the parameter of
> getBeanAccessorForPropertyPath and have:
>
> getBeanAccessorForPropertyPath().readProperty(o, 'a.b.c.')
>
> Then you won't have any computation, but the method readProperty can be
> quite long. You have to consider all the possible paths (and forget
> about recursion, but I think we agreed that recursion won't be possible
> without computation). Now, if the optimization do cut all unused path
> and inline all calls, you have something very good, if it doesn't, then
> you have a monstrous if else if else... that is quite inefficient.
Hopefully the api will be usable by you too (let me know if it isn't).
The changes I've made are as follows:
* PropertyKey is now and interface. It has getters for the full path,
parent path and the property name. It also now has the isMutable()
method and methods to get the value type and the element type if it's
a collection property.
* BeanPropertyAccessor now has only the read and write methods, both
of which take the PropertKey.
* The BeanIntrospector interface now has two methods
getPropertyKey(String path) and getBeanPropertyAccessor(PropertyKey
key). The later method essentially uses the parent path of property
to get an appropriate accessor. If a PropertyKey.getParentBeanType()
would be useful for you then I could add that too.
Other changes I'm thinking of:
* I'm thinking of extracting an AbstractBeanModelProvider (that's
essentially the same as the current BeanModelProvider) so you only
have to implement the two BeanIntrospector methods.
* The new BeanModelProvider implementation will be essentially emtpy
and only be used to trigger the rebind process (instead of
BeanIntrospector). That way you can extend AbstractBeanModelProvider
and rebind how ever you want.
As for optimisation, all of my PropertyKey implementations are
generated by the rebind process and use immutable values. I.e.
nothing is computed so hopefully it should all optimise away.
As for the recursion problem, if the optimistation actually works I
was thinking using a an annotation like @MaxNestedDepth(5) or
@MaxNestedDepth(type=NestedBean.class, depth=5) so the rebind process
would only generate paths to the specified depth. Recusive paths
without a maxDepth annotation would cause the rebind process to throw
an Exception. Anyway, this is all arm waving till is see if
optimisation works, and there's always the case that the property
names passed to the provider are computed, at which point all
optimisation is lost...
And of course if optimisation doesn't add value then you're approach
is nicer....
I've also been thinking about adding support for DTOs with public
properties. This would remove the need for getters/setters where they
do nothing except set the field.
This will all take me a bit of time to do especially since I'm in the
middle of migrating another project over to hiberate and it's taking
some time.
> > There are also a couple of basic principles I'm trying to follow as
> > well which are probably worth outlining when you're generating ideas
> > and patches:
> > * What I can't catch at compile time I try as much as possbile to
> > catch at construction. I.e. provider.getValueModel("a.b.c",
> > String.class) must fail if the property doesn't exist, is the wrong
> > type, wrong element type etc.
>
> That's a good principle in general, but I have a problem here. Maybe
> you can tell me what would be a good solution for it.
>
> In my second path, I write the test classes AbtractRootBean, SonBean1
> and SonBean2 with SonBean1 and SonBean2 extending AbtractRootBean. I
> wanted to be able to use pectin in the case of polymorphism. My problem
> is that I cannot check the property without having a real bean, because
> when I have a property of type AbtractRootBean, it can in fact be a
> SonBean1 or a SonBean2. Would you consider acceptable to lose a little
> bit of check at construction to allow those case to work?
test that constructs the form asserts that all the properties are
correct. This doesn't mean there can't be two implementations, a
static one (the current) and a dynamic one (yours). There would just
be trade offs between the two and other developers could choose the
approach that works best for them.
There may be other ways to solve the type safety issue as well e.g.
code generation, annotated fields with paths in the providers that get
generated during rebind etc. I'm also waiting to see where ValueStore
goes as well.
There are also a couple of issues involved in putting in the two
provider implementations in pectin, one is consistency between the two
styles, the other is my maintenence load (c: In the later a seperate
contrib style project (gwt-pectin-contrib?) that others maintain to
might be worth while. Once the ideas stabilise or are just generally
better they can be pulled into the main project.
Cheers
Benjamin Lerman
> Hopefully the api will be usable by you too (let me know if it isn't).
Andrew
update the unit tests. I'll let you know when I've checked it in.
Andrew
Just a quick status update, I've made the changes (not checked in yet)
but I'm not that happy with the implementation as I ended up with a
Path object, PropertyKey and BeanPropertyAccessor all of which overlap
somewhat. I'm going to have another look at it.
Also in a brief check of the compiler output there's some if/else that
aren't being optimised away, I've only looked briefly at it so I can't
say for sure how much is an isn't being optimised.
Cheers
Andrew
are:
* Moved all the BeanModelProvider functionality to
AbstractBeanModelProvider. Rebinding is still triggered by
BeanModelProvider.
* Replaced the various accessor/introspector interfaces with a single
PropertyDescriptor interface accessed by a single abstract method
`getPropertyDescriptor(String path)` on AbstractBeanModelProvider.
* One PropertyDescriptor is created per unique path and defines
methods such as getValueType(), isMutable(), readValue(..) and
writeValue(..) etc.
The new rebind implementation uses only one layer of if/else's and
after a brief check the optimisted output has all the un-used
properties stripped (only the paths used by boundTo(...) calls are
present in the output). I haven't had a chance to see how much
inlining is done appart from that.
Anyway, I hope the new implementation is easier to understand (with
the parent path stuff, check out ReflectionBeanModelProvider for a
runtime implementation example).
It's not quite finished yet but I'm much happier with the use of the
AbstractBeanModelProvider/PropertyDescriptor combination and I don't
expect that part of the API to change (althought I'm not overly happy
with the PropertyDescriptor name).
If you have any hassles with it let me know.
Cheers
Benjamin Lerman
> Anyway, I hope the new implementation is easier to understand (with
> the parent path stuff, check out ReflectionBeanModelProvider for a
> runtime implementation example).
>
> It's not quite finished yet but I'm much happier with the use of the
> AbstractBeanModelProvider/PropertyDescriptor combination and I don't
> expect that part of the API to change (althought I'm not overly happy
> with the PropertyDescriptor name).
>
> If you have any hassles with it let me know.
I redid my patch, you can find it there:
http://www.ambre.net/misc/gwt-pectin-nested-20100707.patch
What I'd like to add is:
* Follow your suggestion, and be able to generate a Factory from a given
class that will be annotated, instead of looking for all existing
annotations to handle the case of multiple entry points.
* Rework on the generator to be able to handle polymorphism.
Unfortunately, I might not be able to do that before at least a full
week.
Now, I have a question. Having a BeanModelProvider, a bean and a
nested property. I have to write the following method to be able to read/write
the property:
<code>
public <T> Object readProperty(BeanModelProvider<T> provider, T bean, String path) {
PropertyDescriptor propertyDescriptor = provider.getPropertyDescriptor(path);
if(propertyDescriptor.getParentPath() == null) {
return propertyDescriptor.readProperty(bean);
} else {
Object subBean = readProperty(provider, bean, propertyDescriptor.getParentPath());
return propertyDescriptor.readProperty(subBean);
}
}
public <T> void writeProperty(BeanModelProvider<T> provider, T bean, String path, Object value) {
PropertyDescriptor propertyDescriptor = provider.getPropertyDescriptor(path);
if(propertyDescriptor.getParentPath() == null) {
propertyDescriptor.writeProperty(bean, value);
} else {
Object subBean = readProperty(provider, bean, propertyDescriptor.getParentPath());
propertyDescriptor.writeProperty(subBean, value);
}
}
</code>
Would'nt it be simple to be able to do:
<code>
provider.getPropertyDescriptor(path).readProperty(bean)
</code>
even with nested properties. I'm pretty certain that your generator
needs only a very few modification to be able to work like that. So is
there a reason why you want to keep using parentPath and recursion,
instead of having all the logic in the generated PropertyDescriptor
Cheers,
--
Benjamin
Andrew
> I redid my patch, you can find it there:
> http://www.ambre.net/misc/gwt-pectin-nested-20100707.patch
>
> What I'd like to add is:
> * Follow your suggestion, and be able to generate a Factory from a given
> class that will be annotated, instead of looking for all existing
> annotations to handle the case of multiple entry points.
> * Rework on the generator to be able to handle polymorphism.
>
> Unfortunately, I might not be able to do that before at least a full
> week.
so it would help me if you moved your implementation to a distinct
package, perhaps under `com...client.bean.dynamic` perhaps? It will
also need to be a server side reflection implementation along with
some wiki documentation, but hold off on that till I'm closer to
making a decision on what I want to do.
I also need to extract the test classes (i.e. the test beans etc) out
into a new source tree/sub-project so a common data structure can be
used by both the genreated and reflection based providers (till now
it's been an evil cut & paste). This is especially important if I add
your implementation to trunk. This will take a while too and I'm not
sure when I'll get onto it.
implementations is quite different from what you posted, the recursion
happens in the creation of the property descriptor and not in the read/
write methods. I think there may be some confustion as
`AbstractBeanModelProvider.getPropertetyDescriptor(String path)` and
should really be called `PropertyDescriptor
createPropertyDescriptor(String path)`. It's a factory method and not
a getter and is only ever called once to create a PropertyDescriptor
for a particular path. The desciptor is then used by the
BeanPropertyValueModel in the manner you described, i.e.
BeanPropertyValueModel.get/setValue(..) eventually calls
propertyDescriptor.readProperty(bean) and
propertyDescriptor.writeProperty(bean, value).
So in short any implementation of AbstractBeanModelProvder only needs
to implement the single method (soon to be named) `PropertyDescriptor
createPropertyDescriptor(String propertyPath)`. The implementation of
that method and the PropertyDescriptor returned can be anything you
like.
Cheers
Benjamin Lerman
> That's fine. Were you still hoping to get it accepted into trunk? If
> so it would help me if you moved your implementation to a distinct
> package, perhaps under `com...client.bean.dynamic` perhaps? It will
> also need to be a server side reflection implementation along with
> some wiki documentation, but hold off on that till I'm closer to
> making a decision on what I want to do.
>
> I also need to extract the test classes (i.e. the test beans etc) out
> into a new source tree/sub-project so a common data structure can be
> used by both the genreated and reflection based providers (till now
> it's been an evil cut & paste). This is especially important if I add
> your implementation to trunk. This will take a while too and I'm not
> sure when I'll get onto it.
Yes, I still hope to have it accept. If not, I probably try to have a
dedicated project with it, but I'll go this path only if you do not want
it in trunk.
Sorry, I wasn't clear.
I know you do not use the code I posted, it is just an excerpt of what
one must do with PropertyDescriptor to read/write a nested property.
In pectin you do not do it directly like that because you have a tree
of ValueSource that are created to get the list of PropertyDescriptor,
and then each one call the other to do the recursion I showed.
What I do not understand is, why is this necessary. You could modify
the PropertyDescritor so that, for whichever path that it can use, the
PropertyDescritor for this path is sufficient to read/write the value.
Is there a reason for you to have written your API this way.
If you need, I think I can write a patch that show you what I mean. It
should simplify your code, because you won't have to consider if a path
is nested or not anywhere but in the generator.
--
Benjamin
Andrew
> dedicated project with it, but I'll go this path only if you do not want
> it in trunk.
point. That way people won't send me bug fixes and feature requests
and our release cycles can remain independant (once we sort out the
current api issues I expect things to be pretty stable for you to code
against). I'll add link to you implementation on the wiki with a list
of features you want to highlight. I'm still very open to including
it in the future once it's stablised and especially if others find it
useful.
> Sorry, I wasn't clear.
>
> I know you do not use the code I posted, it is just an excerpt of what
> one must do with PropertyDescriptor to read/write a nested property.
>
> In pectin you do not do it directly like that because you have a tree
> of ValueSource that are created to get the list of PropertyDescriptor,
> and then each one call the other to do the recursion I showed.
>
> What I do not understand is, why is this necessary. You could modify
> the PropertyDescritor so that, for whichever path that it can use, the
> PropertyDescritor for this path is sufficient to read/write the value.
> Is there a reason for you to have written your API this way.
doing if you think your approach is better then you can send me a
patch.
The BeanModelProvider creates a graph of ValueModels that matches the
property paths that are used. So for a heirarchy of type `A` with a
property 'b' of type `B` and nested proeprty 'c' of type `C` the
following ValueModel heirarchy will be created within the provider:
// the code
BeanModelProvider<A> provider = ...;
BeanPropertyValueModel<C> cModel = provider.getValueModel("b.c",
C.class);
// the ValueModel structure created by the provider
VM<A> // the root value model in the provider
|
+-BPVM<B> using a PropertyDescriptor(beanType=A,
| valueType=B,
| parentPath=null)
|
+-BPVM<C> using PropertyDescriptor(beanType=B,valueType=C)
valueType=B,
parentPath='b')
The reason for this heirarchy is to ensure that a call to
`provider.getValueModel("b", B.class).setVaue(new B())` will cause
cModel update automatically (i.e. fire value change events).
My reasoning (for better or worse) for the PropertyDescriptor is that
for the path 'b.c' it knows how to:
a) read and write property 'c' on a bean of type `B`
b) knows it's parent path and parent bean type so that the provider
get use it to create the parent ValueModel.
c) The parent path and bean type etc are constants so they can be
optimised away if not used. I still need to check this in more detail
but my initial checks seem to strip all unused properties references
from the generated code.
d) Because of b + c above each PropertyDescriptor is unique for given
path.
> If you need, I think I can write a patch that show you what I mean. It
> should simplify your code, because you won't have to consider if a path
> is nested or not anywhere but in the generator.
Benjamin Lerman
> Cool, I think a separate project would be preferable for me at this
> point. That way people won't send me bug fixes and feature requests
> and our release cycles can remain independant (once we sort out the
> current api issues I expect things to be pretty stable for you to code
> against). I'll add link to you implementation on the wiki with a list
> of features you want to highlight. I'm still very open to including
> it in the future once it's stablised and especially if others find it
> useful.
If I create a separate project, I'll need an API to call on your
generators.
Specifically, I added a method
public String createBeanDescriptor(JClassType beanType)
in BeanModelProviderCreator that calls your generateDescriptorVars and
generateGetPropertyDescriptorMethod methods.
Adding the createBeanDescriptor in your generator class does not seem
to be the right move :). I don't know if you can make the 2 generate
methods public, or if we need to find a better way to extract an API
from this.
> I'm still not sure what you mean, so I'll try and outline what I'm
> doing if you think your approach is better then you can send me a
> patch.
Thanks for the explanation. I did find out while writing my patch why
you need the parent path (mainly to be able to change the mutable state
of your properties if an intermediary object is null) and so my idea
won't work at all.
On another subject, I'm still trying to find out if it is possible to do
any polymorphism. Did you already think about it?
--
Benjamin
Andrew
> If I create a separate project, I'll need an API to call on your
> generators.
>
> Specifically, I added a method
>
> public String createBeanDescriptor(JClassType beanType)
>
> in BeanModelProviderCreator that calls your generateDescriptorVars and
> generateGetPropertyDescriptorMethod methods.
>
> Adding the createBeanDescriptor in your generator class does not seem
> to be the right move :). I don't know if you can make the 2 generate
> methods public, or if we need to find a better way to extract an API
> from this.
AbstractBeanModelProvider.createPropertyDescriptor(...) and the rest
was up to you (c:
My generator (including the BeanInfo/PropertyInfo classes) is tied
pretty tightly to "how I'm doing it", this is even more the case now
that I'm working on limiting nested property recursion (not quite
checked in yet).
> On another subject, I'm still trying to find out if it is possible to do
> any polymorphism. Did you already think about it?