PONSO: NSSet-based templates, improved inverse relationship logic

[tyrone-sudeium]

Three major changes in this one:

Inverse relationships are more Core Data like

In the case where you have, for example:

Employee has one Department
Department has many Employees

If you tried to do:
[anEmployee setDepartment: aDepartment];
It would fail to set the inverse "many" relationship (i.e add anEmployee to the employees array). I've made a few changes to the PONSO template to correct this behaviour. I've taken steps to ensure that it won't infinite loop!

Fixed a memory leak when the "many" relationship in a one-to-many is Transient

In the case above, if it was configured as such:

Employee has one Department [Non-Transient]
Department has many Employees [Transient]

Employees would be added to an NSArray, and by default, NSArrays retain any objects added to them. This results in an unexpected memory leak (since one would assume that making it transient, like for the "one" side, causes it to be "assigned"). I've fixed this by using a trick when creating the NSMutableArray (or NSMutableSet, see below) - using CFArrayCreateMutable, you can set the "retain" and "release" functions to NULL to prevent it from calling retain and release on added objects. Since this makes use of the Toll-free bridge, it will cause issues with ARC projects.

NSSet-based PONSO Templates

I've also added a sub-directory to the PONSO templates folder which is basically the same as the NSArray ones except it uses NSSet instead of NSArray. This is particularly useful, since along with my changes to inverse relationships, with NSArrays, the following code would result in unexpected behaviour:
[anEmployee setDepartment: aDepartment]; [anEmployee setDepartment: aDepartment];
You'd end up with the employees array containing anEmployee twice, which may not be what the developer was expecting. Using the NSSet-based templates gets around this issue since NSSets only accept unique objects.

[nzhuk]

Thanks for great work! I'll review your patch tomorrow and pull it.

On 19.8.2011, at 10.22, tyrone-sudeium reply@reply.github.com wrote:

Three major changes in this one:

Inverse relationships are more Core Data like

In the case where you have, for example:

Employee has one Department
Department has many Employees

If you tried to do:
[anEmployee setDepartment: aDepartment];
It would fail to set the inverse "many" relationship (i.e add anEmployee to the employees array). I've made a few changes to the PONSO template to correct this behaviour. I've taken steps to ensure that it won't infinite loop!

Fixed a memory leak when the "many" relationship in a one-to-many is Transient

In the case above, if it was configured as such:

Employee has one Department [Non-Transient]
Department has many Employees [Transient]

Employees would be added to an NSArray, and by default, NSArrays retain any objects added to them. This results in an unexpected memory leak (since one would assume that making it transient, like for the "one" side, causes it to be "assigned"). I've fixed this by using a trick when creating the NSMutableArray (or NSMutableSet, see below) - using CFArrayCreateMutable, you can set the "retain" and "release" functions to NULL to prevent it from calling retain and release on added objects. Since this makes use of the Toll-free bridge, it will cause issues with ARC projects.

NSSet-based PONSO Templates

I've also added a sub-directory to the PONSO templates folder which is basically the same as the NSArray ones except it uses NSSet instead of NSArray. This is particularly useful, since along with my changes to inverse relationships, with NSArrays, the following code would result in unexpected behaviour:
[anEmployee setDepartment: aDepartment]; [anEmployee setDepartment: aDepartment];
You'd end up with the employees array containing anEmployee twice, which may not be what the developer was expecting. Using the NSSet-based templates gets around this issue since NSSets only accept unique objects.

Reply to this email directly or view it on GitHub:
#68

[nzhuk]

Ok, couple of comments:

If you try to run the sample project, there's now a mismatched endforeach in "contributed templates/Nikita Zhuk/ponso/templates/machine.m.motemplate" on line 211. I think you should add <$endif$> on line 190, but since it's your change you should decide on that.

1. If you run the sample project you'll get a use-after-free error. Stack trace:

#0 0x00007fff8d781737 in _forwarding_ ()
#1 0x00007fff8d781618 in forwarding_prep_0_ ()
#2 0x0000000100006b8b in -[_ModelDepartment setCompany:settingInverse:] at /Users/nzhuk/Downloads/tyrone-sudeium-mogenerator-0d62ffb/contributed templates/Nikita Zhuk/ponso/sample project/PonsoTest/Sources/DataModel/_ModelDepartment.m:213
#3 0x0000000100006c15 in -_ModelDepartment setCompany:
#4 0x0000000100006cba in -_ModelDepartment dealloc
#5 0x00007fff8d6f9800 in CFRelease ()
#6 0x00007fff8d721d20 in -__NSArrayM dealloc
#7 0x00007fff9478c03c in (anonymous namespace)::AutoreleasePoolPage::pop(void*) ()
#8 0x00007fff8d7226a5 in _CFAutoreleasePoolPop ()
#9 0x00007fff93c1a0d7 in -NSAutoreleasePool drain

Reason:
2011-08-20 16:38:08.624 PonsoTest[6222:60b] *** -[ModelCompany removeDepartmentsObject:settingInverse:]: message sent to deallocated instance 0x1049a2fc0

You can reproduce this by running the sample app by first renegerating _ModelXXX classes with your version of array-based templates, compile the PonsoTest and enable Zombies.

[nzhuk]

Also couple of generic comments.

Using NSSet is something I wanted to avoid in my own templates because I see some value in having ordered relationships without requiring sorting at fetch. The best data structure here would be an ordered set. It's already introduced in 10.7 (NSOrderedSet) and probably will be in iOS5. However, to keep compatibility with 10.6 and iOS4 I think the best solution would be just to write a class with similar API as NSOrderedSet and use that in PONSO templates (and switch to native implementation in latest OS versions). For now I've used "external" prevention of duplicates in my ModelXXX subclasses, usually either with naive containsObject: check or with some key-based NSDictionary lookup.

However, if you see that NSSet-based templates would be useful, then I would suggest creating your own "contributed templates" subfolder (e.g. "contributed templates/Tyrone Trevorrow/ponso") and put them there, so you'll get proper credit as well on this work. Feel free to reuse and reconfigure my PonsoTest sample project for your own templates.

[nzhuk]

Also about using the non-retaining CFArray. It basically means that you end up with an array full of dangling pointers when objects get deallocated. I understand that your goal is to break the retain cycles (I've done it myself in ModelXXX subclasses for now), but it opens the possibility of hard-to-track use-after-release bugs. Have you checked out Mike Ash's weak ref library [1] ? How about using MAWeakArray instead?

[1] https://github.com/mikeash/MAZeroingWeakRef

[tyrone-sudeium]

I've fixed that autorelease issue by removing the call to self.relationship = nil in dealloc for transient relationships. Since it's in the dealloc method, and the object being dealloc'd isn't retaining those variables, it would be a no-op at best to be setting that property to nil (and in this case, thanks to the set inverse "magic", causing a crash at worst).

As for NSArray vs NSSet, for the project I'm working on it was pretty much a requirement that we use NSSet, so I had to put the work in anyway so I figured I'd contribute it back to this project. We have no need for an "ordered" NSSet, and our project needs to work on iOS 4, so I'm happy enough with my solution for our internal project. If down the line you're going to implement your own NSOrderedSet-based templates, it would be somewhat counterproductive for me to set up my own contributed templates folder for an implementation that's going to be obsolete very shortly anyway. Feel free to remove my NSSet templates, but by all means use it for inspiration in your future implementation.

As for dangling pointers, I'm reluctant to include another dependency for this project. Part of the appeal of this implementation is that, for the most part, it is standalone and just requires you to add a script to your project and have the mogenerator engine installed. Having to do that, and have to manage what could potentially become a complex tree of dependencies would seriously reduce the appeal. For the most part, the memory management model of the relationships should be pretty well hidden from the developer - the idea is that the relationships will more or less "manage themselves", and all you have to do is set the properties here and there (similar to how it works in Core Data). Changing or nil'ing out the relationships should automatically cause the inverses to have the references removed (and thus handle the dangling pointer issue).

[rentzsch]

Nikita, I'll be pushing out a new version soonish. Merge Tyrone's stuff if you want it in the next release or just let me know it will sit this one out.

[tyrone-sudeium]

Sorry about my long absence, been completely snowed under at work. I've taken nzhuk's advice and moved my variant of PONSO into its own contributed templates folder. It's still 99% nzhuk's code though, and all the READMEs and code comments still reflect this. I also merged in some changes I made which were necessary for the project we just finished (NSCoding support, mostly). We've also tested (at least, the non-NSCoding variant) in a production setting and they work really well!

EDIT: Sorry meant we've tested the non-NSCoding variant in a production setting!

[nzhuk]

Ok, I'll check this today-tomorrow. Thanks for the heads-up!

On 29.11.2011, at 1.00, Jonathan 'Wolf' Rentzsch reply@reply.github.com wrote:

Nikita, I'll be pushing out a new version soonish. Merge Tyrone's stuff if you want it in the next release or just let me know it will sit this one out.

---

Reply to this email directly or view it on GitHub:
#68 (comment)

[nzhuk]

Tyrone, looking good, but could you reset all the changes made in "contributed templates/Nikita Zhuk" so that the NSArray -> NSSet changes are only in your subdirectory? This way you'll be free to maintain your set of templates independently from my templates.

Also, you might want to adjust the value of MOGENERATOR_TEMPLATES variable in "Tyrone Trevorrow/ponso/sample project/PonsoTest/PonsoTest.xcodeproj" -> Run Script phase so that it points to your templates directory.

[tyrone-sudeium]

Hi Nikita, I'm not sure what you mean by NSArray -> NSSet changes. I never actually modified the templates at the root level (contributed templates/Nikita Zhuk/ponso/templates) to change them to NSSet, I merely duplicated them and created an NSSet variation in a subfolder (contributed templates/Nikita Zhuk/ponso/templates/NSSet), which I subsequently moved into my own folder (contributed templates/Tyrone Trevorrow/ponso/templates/NSSet). The only remaining changes I made that are still in your templates are where relationships automatically update their inverse. Is this the change you want removed?

[nzhuk]

Tyrone,

It might just be me not getting Github's pull requests (I'm still a bit new to this), but if I open the "Diff" tab of this pull request, I can see that there's a modification of e.g. "contributed templates/Nikita Zhuk/ponso/sample project/PonsoTest/Sources/DataModel/_ModelAssistant.h" where NSArray is replaced by NSSet.

[tyrone-sudeium]

Hi Nikita, I've copied over the PonsoTest project from rentzsch/master and it should be better now. I did make changes on my end to the sample project for testing and I inadvertently committed those changes. As shown in the diff, there should no longer be any references to NSSet in your templates folder.

[rentzsch]

Thanks guys. Hey Nikita, merge the pull request when you're satisfied. I'll notice and then start the release process.

[nzhuk]

Hi Tyrone,

Thanks, looking good now. Will merge & pull.