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

implement many to many relationships #183

Open
wants to merge 18 commits into
base: feature/m2m
Choose a base branch
from

Conversation

erizocosmico
Copy link
Contributor

@erizocosmico erizocosmico commented Jun 19, 2017

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?

  • Schema generation
  • Go boilerplate generation
  • Inserts + updates with through relationships
  • 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.

@codecov
Copy link

codecov bot commented Jul 3, 2017

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%) ⬆️
store.go 87.32% <100%> (+0.05%) ⬆️
resultset.go 80.64% <100%> (+0.98%) ⬆️
query.go 99.37% <100%> (+0.1%) ⬆️
model.go 58.82% <33.33%> (+0.32%) ⬆️
generator/processor.go 88.11% <33.33%> (-0.63%) ⬇️
batcher.go 76.82% <71.21%> (-2.12%) ⬇️
schema.go 81.01% <88.88%> (+0.45%) ⬆️
generator/types.go 86.91% <90.51%> (+0.92%) ⬆️

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.

@erizocosmico erizocosmico changed the title add manythrough support in queries and batcher, add through props to field implement many to many relationships Jul 3, 2017
@erizocosmico
Copy link
Contributor Author

@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?

@Serabe
Copy link
Contributor

Serabe commented Jul 5, 2017

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

@roobre
Copy link
Contributor

roobre commented Nov 28, 2017

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.

@iorlas
Copy link

iorlas commented Jul 25, 2019

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

@roobre
Copy link
Contributor

roobre commented Jul 25, 2019

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

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

Successfully merging this pull request may close these issues.

5 participants