go-kallax icon indicating copy to clipboard operation
go-kallax copied to clipboard

implement many to many relationships

Open erizocosmico opened this issue 7 years ago • 6 comments

This PR is the first of several that will be merged into feature/m2m, which will be merged when full support for many to many relationships is completely finished.

What is introduced in this PR?

  • Accessing of several properties in through struct tag of a field via methods of Field type.
  • Support both on queries and in the batchRunner for retrieving N:M relationships. 1:N using a through should work but that is yet to be tested.

Notable changes and breaking changes

  • Identifier now has a newPtr method used to get a new fresh empty pointer of that identifier type (internal, just used in the batcher). This restricts the extension of the Identifier interface, though, which is probably not a big deal.
  • Scan now accepts N extra pointers. Again, it's internal and not really exposed to the public, so we should be good.
  • ManyToMany has been renamed to Through, as 1:N with an intermediate table also need to be processed as a Through kind of relationship.
  • ForeignKeys has now a slice of foreign keys, instead of just one. Only generated code uses it, but it will be broken until regeneration.

What's left?

  • [x] Schema generation
  • [x] Go boilerplate generation
  • [ ] Inserts + updates with through relationships
  • [x] Test it works with 1:N with intermediate tables

At least, all the internal mumbo jumbo should be done with this PR.

Review

I suggest you take a look at it per-commit, as they are very clear and separate functionalities. (Should have made 2 PRs actually, but /shrug)

CI

Yes, CI is failing and will be until code generation is implemented, because of the ForeignKeys internal breaking change.

erizocosmico avatar Jun 19 '17 16:06 erizocosmico

Codecov Report

Merging #183 into feature/m2m will increase coverage by 0.41%. The diff coverage is 85.54%.

Impacted file tree graph

@@               Coverage Diff               @@
##           feature/m2m     #183      +/-   ##
===============================================
+ Coverage        78.94%   79.35%   +0.41%     
===============================================
  Files               19       19              
  Lines             3590     3798     +208     
===============================================
+ Hits              2834     3014     +180     
- Misses             544      560      +16     
- Partials           212      224      +12
Impacted Files Coverage Δ
generator/template.go 93.12% <100%> (+0.32%) :arrow_up:
store.go 87.32% <100%> (+0.05%) :arrow_up:
resultset.go 80.64% <100%> (+0.98%) :arrow_up:
query.go 99.37% <100%> (+0.1%) :arrow_up:
model.go 58.82% <33.33%> (+0.32%) :arrow_up:
generator/processor.go 88.11% <33.33%> (-0.63%) :arrow_down:
batcher.go 76.82% <71.21%> (-2.12%) :arrow_down:
schema.go 81.01% <88.88%> (+0.45%) :arrow_up:
generator/types.go 86.91% <90.51%> (+0.92%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e9cc0ec...f4e64d1. Read the comment docs.

codecov[bot] avatar Jul 03 '17 14:07 codecov[bot]

@Serabe @dpordomingo

There is a problem with the automatic insert+update of relationships with a table in between: it's a record that may need to be initialized in some way (imagine a database that used UUIDs as IDs instead of numerical ones).

For example, imagine the following scenario:

type User struct {
  kallax.Model
  ID uuid.UUID
}

type Post struct {
  kallax.Model
  ID uuid.UUID
}

type UserPost struct {
  kallax.Model
  ID uuid.UUID
  User *User
  Post *Post
}

In this case, the user will need to set the ID in the constructor, and this is necessary before inserting this record in the database. The problem there is that constructors can have arbitrary parameters and we're not magicians so we don't know how to call them behind the scenes. There's only so much magic we can do.

So, we can either think of a solution for this that is nice for the user (which I can't really think of right now) or one the following:

  • Records for relationships with a through are read only and insertions need to be manual, but updates can be automatic.
  • Records for relationships with a through are read only and insertions/updates both need to be manual.
  • Create methods like AddX, RemoveX, SaveX for each through relationship in which you have to pass the already initialized through record.

Note that updates are not a problem, since they don't need the through record to be saved. The problem is only during insert.

Any thoughts?

erizocosmico avatar Jul 03 '17 15:07 erizocosmico

I'm adding this to backlog, to keep track in the future.

Serabe avatar Jul 05 '17 13:07 Serabe

In the upcoming days, I'm merging this into a wip/feature branch in order to work more confortably from it. Please, consider discussing any issues on the open issue related to this (#114).

After merging this PR, destination branch will be rebased to master (I'm praying to the flying spaguetti monster, cthulu, or whoever can help me) and another PR will be created when the feature is complete (or, at least, usable) in case someone wants to review it.

roobre avatar Nov 28 '17 18:11 roobre

Sooo... what's the status? Are you guys gone full go-mysql-server or what?

iorlas avatar Jul 25 '19 06:07 iorlas

@iorlas Thanks for your interest. I cannot speak for src-d, but personally I lack the time needed to push this feature forward.

roobre avatar Jul 25 '19 13:07 roobre