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 unique constraint error #27

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

evelyn9191
Copy link
Contributor

This commit fixes situation when the database is already filled with data and the user tries to add them again. In such a case, the script failed with IntegrityError: UNIQUE constraint failed when inserting a value

@jweckstr
Copy link
Collaborator

jweckstr commented Jan 31, 2020

I am curious of in which situations one would like to add the same content into the database? If I recall correctly the original idea has been not to accept this behavior.

Thank you for your contributions!

@rkdarst
Copy link
Member

rkdarst commented Jan 31, 2020

I also seem to remember being designed as a one-shot thing. But your changes are exactly what I would do to allow incremental updating. I guess one use case would be "create the DB, get newer version of gtfs a week later, incrementally update" ?

I'd be careful that no other bugs appear on multiple import. For example, post-processing creating duplicate structures or multiple IDs for the same object.

I'm cautiously optimistic about this, but if you're using this successfully, then that makes me positive.

@evelyn9191
Copy link
Contributor Author

Yes, @rkdarst, that is exactly my use case.
The GTFS data are updated once per week. My app using this library would be working offline, but when a user is online and the new data are available, they will be downloaded. I prefer upserting the data rather than deleting the current DB, because I think that in case of a failed update for any reason, the user should have access to the old (even if half-way updated) data rather than no data, as it will make it impossible for him to search a connection.

I haven't tested this yet on the post-processing as I cannot get the tests working on Windows, so, for the time being, I would leave this PR to your suggestion.

@evelyn9191
Copy link
Contributor Author

Rebased onto master branch.

@rkdarst
Copy link
Member

rkdarst commented Mar 2, 2020

So, I think this is a good idea, assuming it works and makes no other problems. I can't think of any...

Does it work for you with various tests, no corruption from re-importing something that already is in there? If so, let's do it.

@rkdarst
Copy link
Member

rkdarst commented Mar 11, 2020

I think we should do this, it will help someone and won't hurt those doing it the old way. If problems appear later, we can fix or revert.

I just added a changelog entry (and unfortunately had to merge master in - I didn't want to rebase your branch, but feel free to if you want). Could you add a test of some sort - I don't know just what, but you can probably figure out sooner than me. Even a simple thing that tries to load the same file twice would work. Can you think of easy, more advanced tests to add?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants