The recursive reference because of the two-way binding

[namco1992]

What version of SQLBoiler are you using (sqlboiler --version)?

3.2.0

If this happened at runtime what code produced the issue? (if not applicable leave blank)

For example, we have a user table and an account table.

CREATE TABLE `users` (
  ...
  `account_id` int(10) unsigned NOT NULL,
  ...
  CONSTRAINT `users_accounts_id_fk` FOREIGN KEY (`account_id`) REFERENCES `accounts` (`id`)
  ...
)

CREATE TABLE `accounts` (
  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  ...
)

Because of This Commit, when we eager load the Accounts, there will be a recursive reference, the user.R.Account has account.R.Users, and so on. I understand it could be useful in certain cases, but when we try to gob this object and put into the cache, the gob can't handle the recursive reference properly and ends in stack overflow error.

Further information. What did you do, what did you expect?

The bidirectional binding is helpful, but it will break the serialization if you happen to serialize it with a library which doesn't handle that case (like gob).

[aarondl]

I'm not opposed to a flag/config piece that would disable the back referencing. I'd expect a PR for it though.

JSON and friends don't have this problem because we specifically ignore the R field completely and recommend people make their own object graph if they want to serialize it out. Gob to my knowledge does not have a mechanism to ignore a field via struct tags or anything like we do for JSON/TOML/YAML etc and so this is not fixable without said PR.

I would also recommend you to not use Gob. It's bad in very many ways. If you want an efficient binary serialization check out Protocol Buffers or MsgPack. As is this is working as intended and there's no specific fix we can make for Gob unless that PR is created.

[RichardLindhout]

@aarondl I have the same problem with a GraphQL api and I generate converts with:https://github.com/web-ridge/gqlgen-sqlboiler

So when I ask

{
  flowBlocks {
    id
    blockChoice {
      id
    }
    # Parent
    flowBlock {
      id
    }
    block {
      id
      title
      blockType
      blockChoices {
        id
        title
        description
        slug
      }
    }
  }
}

I've got a stack overflow because

func BlockToGraphQL(m *models.Block) *graphql_models.Block {
	r := &graphql_models.Block{.........}
	if m.R != nil && m.R.BlockChoices != nil {
		r.BlockChoices = BlockChoicesToGraphQL(m.R.BlockChoices)
	}
	return r
}

func BlockChoiceToGraphQL(m *models.BlockChoice) *graphql_models.BlockChoice {
	r := &graphql_models.BlockChoice{....}
	if !helper.IntIsZero(m.BlockID) {
		if m.R != nil && m.R.Block != nil {
			r.Block = BlockToGraphQL(m.R.Block) // Stack-overflow
		} else {
			r.Block = BlockWithIntID(m.BlockID)
		}
	}
	return r
}

I don't think the R should be filled unless you send and explicit Load(")
Anyways I've found a solution by doing this

func BlockChoiceToGraphQL(m *models.BlockChoice, root interface{}) *graphql_models.BlockChoice {
	if m == nil {
		return nil
	}
	r := &graphql_models.BlockChoice{.......}
	if !helper.IntIsZero(m.BlockID) {
		if m.R != nil && m.R.Block != nil {
			rootValue, sameStructAsRoot := root.(*models.Block)
			if !sameStructAsRoot || rootValue != m.R.Block {
				r.Block = BlockToGraphQL(m.R.Block, m)
			}
		} else {
			r.Block = BlockWithIntID(m.BlockID)
		}
	}
	return r
}

[RichardLindhout]

I changed some things:

func alreadyConverted(roots []interface{}, check interface{}) bool {
	var matched int
	for _, root := range roots {
		if root == check {
			matched++
		}
	}
	return matched > 2
}

I know have a maximum of two times the same nested struct.

	if !helper.IntIsZero(m.BlockID) {
		if m.R != nil && m.R.Block != nil {
			if !alreadyConverted(roots, m.R.Block) {
				r.Block = BlockToGraphQL(m.R.Block, append(roots, m))
			}
		} else {
			r.Block = BlockWithIntID(m.BlockID)
		}
	}

[aarondl]

@RichardLindhout The backreferencing is problematic in many ways to be honest. We recently ran into a race condition because relationship set helpers are setting .R on the method's input parameters which is probably incorrect. In v4 I think we may stop creating .R links to mirror the databases state because it is in the end a best-effort attempt and will almost always be incorrect anyway. It's up for debate whether it's good or bad tbh. I can see the utility in some cases, and in others I can see how the repercussions of it are fairly painful.

[RichardLindhout]

Maybe self-referencing is not ideal. But are you removing the whole .R thing from models? I still like the .R thing. With what would you replace it?

[aarondl]

@RichardLindhout I doubt that I'd remove it unless I could find a good alternative (which I think I might have in my mind). It's more likely that we'd implement controls so you have the ability to turn on and off forward/backlinking/all linking in .R for a given call to a relationship helper as a baseline starting point.

[namco1992]

@aarondl Hi, I would like to work on this if you are still open to a PR.

[aarondl]

@namco1992 I think as a bandaid fix we could add some sort of gross WithDisabledBackreferencing context object helper. I talked with @nullbio at length about this and we disagree a lot on how we might solve this specific problem.

The issue is mostly a difference in priority. Some people want the object graph in .R to always be a perfect representation of the database. Some people want sqlboiler to stop mutating input parameters! We talked about mutexes and disables and all kinds of things but in the end I think that an sqlboiler v4 would rip .R completely out in some fashion. It's clear that the current eager loading solution is not withstanding the test of time.

If you were to create a PR that disabled backreferencing via context I would accept it.

[namco1992]

Hi @aarondl,
How do you think that we have a flag like --no-backreferencing for generating the templates? Some users like us haven't migrated to the context style yet, then the context object helper won't be helpful. We can simply remove the backreferencing parts based on the flag and it seems to be a cleaner fix to me. It's actually what we are doing now and it works fine.

[aarondl]

@namco1992 The reason for using the context approach is to allow the feature to be turned on/off per query. It may be useful at times.

I think we should be encouraging a migration to the context approach as sqlboiler already supports features like boil.WithDebug and boil.SkipTimestamps to allow per-query debugging, ignoring automatic timestamp fields, and passing an http request context through for cancellation of sql queries is quite nice too. That's to say: I don't think we should discount the idea just because it uses context if it's the "right" way for us to implement it. Though I don't doubt that would squash your eagerness to create a PR for it if you can't use it haha.

So the question to me really comes down to: Is it better to disable backreferencing completely for a generated package? Or is it better to allow it to be per-query? What do you think?

[namco1992]

Hi @aarondl,
I agree with you that a context-based switch is more flexible. In general, using context is a good practice and we are slowly moving towards this direction. I can see why boil.WithDebug and boil.SkipTimestamps are useful in certain cases. However, I can't think of a scenario that I need to switch between backreferencing and no backreferencing. I would like to take advantage of the go generate to save some trivial and repetitive work, therefore I'm in favor of being able to disable it at code generation. But my experience is limited and we can ask for more people's opinions. Maybe @RichardLindhout can share thoughts on it?

[RichardLindhout]

I think we should not go further than specified in the code.

users, err := models.Users(Load("FavoriteMovies")).All(ctx, db)
if err != nil {
  return err
}

So in FavoriteMovies no R.User object except when someone

users, err := models.Users(Load("FavoriteMovies"), Load("FavoriteMovies.User")).All(ctx, db)
if err != nil {
  return err
}

Gorm does this too, this is a good solution since you can specify how far you go.

[RichardLindhout]

But Load("FavoriteMovies.User") should maybe not result in an extra query but should look if their is a back-reference available and then add it. But it should not be the same pointer without the R.FavorieMovies in it.

[aarondl]

@namco1992 The designs actually aren't mutually exclusive. Just like boil.DebugMode was a global switch before context.WithDebug showed up, this could be both as well if we need it to be. We can disable it in code generation in the initial implementation.