Add JSONDecodeError handling for get_lockfile_hash #2629
Conversation
pipenv/project.py
Outdated
| lockfile = self.load_lockfile(expand_env_vars=False) | ||
| except json.decoder.JSONDecodeError: | ||
| # Lockfile corrupted | ||
| return |
There was a problem hiding this comment.
Better to return an empty string here. A lockfile without hash entry can be treated as corrupt as well.
Also JSONDecodeError was added in 3.5, and not available on 2.7. Catch ValueError instead.
pipenv/core.py
Outdated
| verbose=verbose, | ||
| write=True, | ||
| pypi_mirror=pypi_mirror, | ||
| ) |
There was a problem hiding this comment.
This do_lock call can be merged with the other branch below.
|
The logic looks good to me in general, thanks for the work. I made some comments on how the implementation can be improved. |
uranusjr
added
the
PR: awaiting-merge
pipenv/core.py
Outdated
| ) | ||
| err=True | ||
| ) | ||
| os.remove(project.lockfile_location) |
There was a problem hiding this comment.
Wait why are we deleting this?
There was a problem hiding this comment.
We intentionally don't explicitly delete lockfiles unless we are finished re-locking and have a new one ready to replace them with. There is never a scenario where it is OK to just delete a lockfile -- it should all be handled in the project internals. Other than that one line I'm good with the changes, but @uranusjr wouldn't this have been addressed by introducing the lockfile decoder?
There was a problem hiding this comment.
In this particular case I feel it’s fine to delete the lock file because it is corrupted, but either way would do.
There was a problem hiding this comment.
'Corrupted' as in doesn't have a hash in it? Doesn't have content? How does this state occur? Either way I really can't see any reason why we would ever forcibly delete the only file in pipenv we made special functionality to ensure never gets removed without being replaced.
This is super inconsistent. If there is a weird problem in a lockfile users should have a chance to modify or mess with or backup their lockfile unless they successfully ran a new lock, which should be the only thing in all of pipenv that removes lockfiles
There was a problem hiding this comment.
Or we can move the original lock file to Pipfile.lock.bak or with timestamps to prevent duplicated, then recreate a lock file and notice the user this situation?
There was a problem hiding this comment.
No. Do not manipulate the lockfile. We already have tools that do this correctly when you lock. I see no reason to complicate the process with extra manipulation when we already manage all of this. This is a landmine that secretly deletes something that would already be deleted during locking if it succeeded. If it didn’t succeed it would be a lot more helpful if we don’t randomly manipulate it in just this one spot.
There was a problem hiding this comment.
For my particular context “corrupted” means the file cannot be JSON decoded. But the os.remove line is not strictly necessary either way. I’m +0 on removing it.
techalchemy
removed
the
PR: awaiting-merge
|
I’m considering the example where a user makes a manual change and leaves a hanging comma or some other formatting issue. The whole reason we took the approach we did was to avoid deleting this file. I won’t approve any change that deletes it. If I were a user and you deleted my lockfile when instead I could have just fixed the thing I broke, I would be really annoyed |
Now will return None when facing JSONDecodeError when loading the lockfile. do_init will remove the old lockfile and replace it.
uranusjr
force-pushed
the
get_lockfile_hash_with_bad_lockfile
branch
from
fa9bff6
to
bd13183
Compare
uranusjr
added
the
PR: awaiting-merge
Now will return None when facing JSONDecodeError when loading the lockfile.
do_init will remove the old lockfile and replace it.
Fix #2607