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

Bugfix/cannot save file with long URL key #34

Merged
merged 8 commits into from
Apr 26, 2016

Conversation

attila-at-hyper
Copy link
Contributor

If you want to store an image (using Imaginary) in the disk storage and use the file’s remote URL as key, saving will fail if the URL is long. This PR tries to fix the issue by using the URL’s MD5 digest as file name instead of the Base64 encoded version of the URL

@@ -18,4 +18,5 @@ Pod::Spec.new do |s|
s.tvos.source_files = 'Source/{iOS,Shared}/**/*'

s.frameworks = 'Foundation'
s.dependency 'MD5'
Copy link
Contributor

Choose a reason for hiding this comment

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

We could reconsider the source of MD5 generation later and go wth this library to fix the current bug. What do you think @zenangst ?

Copy link
Contributor

Choose a reason for hiding this comment

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

That works

@vadymmarkov
Copy link
Contributor

I think tests are broken now. It needs to be updated with this change.

@attila-at-hyper
Copy link
Contributor Author

@vadymmarkov tests fail because the MD5 pod is not installed by Travis. I've added the MD5 dependency to the podspec and recently tried to add it to a new Carthage file, but none of those helped. Any hints? Sorry, I'm not fluent in Travis... cc @zenangst

@vadymmarkov
Copy link
Contributor

Have you tried to run test @attila-at-hyper ? Because we have this one that needs to be changed I believe https://github.com/hyperoslo/Cache/blob/master/Tests/iOS/Specs/Storage/DiskStorageSpec.swift#L181

I'm not sure that MD5 is available via Carthage and it might be a problem. If it's a case it's better to use https://github.com/krzyzanowskim/CryptoSwift instead.

@attila-at-hyper
Copy link
Contributor Author

@vadymmarkov: I've switched to CryptoSwift since MD5 is indeed not available via Carthage. Local build seems to be working fine and I'm currently waiting for Travis to complete its run to see if it builds well there, too.

I'm having some issues with running the tests though: after hitting Cmd-U tests are seemingly built and fail (as expected), however when I try to check which tests have failed, I see no tests at all (see below). Please advise. Once I have the tests up and running, I'll fix the mentioned test.

diskstoragespec_swift

@attila-at-hyper
Copy link
Contributor Author

@vadymmarkov: I think I've found the reason for the test failure. Working on it.

@attila-at-hyper
Copy link
Contributor Author

@vadymmarkov problem found and fixed. Tests now work fine locally. Waiting for Travis... Mac build currently fails because for some reason the compiler can't find the CryptoSwift module. Is that a killer issue for now?

@attila-at-hyper
Copy link
Contributor Author

@vadymmarkov: never mind, I've managed to fix the Mac test, too. Waiting for Travis to complete.

@vadymmarkov
Copy link
Contributor

@attila-at-hyper All tests have passed. Great work @attila-at-hyper, gonna merge it!

@vadymmarkov vadymmarkov merged commit 8b02837 into master Apr 26, 2016
@vadymmarkov vadymmarkov deleted the bugfix/cannot-save-file-with-long-URL-key branch April 26, 2016 19:54
@zenangst
Copy link
Contributor

Should we make a new release?

@attila-at-hyper
Copy link
Contributor Author

@zenangst: yeah, I think that would make sense. This change does make a difference for Imaginary.

@onmyway133
Copy link
Contributor

onmyway133 commented May 8, 2016

I've made CommonCrypto wrapper https://github.com/onmyway133/CommonCrypto.swift in case CryptoSwift is overkill 😄 . It works now 😬

@onmyway133
Copy link
Contributor

@vadymmarkov @zenangst updated again 😄 You may want to take a look https://github.com/onmyway133/SwiftHash

@vadymmarkov
Copy link
Contributor

@onmyway133 Feel free to make a PR with your new library 😉

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.

4 participants