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

Fix PostgreSQL compound key discovery #57653

Merged
merged 4 commits into from
Jun 21, 2024

Conversation

strk
Copy link
Contributor

@strk strk commented Jun 4, 2024

Closes GH-56420

@strk strk added Requires Tests! Waiting on the submitter to add unit tests before eligible for merging Bug Either a bug report, or a bug fix. Let's hope for the latter! Relations PostGIS data provider labels Jun 4, 2024
@github-actions github-actions bot added this to the 3.38.0 milestone Jun 4, 2024
@strk
Copy link
Contributor Author

strk commented Jun 4, 2024

Tests for relation discovery seem to exist but are based on referenced keys being first attributes of the referenced table. I'm going to update that one.

To run:

QGIS_PREFIX_PATH=${builddir}/output PYTHONPATH=${builddir}/output/python:$PYTHONPATH python tests/src/python/test_provider_postgres.py TestPyQgsPostgresProvider.testValidLayerDiscoverRelationsComposite

Copy link

github-actions bot commented Jun 4, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 847ef3e)

@strk strk force-pushed the fix-relation-discover-GH56420 branch 2 times, most recently from 7f42ebf to 2e48ab9 Compare June 5, 2024 14:19
@strk strk force-pushed the fix-relation-discover-GH56420 branch from fa951bc to 4554b97 Compare June 10, 2024 09:44
@strk strk removed the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Jun 10, 2024
@strk
Copy link
Contributor Author

strk commented Jun 10, 2024

I've pushed a change in existing testcase to basically list the referenced columns in the inverse order in which they were defined. This triggers the bug reported in GH-56420

To show the CI catching the bug I've currently also pushed a revert of the fix, to expect a failure. Once the failure is shown, I'll drop the revert and leave the fix in place to see how that fixes the problem.

@strk
Copy link
Contributor Author

strk commented Jun 10, 2024

Bug confirmed by CI: https://github.com/qgis/QGIS/actions/runs/9446127636/job/26016503531?pr=57653#step:13:2257
Now I'll force-push a version of the branch with the fix in place

@strk strk force-pushed the fix-relation-discover-GH56420 branch from 4554b97 to c70936b Compare June 10, 2024 10:26
@strk strk enabled auto-merge (rebase) June 19, 2024 16:07
@strk strk added the backport queued_ltr_backports Queued Backports label Jun 19, 2024
auto-merge was automatically disabled June 19, 2024 21:20

Pull request was closed

@nyalldawson nyalldawson reopened this Jun 19, 2024
Copy link

Tests failed for Qt 5

One or more tests failed using the build from commit 847ef3e

layout_export_markerline_masked_geometry

layout_export_markerline_masked_geometry

Test failed at test_markerline_masked at tests/src/python/test_selective_masking.py:1376

Rendered image did not match tests/testdata/control_images/selective_masking/layout_export_markerline_masked/layout_export_markerline_masked.png (found 124 pixels different)

The full test report (included comparison of rendered vs expected images) can be found here.

Further documentation on the QGIS test infrastructure can be found in the Developer's Guide.

@strk strk enabled auto-merge (rebase) June 20, 2024 15:10
@strk strk disabled auto-merge June 20, 2024 15:11
@nyalldawson nyalldawson merged commit 946a136 into qgis:master Jun 21, 2024
64 of 66 checks passed
@strk strk changed the title Fix compound key discovery Fix PostgreSQL compound key discovery Jun 21, 2024
@strk strk deleted the fix-relation-discover-GH56420 branch June 21, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport queued_ltr_backports Queued Backports Bug Either a bug report, or a bug fix. Let's hope for the latter! PostGIS data provider Relations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QGIS 3.34 is not able to figure out PostgreSQL relations when there are build using compound keys
2 participants