Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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.

Sign up for GitHub

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

Jump to bottom

Fix M2M relations for models with composite keys #996

Merged
merged 9 commits into from
Oct 26, 2024
Merged

Fix M2M relations for models with composite keys #996

merged 9 commits into from
Oct 26, 2024

Conversation

ygabuev
Copy link
Contributor

@ygabuev ygabuev commented Jun 5, 2024 •
edited
Loading

This PR adds support for M2M relations with composite keys. This feature was probably supposed to be working previously, but was not tested and turned out to be buggy. Existing functionality is not affected, according to the tests.

Commits:

  • fix(m2m join): don't query join columns
  • fix(m2m): enforce ordering of PKs of base table
  • fix(m2m): remove unneeded appending to struct key
  • fix(m2m): correctly infer which columns belong to which model
  • feat: add test for M2M relations with composite keys

Closes #992.

@ygabuev
fix(m2m join): don't query join columns
329e4ac 
Don't query the columns in the M2M table that are used as foreign keys
for the joined table. We only need the columns from the base table.
@ygabuev
fix(m2m): enforce ordering of PKs of base table
05f672a 
We need to ensure that we follow the same ordering of the base table
column names and values when constructing the JOIN query, as well as
when storing the base values.
@ygabuev
fix(m2m): remove unneeded appending to struct key
bcbffba 
We can construct the key by only looking at the relevant columns in the
M2M table, we shouldn't look at the columns from the joined table.
@ygabuev
fix(m2m): correctly infer which columns belong to which model
1e623bd 
The first `len(m.rel.M2MBaseFields)` columns are always coming from the
M2M table, as specified in the M2M join query in function
`relationJoin.m2mQuery()`. Thus we compare the `scanIndex` to this
value.
Additionally, more columns can be requested by the caller, and so the
checking the `m.table.FieldMap` is still relevant.
@ygabuev
feat: add test for M2M relations with composite keys
9f823aa
@ygabuev ygabuev force-pushed the fix-m2m branch 3 times, most recently from 1395704 to 9f823aa Compare June 5, 2024 12:31
@ygabuev ygabuev marked this pull request as ready for review June 5, 2024 12:57
@ygabuev
Copy link
Contributor Author

ygabuev commented Jun 5, 2024

The new testCompositeM2M test fails for the mssql2019 target - it's the only failing test. The issue is that Microsoft SQL Server dialect does not support the (a, b) in ((v1, v2), ...) syntax for lists of tuple-like objects. See the discussion in this Stack Overflow question. This syntax is used to construct the M2M query at the moment.

Since this feature is non-breaking and only fails for a single, arguably niche, target, I'd be happy to see it merged as is. For MS SQL Server backends, we can add a runtime error during the db.RegisterModel call. Alternatively, we can investigate amending the syntax used for the M2M query generation, e.g. along these lines, but this would result in lengthier queries and thus negatively affects all the other SQL backends.

@vmihailenco what are your thoughts?

@vmihailenco vmihailenco merged commit d99b767 into uptrace:master Oct 26, 2024
3 of 4 checks passed
@vmihailenco
Copy link
Member

@ygabuev thanks. I've skipped MSSQL tests for now.

e.g. along these lines, but this would result in lengthier queries and thus negatively affects all the other SQL backends.

I think this is totally fine, but let's not block this PR because of MSSQL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

m2m relations with composite keys
2 participants
@ygabuev @vmihailenco