Skip to content

File creation fix#427

Merged
vbuberen merged 14 commits into
ChuckerTeam:developfrom
MiSikora:file-creation-fix
Aug 25, 2020
Merged

File creation fix#427
vbuberen merged 14 commits into
ChuckerTeam:developfrom
MiSikora:file-creation-fix

Conversation

@MiSikora

@MiSikora MiSikora commented Aug 17, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

📄 Context

There are open issues - #422, #414.

📝 Changes

FileFactory is now IO safe and typesafe. I removed the interface in favor of a concrete implementation. It was internal anyway and it was misleading in tests of ChuckerInterceptor because fake implementation did not take into account stuff like createNewFile(), that was the cause of #422.

One more thing that I changed in the implementation of the factory is that access to the parent directory. It is now lazy, so it should help with #413 since the cache directory will no longer be accessed during initialization. It won't fix it as there are other files being used on the main thread but it is one step forward.

I'm not particularly happy about the fact that TeeSource accepts now File? as an input parameter but I didn't want to overcomplicate things by introducing another source that just counts bytes downloaded. - This isn't the case anymore. See #427 (comment).

📎 Related PR

I copied the test for the file factory from #423.

🚫 Breaking

No.

🛠️ How to test

In practice not sure. Maybe @CodeBreak524 can test with a dependency from my branch on JitPack (com.github.MiSikora.chucker:library:file-creation-fix-SNAPSHOT). I didn't write new tests for ChuckerInterceptor as old ones cover this case due to using real file factory in tests.

⏱️ Next steps

Closes #422
Closes #414
Closes #423

koral--
koral-- reviewed Aug 17, 2020
View reviewed changes
Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt Outdated
vbuberen
vbuberen reviewed Aug 19, 2020
View reviewed changes

@vbuberen vbuberen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for such a good change. Looks good to me.
Only a few minor comments with suggestions for micro-optimizations.

Also, I see a typo in PR description with wrong PR mentioned in the first part in that was the cause of #420. Please update it as well to have proper history descriptions.

Comment thread library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt Outdated
Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt Outdated
@MiSikora

MiSikora commented Aug 19, 2020

Copy link
Copy Markdown
Contributor Author

Ok, I applied your suggestions. I would appreciate it if you can give feedback on #427 (comment) from @koral-- and how you'd like to handle it.

@MiSikora MiSikora marked this pull request as draft August 20, 2020 12:26
@MiSikora

MiSikora commented Aug 20, 2020

Copy link
Copy Markdown
Contributor Author

Coverted it to a draft because I discovered some issues.

@MiSikora MiSikora marked this pull request as ready for review August 20, 2020 12:45
@MiSikora

MiSikora commented Aug 20, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

In cbedcbc I decoupled tee operation from reporting functionality. Reporting is now a part of the ReportingSink class. My main motivation was that TeeSource started to become too complex and had too many responsibilities.

I can remove it from this PR and make a different one with just this commit if you want me to. Or remove it completely but I'm happy now with how reporting bytes and copying bytes is no longer coupled.

vbuberen
vbuberen reviewed Aug 21, 2020
View reviewed changes

@vbuberen vbuberen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, looks good to me.

Replied to mentioned comment as well.

Comment thread library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt Outdated
Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt Outdated
Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt Outdated
Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt Outdated
Comment thread library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt Outdated
Comment thread library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt Outdated
Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt Outdated
koral--
koral-- reviewed Aug 21, 2020
View reviewed changes
Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt Outdated
@MiSikora MiSikora mentioned this pull request Aug 22, 2020
Closed
2 tasks
cortinico
cortinico reviewed Aug 23, 2020
View reviewed changes
Comment thread library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt Outdated
Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt Outdated
Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt Outdated
Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt Outdated
@MiSikora

MiSikora commented Aug 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I added the suggested changes. CI fails for now because I added a functional interface and #438 needs to be merged first. I'll rebase after. Unless you think that () -> File? and a type alias will suffice.

Michał Sikora and others added 10 commits August 23, 2020 18:40
@koral-- koral-- mentioned this pull request Aug 23, 2020
Closed
@cortinico

cortinico commented Aug 24, 2020

Copy link
Copy Markdown
Member

CI fails for now because I added a functional interface and #438 needs to be merged first. I'll rebase after. Unless you think that () -> File? and a type alias will suffice.

Since we're on 1.4, let's go with a functional interface 👌

cortinico
cortinico approved these changes Aug 24, 2020
View reviewed changes

@cortinico cortinico left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some nits. Overall the change looks good to me.

Thank you very much for taking the time to do this change @MiSikora 🙏
I'd love to get a pass also from @vbuberen if possible before merging 👌

Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryFactory.kt Outdated
Comment thread library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryFactory.kt Outdated
Comment thread library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt Outdated
@MiSikora

MiSikora commented Aug 24, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Great, thanks! I added one test for a missing cache in the first place.

vbuberen
vbuberen approved these changes Aug 25, 2020
View reviewed changes

@vbuberen vbuberen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@vbuberen vbuberen merged commit 5415940 into ChuckerTeam:develop Aug 25, 2020
@MiSikora MiSikora deleted the file-creation-fix branch August 25, 2020 08:13
@cortinico cortinico added this to the 3.3.0 milestone Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cortinico cortinico cortinico approved these changes

@vbuberen vbuberen vbuberen approved these changes

+1 more reviewer

@koral-- koral-- koral-- left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.3.0

Development

Successfully merging this pull request may close these issues.

IOException when app uses 3rd party SDK Potential NPE during AndroidCacheFileFactory initialization

4 participants

@MiSikora @cortinico @koral-- @vbuberen