Designing a fluent interface (and support for different collection types)

[Paul Batum]

So I wanted to implement support for the different collection types. I
was aiming for just set and bag for my first commit, as they don't
require the additional 'index' and 'element' xml elements. I wanted to
support it in two different forms:

var classMap = new ClassMap<OneToManyTarget>();
classMap.HasMany<ChildObject>(x => x.SetOfChildren)
.IsSet();

And also:

var classMap = new ClassMap<OneToManyTarget>();
classMap.HasSet(x => x.SetOfChildren);

Now I've run into a bit of a conflict with Andy's change (r9) where he
added a HasMany overload that accepts the foreign key column name. My
issue is now I will have to overload HasSet and HasBag aswell. Same
goes for HasMap, HasList, etc.

Now my question, is really a general question about fluent interface
design. I can see 3 options here and I have no idea which one would be
preferred:

1. Don't provide concise methods such as HasSet
2. Don't provide a HasMany overload that takes the column name,
instead set the column name using a fluent interface, such as:

var classMap = new ClassMap<OneToManyTarget>();
classMap.HasMany<ChildObject>(x => x.SetOfChildren)
.TheKeyColumnIs("ParentID");

3. Stop stressin' so much, let the methods and overloads proliferate!

Really interested to hear your thoughts on this one, because I suspect
its going to come up again and again as we flesh Fluent NHibernate
out. I need guidelines!

[Andrew Stewart]

Hello

I think I agree with number 3 ;o) only joking

I quite like this one

var classMap = new ClassMap<OneToManyTarget>();

classMap.HasMany<ChildObject>(x => x.SetOfChildren).ForeignKey("ParentID");

Then it's optional and keeps the api simpler, although slightly longer.

Andy

--
=================
I-nnovate Software - Bespoke Software Development, uk wirral.
http://www.i-nnovate.net

[John Teague]

In general I feel that when creating a fluent interface required fields are paramters and all optional are added through chaining.

[James Gregory]

I agree as well. Shortcuts are nice, but they reduce the redability of the interface.

[Andrew Stewart]

That and I think we want to encourage convention of some form accross the database, so you only deviate from that when you absolutly have to.

 

Andy

[Paul Batum]

So just to make sure I'm not misunderstanding.. You don't want this:

classMap.HasSet(x => x.SetOfChildren);

?

[Andrew Stewart]

Good point actually if we're going to go for a convention should all relationships be mapped the same unless otherwise specified so it would be something like below:

 

classMap.HasMany(x => x.SetOfChildren);

 

classMap.HasMany(x => x.SetOfChildren)

              .Type(ManyType.Bag)

              .ForeignKey("PersonId");

Or am I just confusing the matter.

 

Andy

 

On Thu, Jul 31, 2008 at 9:29 AM, Paul Batum <paul....@gmail.com> wrote:

So just to make sure I'm not misunderstanding.. You don't want this:

classMap.HasSet(x => x.SetOfChildren);

?

[James Gregory]

I'm not too keen on just HasSet, because it isn't clear what kind of relationship it is.

I like Andrew's example, with one change:

classMap.HasMany(x => x.SetOfChildren)
    .AsBag()

Or something to that effect. Basically, I'd like to avoid using enums where possible, as I think they reduce the fluency of the interface by adding extra duplication.

classMap.HasMany(x => x.SetOfChildren)
    .Type(ManyType.Bag)

[Paul Batum]

On Thu, Jul 31, 2008 at 9:45 AM, James Gregory <jagreg...@gmail.com> wrote:
> I'm not too keen on just HasSet, because it isn't clear what kind of
> relationship it is.

I expected there might be arguments about the idea of HasSet, which is
part of why I floated the idea rather than just commiting, but this
isn't one I was expecting.
Isn't it obvious that the relationship type is Set? I must be missing something.

[James Gregory]

It's obvious what type of collection it is, yes, but the relationship is only implied. Its through our existing knowledge of how NHibernate works that we know a Set is a one-to-many. If somebody isn't versed in the theory behind collections in NHibernate, they may not know what a Bag is specifically, but HasMany is clear regardless of it's type.

Just having HasMany keeps the API simpler too.

On Thu, Jul 31, 2008 at 1:03 PM, Paul Batum <paul....@gmail.com> wrote:

[James Gregory]

Should've been Set, not Bag there.

[Chad Myers]

How 'bout something like this?

Sets (of values)
---------------------
.HasElementsOf<Child>(x=>x.SetOfChildren,m=>m
.AsSet()
.KeyedTo(c=>c.ParentID)
.ElementIs(c=>c.Description)
.OrderAscendingBy(c=>c.Size)
)

.HasElementsOf<Child>(x=>x.SetOfChildren,m=>m
.AsSet()
.KeyedTo(c=>c.ParentID)
.CompositeElementOf<Contact>(
c=>c.Type,
c=>c.Description)
)

Sets (bi-directional)
---------------------
.HasMany<Child>(x=>x.SetOfChildren, m=>m
.AsSet()
.KeyedTo(c=>c.ParentID)
)

Bags (of values)
---------------------
.HasMany<Child>(x=>x.BagOfChildren, m=>
.AsBag()
.KeyedTo(c=>c.ParentID)
.ElementIs(c=>c.Description)
)

Bags (bi-directional)
---------------------
.HasMany<Child>(x=>x.BagOfChildren, m=>
.AsBag()
.KeyedTo(c=>c.ParentID)
.CascadeAll()
)

[Matt Hinze]

Yeah.. And it infers defaults based on type.  ISet, set; IList, bag; IDictionary, map, etc.

support for composite-elements and nested-composite-element is another thing on my wish list (haven't added to the issue tracker yet)..

[Paul Batum]

On Thu, Jul 31, 2008 at 1:18 PM, James Gregory <jagreg...@gmail.com> wrote:
> It's obvious what type of collection it is, yes, but the relationship is
> only implied. Its through our existing knowledge of how NHibernate works
> that we know a Set is a one-to-many. If somebody isn't versed in the theory
> behind collections in NHibernate, they may not know what a Bag is
> specifically, but HasMany is clear regardless of it's type.

Ahh, now I follow. Part of the reason I was fond of HasSet is that I
could customise the signature so that the child type would be
inferred. But I see your point.

Chad, have you already got an implementation using that api? If not I
am happy to implement it. But what's the m => m for?

[Paul Batum]

Looking at it again, I can see that the fluent interface is chaining
off the m parameter (presumably m stands for 'mapping'), rather than
the return type of the HasMany. But I guess my question still stands,
why the extra lambda parameter, why not just chain off the method
return?

I'm also not following the difference between HasElementsOf and HasMany.

[Chad Myers]

First, I do not already have an impl of that API, but I don't think it would be that hard.

The reason I used the m=m.Blah.Blah.Blah is to prevent FI dead-ending and to keep all the "collection-y" stuff in one expression so that the rest of the mapping FI stays on it's intended path.

"m" in this context would be an existing or new object that knows all about collection mappings (sets, bags, etc) and has a specialized interface/API just for dealing with them so you don't pollute the rest of the API.

HasElementsOf is for element collections (i.e. a collection of all the descriptions off the Customer object, as opposed to a bi-di collection of actual Customer entities). From my experience, element collection mappings are rare so it's probably not worth the time to do anything with them. I was following along in the NHib docs and showing what it might look like if you did need to map it.

-c

________________________________

[Paul Batum]

The current implementation of HasMany returns a OneToManyPart so the
FI is already at a dead end. I see what you're after though, you'd
like:

var classMap = new ClassMap<Parent>()
.HasMany<Child>(x => x.Children, m => m
.AsBag()
.Etc())
.Map(x => x.Name)
.Map(x => x.StartDate)
.Map(x => x.Etc);

This requires quite a few changes as other methods such as Map would
also have to chain off a lambda parameter and return the classmap.
Certainly possible, but too much for me to tackle in one go.

I will see how far I can get tonight.

[Chad Myers]

Yeah, forgot about that. Well, whatever you think is prudent, I was just brainstorming. The actual impl will probably be quiet different.

Let me know how I can help.

[Paul Batum]

Ok I just commited basic support for lists, bags and sets. I decided to make use of Chad's trick of chaining off a lambda parameter for specifying the index properties for lists. I'm not sure if this was appropriate or if the implementation is acceptable so I'm keen for some feedback on that (see OneToManyTester.cs and OneToManyPart.cs).

Andrew, I removed the HasMany overload that takes a column as we discussed, so I guess this change will break your project. Just a heads up.

One thing that makes me uncomfortable is that there is no test where the generated xml is actually being pumped to NHibernate. Personally I would feel a lot more confident if I could write some tests to do this. I know said tests would be more 'integration' style tests than unit tests but it would still make me feel better. I was wondering if anyone felt the same way, and if so, what the preferred way of pumping the generated xml into NHibernate would be?

[Bobby Johnson]

I Paul can you also commit OneToManyTester as well? Unit tests are currently failing to build.

--
"The explanation requiring the fewest assumptions is most likely to be correct."

- Occam's Razor
http://en.wikipedia.org/wiki/Occam's_Razor

[Matt Hinze]

Looks like there are some things in ConnectedTester but it's too
StructureMappy to be useful in general. This is a 5 minute
hack/brainstorm/yada, but you can do something like this:

[TestFixture]
public class NHibernateTester
{
[Test, Explicit]
public void CanCompileSimplestMapping()
{
var map = new ClassMap<MappedEntity>();
map.Map(e => e.Name, "Name");
map.Id(e => e.Id, "Id");
XmlDocument xml = map.CreateMapping(new MappingVisitor());

IDictionary<string, string> props = getProps();

var config = new Configuration();
config.AddProperties(props);
config.AddXml(xml.InnerXml);

config.BuildSessionFactory();

var exporter = new SchemaExport(config);
exporter.Create(true, false);

// do whatever tests here I guess
config.ClassMappings.Count.ShouldEqual(1);
}

private static IDictionary<string, string> getProps()
{
IDictionary<string, string> props = new Dictionary<string,
string>();
props.Add("connection.provider",
"NHibernate.Connection.DriverConnectionProvider");
props.Add("connection.driver_class",
"NHibernate.Driver.SqlClientDriver");
props.Add("dialect", "NHibernate.Dialect.MsSql2000Dialect");
props.Add("hibernate.dialect",
"NHibernate.Dialect.MsSql2000Dialect");
props.Add("use_outer_join", "true");
props.Add("connection.connection_string",
"Data Source=.;Initial
Catalog=ShadeTree;Integrated Security=True;Pooling=False");
//props.Add("show_sql", showSql);
props.Add("show_sql", true.ToString());
return props;
}
}

public class MappedEntity
{
public virtual int Id { get; set; }
public virtual string Name { get; set; }
}

and get a nice, friendly (and slow) output of:

------ Test started: Assembly: FluentNHibernate.Testing.dll ------

if exists (select * from dbo.sysobjects where id =
object_id(N'MappedEntity') and OBJECTPROPERTY(id, N'IsUserTable') = 1)
drop table MappedEntity
create table MappedEntity (
Id INT IDENTITY NOT NULL,
Name NVARCHAR(100) null,
primary key (Id)
)

1 passed, 0 failed, 0 skipped, took 3.22 seconds.

>>>> On Thu, Jul 31, 2008 at 1:18 PM, James Gregory <jagreg...@gmail.com>

[James Gregory]

Guys, I'm not so sure about this extra lambda parameter. Don't get me wrong, it's very cool, but is it nessecary? It's not something I've seen used before, and I think it complicates the API. I appareciate that it solves dead-ending, but is that actually a problem? As someone else said, it already happens elsewhere and it hasn't been a problem yet.

Mainly, I think we need to pick a method and stick with it. If we're going to use the extra lambda, then we need to make the rest of the API like it, or we shouldn't use it at all (unless really nessecary). Otherwise we're going to end up with an inconsistent, unobvious, API.

I'm thinking it might be worth writing a small "style guide", so we can all refer to it while writing new features, hopefully that'd help us keep it neat.

>>>> On Thu, Jul 31, 2008 at 1:18 PM, James Gregory <jagregory.com@gmail.com>

[Paul Batum]

Sorry about the broken build, I'm spoiled by continuous integration. I think I need a 'svn for dummies' book or something, I'm used to the checkout/checkin model. I thought svn would automatically track newly added files? In any case, I've commited the missing file.

James, I'm happy to drop the extra lambda for the index. What would you suggest instead?

[James Gregory]

Hmm, after looking at your tests for how it's actually used, I think it might be alright as it is. Our other option is to have the AsList always be the last method, because it terminates. I think that's a worse idea though, it's certainly less usable.

I'm mainly concerned with readability and usability. I think Chad's example bothered me with the multiple lambdas in the method signature, but the single index one seems reasonable.

Basically, ignore me :)

[Paul Batum]

Hey Matt,

Yeah thats roughly what I was talking about. I don't really feel compelled to generate the schema, as long as the session factory builds I think I'm happy. I agree that the connected tester looks like its sort of what we want but it would need some work.. Did you look at using the PersistanceModel class at all?

On Fri, Aug 1, 2008 at 3:45 AM, Matt Hinze <mhi...@gmail.com> wrote:

>>>> On Thu, Jul 31, 2008 at 1:18 PM, James Gregory <jagregory.com@gmail.com>

[Matt Hinze]

That's it. Terminating methods and a builder hierarchy are sort of
ugly. Not to mention cumbersome to design (although it may eventually
come down to that). Not that all that lambda "syntactical tar" is
super clean, but I think I like it better also.

[Chad Myers]

You only need up to the point of BuildSessionFactory. At that point,
NHibernate will start complaining about bad mappings.

As far as mappings that appear to be correct, but will blow you, you
need to:
a.) gen the DB using BuildSchema from SessionSource
b.) use the PersistenceSpecification to save entities, load them back up
and do field-by-field comparison

-c

[Matt Hinze]

Here's a tiny start.

[Chad Myers]

I agree about dropping the inner-lambda. I forgot that the mapping FI already had dead-ends, so it’s cool.

 

Sorry about the confusion.

 

-c

[James Gregory]

Chad: I want to point out now this is not a criticism of your suggestion, because it's perfectly valid. You've just raised a design issue that I think we need to discuss before we progress.

I've been thinking about the lambda expressions and terminating some more, and I've come to a conclusion.

1. Properties and other class-level mappings should be terminating

We shouldn't complicate the interface by allowing that kind of reverse navigation. While initially expressive, when a class grows the mapping expression will get more complicated. I'm of the opinion that long running fluent statements can complicate things more than they aid, so I'd prefer if it were broken into logical groups.

I'll try to illustrate...

class

    .Map(x => x.Name)

    .Map(x => x.DateOfBirth)

    .Map(x => x.Gender)

    .HasMany<Person>(x => x.Siblings)

    .HasMany<Animal>(x => x.Pets);

That's a nice example of when chaining properties would be a nice feature; however, I don't think it scales.

class

    .Map(x => x.Name, m => m

        .WithColumn("PersonName")

        .Access.AsLowerCaseField())

    .Map(x => x.DateOfBirth)

    .HasMany<Person>(x => x.Siblings, m => m

        .AsList(index => index

            .WithColumn("Index"))

        .Access.AsField())

    .HasMany<Animal>(x => x.Pets, m => m

        .AsBag());

By adding more customisations to the mappings, this has introduced more complexity to the interface because we've had to add the m lambda to allow reverses navigation. If this were to be expanded further with a few more properties, I'd see it quickly becoming less readable.

For the sake of sacrificing a little bit of fluency, and adding a few more characters, I think this is easier:

class.Map(x => x.Name)

    .WithColumn("PersonName")

    .Access.AsLowerCaseField();

class.Map(x => x.DateOfBirth);

class.HasMany<Person>(x => x.Siblings)

    .AsList(index => index

        .WithColumn("Index"))

    .Access.AsField();

class.HasMany<Animal>(x => x.Pets)

    .AsBag();

Without the need to traverse back to the class, we don't need the m lambda, and this keeps the interface cleaner.

2. Anything within a class should not be terminating

So properties, relationships and what not should always return themselves. This is the logical group I mentioned, a mapping part should be able to be self contained. So with this, anything that does require multiple method calls for setup should use a lambda. So as we mentioned before, AsList should not be terminating, it should use a lambda.

---

Re-reading this, it sounds like I'm telling people what to do. That isn't the case, I'm just putting this out for discussion. What does everybody think? I'm open to persuasion.

Like I said though, I'd like to get some kind of style-guide going on so we can all refer back to it when developing new features. Should help us keep a consistent and intuitive API.

On Fri, Aug 1, 2008 at 1:34 PM, Matt Hinze <mhi...@gmail.com> wrote:

That's it.  Terminating methods and a builder hierarchy are sort of
ugly.  Not to mention cumbersome to design (although it may eventually
come down to that).  Not that all that lambda "syntactical tar" is
super clean, but I think I like it better also.

[Brett Veenstra]

How about something like ".Required(IList<IColumn>)" to be more
explicit with the required fields?

On Jul 30, 5:53 pm, "John Teague" <jctea...@gmail.com> wrote:
> In general I feel that when creating a fluent interface required fields are
> paramters and all optional are added through chaining.
>
> On Wed, Jul 30, 2008 at 4:12 PM, Andrew Stewart <
>

> andrew.stew...@i-nnovate.net> wrote:
> > Hello
>
> > I think I agree with number 3 ;o) only joking
>
> > I quite like this one
>

> > var classMap = new ClassMap<OneToManyTarget>();

> > classMap.HasMany<ChildObject>(x => x.SetOfChildren).ForeignKey("ParentID");
>
> > Then it's optional and keeps the api simpler, although slightly longer.
>
> > Andy
>

[Paul Batum]

I think perhaps you misunderstood Brett. When John said 'required fields', he was referring to mandatory parameters, i.e. information that must be passed when invoking particular operations against the fluent interface. Not required fields in terms of validation.