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

Jump to bottom

fix: PostPolicyV4 classes could be improved #442

Merged
merged 9 commits into from Aug 21, 2020
Merged

fix: PostPolicyV4 classes could be improved #442

merged 9 commits into from Aug 21, 2020

Conversation

dmitry-fa
Copy link
Contributor

@dmitry-fa dmitry-fa commented Jul 29, 2020 •
edited

Fixes #440

(new tests will be suggested in a separate PR)

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 29, 2020
@codecov
Copy link

codecov bot commented Jul 29, 2020 •
edited

Codecov Report

Merging #442 into master will increase coverage by 0.95%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/com/google/cloud/storage/Storage.java 80.64% <ø> (ø) 0.00 <0.00> (ø)
...in/java/com/google/cloud/storage/PostPolicyV4.java 86.33% <100.00%> (+23.67%) 5.00 <0.00> (+1.00)
...src/main/java/com/google/cloud/storage/Bucket.java 82.20% <0.00%> (+0.12%) 34.00% <0.00%> (ø%)
...ain/java/com/google/cloud/storage/StorageImpl.java 81.25% <0.00%> (+0.24%) 147.00% <0.00%> (ø%)
...main/java/com/google/cloud/storage/BucketInfo.java 82.01% <0.00%> (+0.27%) 86.00% <0.00%> (+3.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8a4b12...b5a4f90. Read the comment docs.

@dmitry-fa dmitry-fa requested review from frankyn and elharo July 29, 2020 12:05
elharo
elharo suggested changes Jul 29, 2020
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Tests should be written before the model code and absolutely in the same PR.

@dmitry-fa
Copy link
Contributor Author

@elharo
The issue for new tests: #410

The model code was integrated without proper coverage by tests. For the time being it's covered by 10 conformance tests. This just adds javadoc and fixes minor bugs found.

Before developing new tests I'd like to get feedback on the proposed changes.

elharo
elharo suggested changes Jul 29, 2020
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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
@dmitry-fa dmitry-fa requested a review from elharo August 18, 2020 21:05
@dmitry-fa
Copy link
Contributor Author

@elharo

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.

All the tests have been developed. Will much appreciate if you take a look.

elharo
elharo reviewed Aug 20, 2020
View reviewed changes
google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java Outdated
*/
public final class PostPolicyV4 {
private final String url;
private final Map<String, String> fields;

private PostPolicyV4(String url, Map<String, String> fields) {
try {
new URL(url);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds like new URI(url).isAbsolute() is your best bet then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, fixed.

@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/java-storage API. label Aug 21, 2020
@frankyn frankyn merged commit 8602b81 into googleapis:master Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elharo elharo elharo approved these changes

@JesseLovelace JesseLovelace JesseLovelace approved these changes

@frankyn frankyn Awaiting requested review from frankyn

Assignees
No one assigned
Labels
api: storage Issues related to the googleapis/java-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PostPolicyV4 classes could be improved
4 participants
@dmitry-fa @elharo @JesseLovelace @frankyn