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

[OGR] Fix transactional editing for GPKG/SQLite #60198

Merged
merged 4 commits into from
Jan 22, 2025
Merged

[OGR] Fix transactional editing for GPKG/SQLite #60198

merged 4 commits into from
Jan 22, 2025

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Jan 21, 2025

Tell the provider to reload the fields after a rollback and add some checks to verify if after the rollback the provider still needs to update the field.

Followup OSGeo/gdal#11695
Followup #59797

@elpaso
[OGR] Fix transactional editing for GPKG/SQLite
f2f9d3a 
Tell the provider to reload the fields after a rollback
and add some checks to verify if after the rollback
the provider still needs to update the field.

Followup OSGeo/gdal#11695
Followup qgis#59797
@elpaso elpaso added the Bug Either a bug report, or a bug fix. Let's hope for the latter! label Jan 21, 2025
@github-actions github-actions bot added this to the 3.42.0 milestone Jan 21, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 21, 2025 •
edited
Loading

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 61c30da)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 61c30da)

troopa81
troopa81 reviewed Jan 22, 2025
View reviewed changes
Copy link
Contributor

@troopa81 troopa81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind adding test?

src/core/providers/ogr/qgsogrprovider.cpp Outdated Show resolved Hide resolved
@elpaso
Copy link
Contributor Author

elpaso commented Jan 22, 2025

Do you mind adding test?

The test is already there, it was just disabled in #59910 because it was failing. I tested it locally and re-enabled the test (but it will only be executed in CI when GDAL will be updated to 3.11).

see: https://github.com/qgis/QGIS/pull/60198/files#diff-27d7924ae82e21553b160a9cbd020ca166502c8f38a3fa5b1640d249d5519f47R855

@elpaso
Copy link
Contributor Author

elpaso commented Jan 22, 2025

Do you mind adding test?

The test is already there, it was just disabled in #59910 because it was failing. I tested it locally and re-enabled the test (but it will only be executed in CI when GDAL will be updated to 3.11).

see: https://github.com/qgis/QGIS/pull/60198/files#diff-27d7924ae82e21553b160a9cbd020ca166502c8f38a3fa5b1640d249d5519f47R855

Forgot to mention that the upstream changes that are applied here were extensively tested.
https://github.com/OSGeo/gdal/pull/11695/files#diff-847e53968d7d4ca113d215ab216da9baf66ce59de9eff9f7bae7a78b9f9b562eR553

@troopa81
Copy link
Contributor

The test is already there, it was just disabled in #59910 because it was failing. I tested it locally and re-enabled the test (but it will only be executed in CI when GDAL will be updated to 3.11).

OK then

troopa81
troopa81 approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@troopa81 troopa81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@elpaso elpaso merged commit 4fe8c62 into qgis:master Jan 22, 2025
31 checks passed
@elpaso elpaso deleted the fix-transactional-field-editing-with-gdal branch January 23, 2025 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@troopa81 troopa81 troopa81 approved these changes

Assignees
No one assigned
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter!
Projects
None yet
Milestone
3.42.0
Development

Successfully merging this pull request may close these issues.
2 participants
@elpaso @troopa81