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

Proposal: confusion on the eager on connection and function behaviors #307

Open
sio4 opened this issue Nov 15, 2018 · 0 comments
Open

Proposal: confusion on the eager on connection and function behaviors #307

sio4 opened this issue Nov 15, 2018 · 0 comments
Labels
f: associations the associations feature in pop proposal A suggestion for a change, feature, enhancement, etc

Comments

@sio4
Copy link
Member

sio4 commented Nov 15, 2018

Hi @mclark4386, @stanislas-m, and all!

I wonder following behaviors:

Query.Clone() does not copy eager and eagerFields. Is it desired action?

pop/query.go

Lines 29 to 54 in 53115b9

// Clone will fill targetQ query with the connection used in q, if
// targetQ is not empty, Clone will override all the fields.
func (q *Query) Clone(targetQ *Query) {
rawSQL := *q.RawSQL
targetQ.RawSQL = &rawSQL
targetQ.limitResults = q.limitResults
targetQ.whereClauses = q.whereClauses
targetQ.orderClauses = q.orderClauses
targetQ.fromClauses = q.fromClauses
targetQ.belongsToThroughClauses = q.belongsToThroughClauses
targetQ.joinClauses = q.joinClauses
targetQ.groupClauses = q.groupClauses
targetQ.havingClauses = q.havingClauses
targetQ.addColumns = q.addColumns
if q.Paginator != nil {
paginator := *q.Paginator
targetQ.Paginator = &paginator
}
if q.Connection != nil {
connection := *q.Connection
targetQ.Connection = &connection
}
}

I tried to do something like following before I looking in it:

normalQuery := tx.Eager("A").Eager("B").Where("some = true")
specialQuery := &pop.Query{}
normalQuery.Clone(specialQuery) // eager information is not copied
specialQuery = specialQuery.Where("")

It does not work as I imagine. I think Clone() should copy everything.

But for Q(Connection), it copies eager and eagerFields even though the comment said it will create a new "empty" query.

pop/query.go

Lines 178 to 186 in 53115b9

// Q will create a new "empty" query from the current connection.
func Q(c *Connection) *Query {
return &Query{
RawSQL: &clause{},
Connection: c,
eager: c.eager,
eagerFields: c.eagerFields,
}
}

I know, when we call Connection.Where(...), it internally do Q(c) and we can get query with earger information without further action. So I can improve my routine like below:

normalQuery := tx.Eager("A").Eager("B").Where("some = true")
specialQuery := pop.Q(normalQuery.Connection)
normalQuery.Clone(specialQuery) // eager information is not copied
specialQuery = specialQuery.Where("")

But this code is not smoothly read.

When I found this sequence at the first time, I wonder why Connection has eager and eagerFields but now I found it is required for the eager version of executors. But I feel it makes some confusion and I just thought about:

  • if we add another structure something like Eager and
  • Connection.Eager() returns Eager instead of NEW Connection and
  • add Eager.Where() wrapper and Eager.Create() wrapper, and so on,...

Then, can we keep the structure more simple?

I know there are busy actions on the eager implementation recently. Can you consider above confusion and idea?

@sio4 sio4 added the proposal A suggestion for a change, feature, enhancement, etc label Nov 15, 2018
@sio4 sio4 added the f: associations the associations feature in pop label Sep 20, 2022
@sio4 sio4 added this to the v6.1.0 - association improvement milestone Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: associations the associations feature in pop proposal A suggestion for a change, feature, enhancement, etc
Projects
None yet
Development

No branches or pull requests

1 participant