Skip to content
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

many-to-many relationship #18

Open
timc13 opened this issue Jul 25, 2018 · 5 comments
Open

many-to-many relationship #18

timc13 opened this issue Jul 25, 2018 · 5 comments
Labels

Comments

@timc13
Copy link
Collaborator

timc13 commented Jul 25, 2018

@schettino72 It looks like the secondary support I just added is not working as expected, so I've been debugging it.

Now that I am really digging into the code and looking at the tests - there is something that doesn't quite make sense to me here.

I would have expected Group.members to be a list of GroupMember references instead of Profile references. Can you explain the current implementation?

@schettino72
Copy link
Owner

The model is based on Association-Object pattern [1]. A different approach then what you used with a secondary table.

See the commit where it was added 06f8a09
There is quite a bit of magic there... (sorry).

I guess my reasoning was that if you want to have GroupMember with extra attributes you would add entries of GroupMember's on its own. But for simple case where Association-Object is only a reference it provides a bit of magic for convenience (and it would work same as the secondary table you have just implemented).

Are you just trying to understand the code or this is causing trouble for you?

[1] http://docs.sqlalchemy.org/en/latest/orm/basic_relationships.html#association-object

@timc13
Copy link
Collaborator Author

timc13 commented Jul 25, 2018

It's not causing trouble necessarily - but it just seems like a confusing API for the developer.

The "magic" part is quite confusing when just looking at the YAML. For the most part, relationships are declared with references to the model, except for this one case where it can be declared with a reference to a completely different model inferred from the association (?)

SQLAlchemy indicates that the use case for an Association Object should be for any extra columns beyond the foreign keys. The secondary argument of the relationship should be used otherwise. With that being supported now, I don't think we should be doing this indirect referencing for the Association Object.

I can fix the bug with secondary for a 0.9.1 release, but I propose a breaking change for 1.0.0 to remove the magic of the association table.

@schettino72
Copy link
Owner

The secondary argument of the relationship should be used otherwise.

where does it say that? just because it mentions a use-case it doesn't mean the only reason is that use-case.

@timc13
Copy link
Collaborator Author

timc13 commented Jul 25, 2018

it doesn't say that, but i think WE (this library) should say that as it will make it less confusing by only allowing direct relationship references.

@schettino72
Copy link
Owner

I personally prefer to use the Association-Object pattern. I think it has some advantages... I think it makes the ORM code easier to read as all tables have a mapper.

  • How you query all associations when using a secondary-table pattern?
  • You can use a single pattern regardless if the association has extra data or not.

Also, fixtures for many-to-many being spelled the same way regardless of how you choose to represent the relation with SQLAlchemy is IMO a very neat feature.

If you think it is confusing, maybe it is just the case of adding some notes to docs. Not removing the feature.

And as your own code shows, it is possible for both to co-exist without interfering with each other.

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

No branches or pull requests

2 participants