Make readAll() resilient to exceptions reading a single file #296
Conversation
|
Ideally I'd prefer the |
|
@digitalbuddha I updated the PR, adding a new method, |
|
@digitalbuddha @ramonaharrison Any chance we could merge this? |
|
So sorry for delay. I wasn't clear with last request. What still needs to be done is adding a builder configuration for when you build the store. This configuration would be to either use the safe read all or the regular one. You would also need to change RealInternalStore to read the flag and properly call the correct persister. Alternatively you can make a new persister who's readAll is the safe version and then pass that persister in. Whichever way you go pls include a test showing usage. |
riclage
force-pushed
the
feature/fix-read-all-individual-crash
branch
from
5dccfac to
40db2db
Compare
riclage
force-pushed
the
feature/fix-read-all-individual-crash
branch
from
40db2db to
fb591af
Compare
|
@digitalbuddha Apologies for the delay in replying. I'm finally having the time to go back to this but it's not quite clear to me what you mean. As far as I understood the code, there is currently no explicit builder configuration for any of the Unless in your comment, did you mean to also add a kind of |
|
hey there @riclage i'm so sorry we dropped the ball on this, i am planning a holiday here shortly and will be doing some store work. i will pick this up where you left off and add to this pr if that is ok with you? i'm looking forwards to getting this in. thank you! |
|
Hi @brianPlummer , that's ok with me. However, I feel like this PR could be merged as is. Your contributions could be made in subsequent PRs. Do you have any particular reasons to make commits to this PR specifically? Thanks! |
|
@brianPlummer Any update on this? |
Currently, if an exception occurs on a
fileSystem.read(s)call inside theFSAllReader.readAll()method, then the whole chain will stop with an onError emission. This prevents us from getting subsequent files on the path.This PR proposes a solution to flat map each individual file read, wrapping its observable. If an exception occurs for a file read, then we return a
ReadResultBufferedSourcefor that file that wraps the exception.Current known issues:
ReadResultBufferedSourceseems like an unnecessary wrapping of a buffer just to fit the contract of theObservable<BufferedSource> readAll()method, which requires aBufferedSource. Ideally, we could return anObservable<ReadResult>instead which wraps either the exception or theBufferedSource. This would be, however, an API breaking change.GsonSourceParser) to returnnullfor an empty buffer. This will throw a NPE on the chain instead of the original exception when reading the file.safeReadAll()that filters out all the results that came from an exception?This PR fixes #293