fix: PostPolicyV4 classes could be improved#442
Conversation
Codecov Report
@@ Coverage Diff @@
## master #442 +/- ##
============================================
+ Coverage 63.17% 64.12% +0.95%
- Complexity 619 623 +4
============================================
Files 32 32
Lines 5133 5177 +44
Branches 489 498 +9
============================================
+ Hits 3243 3320 +77
+ Misses 1726 1693 -33
Partials 164 164
Continue to review full report at Codecov.
|
elharo
left a comment
There was a problem hiding this comment.
Tests should be written before the model code and absolutely in the same PR.
elharo
left a comment
There was a problem hiding this comment.
Comment changes don't need tests but this PR changes behavior. Every behavioral change should be proceeded by a test that fails and only passes after the model code is added.
google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java
Show resolved
Hide resolved
All the tests have been developed. Will much appreciate if you take a look. |
|
|
||
| private PostPolicyV4(String url, Map<String, String> fields) { | ||
| try { | ||
| new URL(url); |
There was a problem hiding this comment.
What URLs do you really want to allow and not allow here? java.net.URL is based on java protocol handlers, and is likely to be either too lenient or too strict depending on what you need.
There was a problem hiding this comment.
I want to allow strings that could be used to construct an URL object and reject strings like 'google.com' given by mistake instead of 'https://google.com'. Catching 'ftp://google.com' is not the goal.
There was a problem hiding this comment.
sounds like new URI(url).isAbsolute() is your best bet then
Fixes #440
(new tests will be suggested in a separate PR)