Fix primary key issues for shapefile import #646
Merged
Conversation
This file contains 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
- Don't use FID as a primary key, don't do anything with FID - Use any other field as the primary key if required, otherwise, generate a new primary key. - Make tests check both of these behaviours - Delete tests that convert GPKG to SHP files to use as test data - checked in equivalent test data. The auto-converted test data was bad anyway: it had lost the primary key columns. Fix the PkGeneratingTableImportSource - it shouldn't modify the schema of the delegate import-source that it wraps. (Modifying the schema of an import-source in place is allowed, as long as you only change the IDs of the columns - these IDs are Kart-internal and so are arbitrary for an import-source until and unless we align them to an existing table. But, other modifications to the import-source schema - such as adding a new `auto_pk` column - can cause the import-source to crash, so this must be done at the outer wrapper class only - in the PkGeneratingTableImportSource).
craigds
approved these changes
Jun 9, 2022
| if primary_key_name == self.ogrlayer.GetFIDColumn(): | ||
| # OGR automatically turns 'ogc_fid' column in postgres into an FID, | ||
| # and removes it from the list of fields below. | ||
| self.use_ogc_fid_as_pk = True |
There was a problem hiding this comment.
Probably it shouldn't? We could set the PGSQL_OGR_FID config option to some random UUID to prevent it doing this. Then we could just treat an ogc_fid field as a regular field
There was a problem hiding this comment.
... yeah we could do that. But we're not using this except for shapefiles right now so the comment is out of date and the behaviour for non-shapefiles isn't very important. I think best not to change this code too much unless it fixes a bug or we start using it for another OGR import type.
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.
generate a new primary key.
checked in equivalent test data. The auto-converted test data
was bad anyway: it had lost the primary key columns.
Fix the PkGeneratingTableImportSource - it shouldn't modify the
schema of the delegate import-source that it wraps. (Modifying
the schema of an import-source in place is allowed, as long
as you only change the IDs of the columns - these IDs are
Kart-internal and so are arbitrary for an import-source until
and unless we align them to an existing table. But, other
modifications to the import-source schema - such as adding
a new
auto_pkcolumn - can cause the import-source to crash,so this must be done at the outer wrapper class only - in
the PkGeneratingTableImportSource).
Checklist: