search

schettino72 / sqla_yaml_fixtures

Load YAML data fixtures for SQLAlchemy
Other
13 stars 8 forks source link

many-to-many relationship #18

Open timc13 opened 6 years ago

timc13 commented 6 years ago

@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 commented 6 years ago

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 06f8a090a0990b8ba413d513fd370ce4a6cc64f6 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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

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.