fix(schema): resolve m2m BasePKs through the base table's FieldMap#1375
Open
dragonator wants to merge 1 commit into
Open
fix(schema): resolve m2m BasePKs through the base table's FieldMap#1375dragonator wants to merge 1 commit into
dragonator wants to merge 1 commit into
Conversation
fef87d4 to
a6ab5d8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
m2mRelationcopiesrel.BasePKsfromleftRel.JoinPKs, whoseField.Indexis relative to the m2m left field's belongs-to target. When the base table embeds that target — a common pattern for wrapping a model with extra fields — walking those indices against the outer struct at query time hits the wrong field, and panics when the Go field types disagree.This PR re-resolves the base PKs by name through
t.FieldMapso embedded outer tables use their re-indexed fields. Whentis the target itself the lookup returns the same*Field, so existing behavior — including the composite-key fix from #996 — is preserved.Reproduction
Root cause
schema/table.gom2mRelation:leftRel.JoinPKsare looked up in the belongs-to target'sFieldMap—Order.FieldMap["id"]withIndex=[1]in the example above.At query time,
relation_join.gom2mQuerywalks those PKs against the outer base model:For
OrderWithExtra,j.BaseModel.rootValue()is the wrapper struct — its field index[1]is the embedded*Orderpointer, notOrder.ID. The correct path throughOrderWithExtra → *Order → IDis[1, 1], which is exactly whatt.FieldMap["id"]already holds for the wrapper (processFieldsre-indexes embedded fields viamakeIndex(...)).Regression history
d99b767, which changedm2mQueryfrombaseTable.PKstoj.Relation.BasePKs. Correct in spirit (composite keys need the relation's ordered PK set, not all base PKs), but it implicitly assumed the relation'sBasePKswere already indexed against the base table — which they aren't when the base table embeds the target.master/ v1.2.18.The fix
What this means in practice:
OrderWithExtra→*Order):t.FieldMap["id"]returns the cloned field withIndex=[1, 1].m2mQuerywalks the wrapper correctly.Orderqueried directly):t.FieldMap["id"]is the same*Fieldinstance asleftRel.JoinPKs[0]. No behavior change.leftRel.JoinPKsstill drives the order and count of base PKs — the lookup just substitutes per-element references.testCompositeM2Mcontinues to pass.t.FieldMap(e.g. shadowed bybun:"-"on the wrapper), the original field is kept so behavior is no worse than today.Tests
Unit test —
schema/table_test.go::TestTable/m2m_on_embedded_base. Builds the wrapper shape, registers the m2m table, and asserts:outer.FieldMap["id"].Index == [1, 1]— sanity thatprocessFieldsre-indexed the embedded field.outer.Relations["Items"].BasePKs[0]is the same*Fieldinstance asouter.FieldMap["id"].Without the fix the relation references the embedded type's
*Field(Index=[1]), and the test fails with a clear[]int{1, 1}vs[]int{1}diff.Integration test —
internal/dbtest/orm_test.go::testM2MRelationOnEmbeddedBaseModel. Runs an end-to-end SELECT with.Relation("Items")against the embedded wrapper and asserts the right children load. Compiles against all dialect builds; full end-to-end verification needs the docker-compose setup frominternal/dbtest/test.sh.Risk
Minimal. The change is local to
m2mRelationand only swaps the source ofrel.BasePKsfor same-named entries in the base table'sFieldMap. All existing schema and ORM unit tests pass; the composite-key m2m test is unaffected.