For custom cassette persisters no longer catch ValueError

[amosjyng]

I had to spend some time digging before I realized that serialization/deserialization errors were being swallowed here

[hartwork]

@amosjyng I see how this change could help on an abstract level. Could you elaborate what concrete practical problem this is fixing for you? What made you want to change this code? Thank in advance! 🙏

[amosjyng]

IIRC I was trying to save different keys to the recorded file, but deserialize(data, serializer) was throwing a ValueError because the keys were not as expected. vcrpy would continually treat the session recording as if it weren't there at all, and it took a little while to debug that it was due to this mismatch between how the session was serialized vs how it was deserialized. Hence, this PR as a suggestion to make such development easier in the future.

[hartwork]

Hi @amosjyng,

thanks for elaborating!

I just found that the documentation at https://vcrpy.readthedocs.io/en/latest/advanced.html#register-your-own-cassette-persister says that…

[t]he load_cassette method must return a deserialized cassette or raise ValueError if no cassette is found.

I think that means that changing except ValueError: would break documented API. If that understanding is correct it would mean that we either (a) cannot change it or (b) need a major version bump to clearly communicate this change.

So we probably need to clarify that first — @jairhenrique @kevin1024 what do you think, does this break the API?

Best, Sebastian

---

Notes to self for later:

• rebase onto latest master to get the CI involved
• class name ExpectedCassetteError not ideal?
• add a simple test using a mock custom cassette persistor

[kevin1024]

1. Yes, this would be a breaking change to a documented API and would require a major version bump
2. Yeah, this probably would be a quality-of-life improvement for those using vcrpy.

My guess is that most folks have not built their own persister, so it should only affect a minority of vcrpy users. Has anyone built their own persister? Honestly IIRC I just thought it was a cool idea for someone to store cassettes in a database or s3 or something but I haven't heard of anyone using it.

[hartwork]

@kevin1024 to be sure I read (2) right: You're pro fixing this and the major version bump?

[kevin1024]

Yeah, but this single change alone probably isn’t worth releasing a version bump in and of itself. Maybe we can batch it up with some other breaking changes like dropping older models python versions and/or old http library versions and do a major bump sometime in the future. What do you think?

[hartwork]

Yeah, but this single change alone probably isn’t worth releasing a version bump in and of itself. Maybe we can batch it up with some other breaking changes like dropping older models python versions and/or old http library versions and do a major bump sometime in the future. What do you think?

@kevin1024 if we go the collect-more-breakage route, we either need a second master branch or we lose instant releasability on the master branch. The latter is a clear non-option to me and former is probably overkill and may hold this for later for an unclear amount of time. Given that the breakage is easy to adjust to, will likely not affect most users (as you mentioned), and will likely show directly where used in the test suite, I would vote for the simple approach and just treat this like a normal pull request except with bumping the version to 5.0.0, and continue as usual. If you like, we can merge #695 dropping Python 3.7 right after and call that the release, if you feel it's in the breaking bucket (— personally, I don't see it as breaking but it can be argued). What do you think?

[kevin1024]

Excellent points, all! I agree with your plan. The only thing this PR needs still is an update to the documentation to mention the new API (different exception)

[hartwork]

Excellent points, all! I agree with your plan.

@kevin1024 cool!

The only thing this PR needs still is an update to the documentation to mention the new API (different exception)

Yes!

I believe in total (integrating my earlier todo-notes from #681 (comment) above) we need:

• a rebase onto latest master (e.g. to bring the CI in)
• probably two(?) new exception classes to cleanly distinct the two likely major/common cases for custom persisters, e.g.
  • CassetteNotFoundError, potentially inheriting from built-in FileNotFoundError
  • CassetteDecodeError, potentially inheriting from ValueError
• a simple test per new exception
• doc fixes at

  vcrpy/docs/advanced.rst

  Line 139 in f4316d2

  ``ValueError`` if no cassette is found.

@amosjyng @kevin1024 what do you think?

PS: I can help out with any of these parts as needed.

[amosjyng]

Sounds great to me! There's no rush on my end, we can wait for whenever the next major version is due. I just opened this PR as a suggestion to hopefully save others in the future some time :)

[hartwork]

Hi @amosjyng I think it's likely that (driven by #695) there will be a natural next release before ~2023-07-01. Let's get started on the ValueError thing then so that we're done by then, I'd be happy to review the next iteration.

[hartwork]

@amosjyng any news on the implementation?

[amosjyng]

Sorry, I thought you guys were talking to each other about making those changes, not to me :P

I've split it into CassetteNotFoundError and CassetteDecodeError as requested. I've added tests for a custom persister that throws those errors, as well as a test for the default persister throwing a CassetteNotFoundError. I'm not sure how to get the default persister to throw the CassetteDecodeError as it's not clear to me how you'd trigger a UnicodeEncodeError while reading a file; should we perhaps be checking for UnicodeDecodeError as well?

[codecov-commenter]

Codecov Report

Merging #681 (12d44d7) into master (7eb235c) will decrease coverage by 0.04%.
The diff coverage is 87.50%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
- Coverage   90.30%   90.26%   -0.04%     
==========================================
  Files          28       28              
  Lines        1774     1778       +4     
  Branches      313      313              
==========================================
+ Hits         1602     1605       +3     
- Misses        137      138       +1     
  Partials       35       35              

Impacted Files	Coverage Δ
vcr/persisters/filesystem.py	96.29% <83.33%> (+4.99%)	⬆️
vcr/cassette.py	96.31% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[hartwork]

@amosjyng thanks for this, looking pretty good already, nice! 👍

I was wondering whether UnicodeEncodeError should actually be UnicodeDecodeError instead also — by now I think it's indeed mixed up. Please see my comments on details below:

[hartwork]

@amosjyng thanks for the adjustments, looks great! 👍

I have some hope that @kevin1024 is with me on releasing 4.4.0 shortly before the merge (i.e. issue #724). Let's see what he thinks; approving the pull request now…

[hartwork]

@amosjyng you probably saw https://github.com/kevin1024/vcrpy/releases/tag/v5.0.0 but… your documentation changes are also live at https://vcrpy.readthedocs.io/en/latest/advanced.html#register-your-own-cassette-persister now. Thanks again! 🙏

[amosjyng]

Woohooo, that's cool to see! Thanks @hartwork ! :)