Repository navigation
Conversation
Generated managers matched prefetched reverse references by reading the referencing column through getters on the row classes, which custom row classes don't have to declare. The generated code then failed to compile. Match the rows by their raw column values instead. Closes simolus3#3437
Owner
|
Thanks for the contribution! I have merged your commit in 8e3cc33 and ported this to drift3 as well 👍 |
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.
Closes #3437
When a table uses a custom row class that doesn't have a getter for a column that references another table, prefetching the reverse reference generates code that doesn't compile. The generated
getPrefetchedDataCallbackmatches the prefetched rows to their parent with the row classes' getters:With
@UseRowClass(Member)on aMemberstable whoseMemberclass only hasid, the build fails with:5ecc9a7 already fixed this for forward references (
$_itemColumnreads the column from theTypedResult). This PR does the same for prefetched reverse references.Change
drift: a new$_getPrefetchedDataByColumnshelper next to$_getPrefetchedData. It runs the same combined query, but throughbuildSelectStatement(), so it getsTypedResults. It groups the referenced rows by the raw value of the referencing column and looks up each current row by the raw value of the referenced column. Both columns are passed by their SQL name and resolved withcolumnsByName, the same way$_itemColumndoes it.$_getPrefetchedDatais unchanged, so code generated by olderdrift_devversions keeps working.drift_dev: outside the drift3 preview, the manager writer emits$_getPrefetchedDataByColumnswithcurrentColumn/referencedColumn(and the referenced table class as a fourth type argument) in place of thereferencedItemsForCurrentItemclosure. The drift3 path is unchanged (more on that below).drift/test/generated/todos.g.dartanddrift/example/main.g.dart. I didn't regenerate the docs snippets or the other examples. They still call$_getPrefetchedData, which still exists.driftanddrift_dev.Tests
New
drift/test/integration_tests/regress_3437_test.dart(with its generated part file): aGroupstable and aMemberstable with a custom row class that has no getter for thegroupcolumn. It checks:membersRefsfrom groups returns the right members per group. Ondevelopthe generated file doesn't compile (the error above), so the test fails to load;groupreference from members works. This already passes ondevelop(since 5ecc9a7) and is there as a regression guard. It's also the case from Table primary composite key from Foreign Keys fails in 2.20 #3173, which looks fixed ondevelopalready.The existing manager prefetch tests (
drift/test/manager/manager_prefetch_test.dart) now run through the new helper becausetodos.g.dartwas regenerated.What I ran locally (Dart 3.13.5, macOS arm64). The workspace includes Flutter packages and I didn't have Flutter installed, so I ran
dart pub getwith the root workspace temporarily cut down todrift,drift_dev,sqlparser,future/*and the two testcase packages. That change is not part of the PR.drift:dart run build_runner build --delete-conflicting-outputs(no changes to committed files after the commit),dart format -o none --set-exit-if-changed .,dart analyze --fatal-infos --fatal-warnings,DID_RUN_BUILD_RUNNER=1 dart test --preset ci_unit_tests: 931 passed, 4 skipped.drift_dev: format and analyze clean,dart test: 512 passed, 1 skipped.sqlparser:dart test: 753 passed.future/drift_manager:dart test: 35 passed.Not run: browser/wasm tests, the Flutter packages, and the docs build.
Notes
drift_devneeds adriftthat has$_getPrefetchedDataByColumns.drift_devcurrently allowsdrift: ">=2.30.0 <2.36.0", so the lower bound needs to go up when this is released. I left that for the release, as with the other constraint bumps.ManagerResultRowis built differently, so I kept the existing closure there. Custom row classes without these getters will still fail to compile with drift3. I can port the helper todrift_managerif you want it in the same PR.