Jaguar-dart / jaguar_orm

Source-generated ORM with relations (one-to-one, one-to-many, many-to-many), preloading, cascading, polymorphic relations, etc
https://jaguar-dart.github.io
BSD 3-Clause "New" or "Revised" License
217 stars 52 forks source link

Problem with multiple foreign keys in one bean referencing the same bean #123

Closed mbartnik closed 5 years ago

mbartnik commented 5 years ago

Hello, I'm not sure if there is a better way to handle the situation I'm about to show since I'm no sql expert. If there is one, other than creating separate tables for these foreign keys, please point me to it.

class A { 

@PrimaryKey() 
int id; 

@Column(isNullable: true)
int c1Id;

@Column(isNullable: true)
int c2Id;

@HasOne(CBean) 
C c1;

@HasOne(CBean) 
C c2;
} 

class C { 

@PrimaryKey() 
int id;

@BelongsTo(ABean, refCol: 'c2Id', isNullable: true) 
int c2Id;

@BelongsTo(ABean, refCol: 'c1Id', isNullable: true) 
int c1Id;
 }

What I was expecting to happen is that there would be rows in the C table which only have c1Id or c2Id filled in but in reality those 2 ids are treated like a single id. All the findBy methods are generate with a where clause which finds models that have c1Id && c2Id instead of only one of them. Similar thing happens during insert/upsert where method associate associates both c1Id and c2Id from A table rather than only one of them. I tried doing something about the problem in https://github.com/RnDity/jaguar_orm/pull/1 but the changes I made were not enough and I'm not even sure if my way of solving this issue is correct. I would appreciate being pointed in the right direction on how to deal with this, if no one better qualified has time to fix this.

Thanks in advance.

tejainece commented 5 years ago

I will fix this.

mbartnik commented 5 years ago

Thank you. We've fixed this issue and some other that were bugging us on https://github.com/RnDity/jaguar_orm/tree/upsert_and_generator_changes, I know that we've used different approach that you already started developing (the @ForeignKey annotation) but if you wish, we can clean up the code from the unnecessary additions from us and merge it if you're happy with our solution. If you want to replace @HasOne and @HasMany with @ForeignKey only though, that would be much better imo.

tejainece commented 5 years ago

Nice! Please submit a pull request.

tejainece commented 5 years ago

Composite foreign keys have always worked. No?

mbartnik commented 5 years ago

You're talking about the refColargument in @BelongsTo? When you refer to my example in 1st post, the problem there is that c1 doesn't have any connection to c1id, therefore it doesn't know that it's supposed to preload data using column c1Id. If you're talking about @ForeignKey annotation, it was marked as unfinished (commented as todo).

There are actually a few more problems fixed in my PR, let me know if you want separate issues created for them:

  1. Lets say that in my example in first post c1 is annotated with @HasOne and c2 with @HasMany. In this situation generator will generate only one findByC method, whichever was defined first in my example so it'll be findByC in this case, for both c1 (correct) and c2 (incorrect). If c2 was defined first, it would generate only findByCList method.

  2. Another problem lies in generated associate method. Again referring to my example, it would generate associate method for C model like this: void associateA(C child, A parent) { child.c1= parent.c1id; child.c2= parent.c2id; } which is incorrect because we only want either c1 or c2 usually.

My PR is currently marked as a draft but if you wish you can do whatever you like with it. If you don't have time working on it, then give us some feedback and we'll try to fix it.

tejainece commented 5 years ago

Ahhh. Now I get what you mean. If you make a small/minimal example repo, it would be very useful.

mbartnik commented 5 years ago

Do you want this example repo to illustrate the problems using current versions of libraries or example repo using changes from my PR? Changes made in PR are breaking; byHasMany argument in @BelongsTo annotation would be mandatory and I'm introducing a new argument foreignKeyColumn to @HasOne and @HasMany annotations.

tejainece commented 5 years ago

Without the PR. I have made a lot of changes to the repo preparing for 4.x.x release.

I have added BelongsTo.name and Reference.name on the foreign key side. And HasOne.linkByName, HasMany.linkByName and ManyToMany.linkByName on the relations side.

BelongsTo.name and Reference.name names the foreign key constraint as well as help in linking with the correct relation (if there are multiple relations between same two tables.). linkByName is basically same as foreignKeyColumn. Suggestions for better naming is welcome.

Do you think this a reasonable fix?

I haven't implemented the generation part yet though. I am planning to implement in the next few days.

mbartnik commented 5 years ago

Yes, the fix seems similar to what we've done. I've made an early version of the example repo here: https://github.com/mbartnik/jaguar-dart-example although I still have problems with running it. Previously I've used sqflite adapter and postgres adapter causes me some problems, not sure if I'm doing something wrong or if it's the adapter. I'm getting PostgreSQLSeverity.error 42830: there is no unique constraint matching given keys for referenced table "cart" even though I've defined uniqueGroup for the id columns in cart. Nevertheless, I've generated the jorm files and it shows the problem nr 1, which is only one findBy method generated for both @HasOne and @HasMany columns. When you fix line 268 in cart.jorm.dart by changing it to, for example model.item = (await cartItemBean.findByCart(model.beverages_id, model.item_id, preload: cascade, cascade: cascade))[0];, you get the aforementioned error when running the example.

tejainece commented 5 years ago

Thanks for the simple reproducible. I have added your repo as official example repo for this usecase here: https://github.com/jaguar-orm/multiple_relations_same_tables.

I have fixed the issue in latest version of generator. It is also reflected in the example repo.

4.x.x however is a WIP.