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

Add @OneToMany annotation as alternative to @BelongsTo #664

Closed
cschabl opened this issue Jan 31, 2018 · 17 comments
Closed

Add @OneToMany annotation as alternative to @BelongsTo #664

cschabl opened this issue Jan 31, 2018 · 17 comments

Comments

@cschabl
Copy link
Contributor

cschabl commented Jan 31, 2018

There should be a @OneToMany annotation for associating parent models with child models for loading child models with getAll() from the parent model, because adding a @BelongsTo to the child model introduces a cyclic dependency between parent and child.

The latter is ok if both parent and child are in the same package. But if they are in different logical modules, it could be objectionable.

@ipolevoy
Copy link
Member

Enable logging and see how the framework discovers @BelongsTo. It creates two associations: has many and belongs to at the same time. Not sure why you think there is a cyclic dependency. We were able to use this to even create tree structures of one to many dependencies of the same type.
Can you elaborate or create an example that explains your point better?

@cschabl
Copy link
Contributor Author

cschabl commented Feb 1, 2018

Children.java:

package moduleA;
// imports omitted for brevity
@BelongsTo(parent = Parent.class)
public class Children extends Model {
}

Parent.java:

package moduleB;
// imports omitted for brevity
public class Parent extends Model {
   public List<Children> getChildren() {
      return getAll(Children.class);
   }
}

Tools like Sonar or IntelliJ IDEA ("Analyze Cyclic Dependencies") will tell you that there is a cyclic dependency between package moduleA and moduleB. If these packages were in different modules of a multi-module project, the code wouldn't even compile. If there were a annotation OneToMany(children = Children.class) on the class Parent, the dependency cycle could be avoided.

@ipolevoy
Copy link
Member

ipolevoy commented Feb 1, 2018

@cschabl when you mention models, are you talking about Java 9, or you mean Maven modules?
Can you also include a screenshot? Additionally, are you sure you need the annotation? If you follow naming conventions, you do not need any annotations: http://javalite.io/one_to_many_associations#writing-models

@ipolevoy
Copy link
Member

ipolevoy commented Feb 1, 2018

small note: I suggest a singular form to name models (Child rather than Children), since there is one instance created per row.

@ipolevoy
Copy link
Member

ipolevoy commented Feb 1, 2018

I think I understand your issue. You split your models across Maven modules and the reactor cannot figure out what module to compile first, right? Generally, the best practice is to have all your models in the same module, since they are the DB access layer. We usually have a module common that contains common services, db access logic as well as models.

@cschabl
Copy link
Contributor Author

cschabl commented Feb 2, 2018

Hi Igor,

in the "multi-module project" example, I meant a Maven module. But in this conversation I generally take the perspective of software architecture/design modelling. In this context I should rather use the term "component" instead of "module".

As we decided to lay out the Java package structure along our component model instead of the architectural layers, we don't have a Java package or project module common containing all ActiveJDBC model classes.

Removing the BelongsTo annotation from our child model class results in a "not associated" exception by ActiveJDBC. That is, our DB schema given by our DB architect doesn't conform to ActiveJDBC conventions here.

So a @OneToMany wouldn't force me to introduce a cyclic dependency between components or to follow a certain package or project module layout.

@ipolevoy
Copy link
Member

ipolevoy commented Feb 2, 2018

@cschabl this is a relatively easy task to do. If you have some time, take a look at how this works. Specifically there are Association objects created during the registration phase in the Registry.

If you can contribute, I will guide and help you. If not, I will get to it later sometime, 2 -3 weeks time frame.
Please, see http://javalite.io/contributors

@cschabl
Copy link
Contributor Author

cschabl commented Feb 5, 2018

I guess, the starting point would be Registry.processOverrides()?

@ipolevoy
Copy link
Member

ipolevoy commented Feb 5, 2018

@cschabl this is correct. The processOverrides calls processOverridesBelongsTo, which is exactly what you need. The new @One2Many annotation should have exactly the same effect. Additionally, if a developer uses both: @BelongsTo and @One2Many, the Registry class should drop a warning to the log:

 "Redundant annotations used: @BelongsTo and @One2Many on a relationship between ModelA and ModelB". 

.. but nothing bad should happen.

Additionally, the new annotation should be called @HasMany rather than @One2Many.
Let me know if you need more help..

cschabl added a commit to cschabl/activejdbc that referenced this issue Feb 7, 2018
@cschabl
Copy link
Contributor Author

cschabl commented Feb 7, 2018

@ipolevoy can you take a look on my branch enhancement #664. If the changes are ok, I will adapt the other StatementProvider classes and submit a pull request.

@ipolevoy
Copy link
Member

ipolevoy commented Feb 7, 2018

@cschabl looks great, add support to other DBs, and submit a PL

cschabl added a commit to cschabl/activejdbc that referenced this issue Feb 7, 2018
@cschabl cschabl mentioned this issue Feb 7, 2018
@ipolevoy
Copy link
Member

ipolevoy commented Mar 13, 2018

@cschabl seems your SQL scripts are missing CREATE statements?
This: https://github.com/javalite/activejdbc/blob/33e3bfc4cb4f1328f036aad1d209b52ab626268b/activejdbc/src/test/resources/h2_schema.sql#L207 is not defined for any other database.

cschabl added a commit to cschabl/activejdbc that referenced this issue Mar 14, 2018
@cschabl
Copy link
Contributor Author

cschabl commented Mar 14, 2018

@ipolevoy, that is true. I created PR #696 for it.

ipolevoy pushed a commit that referenced this issue Mar 14, 2018
#664 Add @OneToMany annotation as alternative to @BelongsTo (create-table statements for other DBs)
@ipolevoy
Copy link
Member

@cschabl PR merged, lets see it it fixes the builds.

cschabl added a commit to cschabl/activejdbc that referenced this issue May 16, 2018
ipolevoy pushed a commit that referenced this issue May 17, 2018
…tatements

#664: create table statements for teams and players for other DBs.
@cschabl
Copy link
Contributor Author

cschabl commented Nov 5, 2018

Fixed in 2.2

@cschabl cschabl closed this as completed Nov 5, 2018
@ipolevoy
Copy link
Member

ipolevoy commented Nov 5, 2018

👍

@mppfiles
Copy link
Contributor

This is great. @ManyToMany feels more natural sometimes. Thanks!

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

No branches or pull requests

3 participants