Skip to content

Relationships do not create the correct sub-entity #112

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.

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

Merged
merged 37 commits into from
Sep 8, 2014

Conversation

rhodgkins
Copy link
Contributor

I'm going to try to explain this but it might be better having a look at the object graph and test cases!

Object graph

Root allows sub entities of Parent to be stored against it, but the Parent entity description of any subentites is returned instead of the specific ChildA or ChildB entity descriptions.

730554f adds 2 failing test cases for one-to-one and many-to-many relationships.

  • one-to-many relationships (fixed in 834de26)
    This was fixed in the same way as when fetching entities based on parent entities, looking up the hash of the entity name from entityTypeCache (line 231 and 267).
    I think this also needs to be applied to the other 2 relational types.
  • one-to-one relationships (fixed in c36fe53)
    I think these types of relationships with subentites will need a join on the destination entities table (where appropriate) so instead of:
SELECT name, oneToOne__objectid FROM ecdRoot WHERE __objectID=?;

There should also be another column for the type of the oneToOne entity:

SELECT ecdRoot.name, ecdRoot.oneToOne__objectid, ecdParent.__entityType FROM ecdRoot INNER JOIN ecdParent ON ecdParent.__objectid=ecdRoot.oneToOne__objectid WHERE ecdRoot.__objectid=?;
  • many-to-many relationships (fixed in 8ea8aa3)
    This has deeper issues as the relation table name columns are based on the relational subentities not its parent entities. For example the following table is created for the above object graph:
CREATE TABLE ecd_manyToMany_manyToManyInverse ('ChildA__objectid' INTEGER NOT NULL, 'Root__objectid' INTEGER NOT NULL, PRIMARY KEY('ChildA__objectid', 'Root__objectid'))

But the primary key column need to be based on the super entity:

CREATE TABLE ecd_manyToMany_manyToManyInverse ('Parent__objectid' INTEGER NOT NULL, 'Root__objectid' INTEGER NOT NULL, PRIMARY KEY('Parent__objectid', 'Root__objectid'))

And also when querying for many-to-many relations a JOIN needs to be performed, and the type column added if needed:

SELECT ecd_manyToMany_manyToManyInverse.Parent__objectid, ecdParent.__entityType FROM ecd_manyToMany_manyToManyInverse INNER JOIN ecdParent ON ecdParent.__objectid=ecd_manyToMany_manyToManyInverse.Parent__objectid WHERE ecd_manyToMany_manyToManyInverse.Root__objectid=?

I'm going to try looking at the other issues, but I think I'm along right lines with the last 2 issues?

@rhodgkins
Copy link
Contributor Author

I've also updated the project a bit to use XCTest and fixed some warnings.

@rhodgkins
Copy link
Contributor Author

I think the issue should be fixed now! I might try add a few more test cases to just ensure any edge cases are fixed...

@rhodgkins
Copy link
Contributor Author

Many-to-many relationships fixed in commits 8ea8aa3

@rhodgkins
Copy link
Contributor Author

I've also added support for the BETWEEN comparison predicate.

I think this is ready for a merge now, like I said above I might make some more test cases.
Though on the topic of this I might create a test suite against a standard core data stack and then create the same entities in both and then compare various fetches and relations against this.

Also a note here that this should probably be tagged as 3.0 as it contains breaking changes from previous versions which cannot be migrated as these schemes did not have the __entityType column.

@gavin-black
Copy link
Member

Hi @rhodgkins,

Wow this is a great update, that hopefully puts to rest most of the many relationship issues. Sorry it took a while to get a response out to you.

Appreciate you taking the time to show us the entity diagram. That definitely helped make sense of the new changes. It's a pretty good sized rework that was really needed.

Changing things to be tied to the super entity definitely looks like the right thing to do. I'm glad you thought through it that much, makes more sense than how we originally had it (Tied to the subentities). Also doing the JOIN for many-to-many while tracking the type seems like it was pretty much required to get things to work properly.

Merging in the pull request now, and will tag it as 3.0 to avoid any confusions.

Thanks again for your contributions!

gavin-black added a commit that referenced this pull request Sep 8, 2014
Relationships do not create the correct sub-entity
@gavin-black gavin-black merged commit 76634ec into project-imas:master Sep 8, 2014
@rhodgkins
Copy link
Contributor Author

@gavin-black no problem happy to help with such a great and useful framework :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants