Normalize Object Key when Getting Resources #19

michaelahlers · 2017-09-22T16:19:04Z

Possible solution for ohnosequences/sbt-s3-resolver#52.

laughedelic

Hey @michaelahlers! Even though this is not where the problem comes from (see ohnosequences/sbt-s3-resolver#52 (comment)), I still think this is a valuable improvement here. Let's apply it to all relevant methods in S3Repository:

get (source arg)
list (parent arg)
put (destination arg)

P.S. Also if you don't have time for it, tell me. No problem, I'll do it myself.

laughedelic · 2017-09-23T12:38:37Z

src/main/java/ohnosequences/ivy/S3Repository.java

@@ -117,6 +117,8 @@ public void get(String source, File destination) {
  }

  public Resource getResource(String source) {
+    /* For ohnosequences/sbt-s3-resolver#52, has the effect of cleaning up redundant path delimiters (thereby fixing invalid S3 object keys). */
+    source = java.net.URI.create(source).normalize().toString();


Take a look at the S3Utils.getUri method:

https://github.com/michaelahlers/ivy-s3-resolver/blob/df2ffd3dc5010139b9ac0276646dc301dd1a7bd5/src/main/java/ohnosequences/ivy/S3Utils.java#L50-L56

You can add there .normalize() and then this change will propagate to the getBucket, getKey methods used in S3Repository.put.

And then here in getResource you can also use it as S3Utils.getUri(source).toString().

michaelahlers · 2017-09-24T18:25:16Z

@laughedelic, found a little time today, so I'll try wrapping this up!

michaelahlers · 2017-09-24T18:33:02Z

@laughedelic, any objection to adding a (Scala-fluent) unit test framework to this project?

laughedelic · 2017-09-24T18:40:44Z

@michaelahlers of course, you are more than welcome to add tests here!

This library was maintained by my colleague before, and since it was handed to me, I was keeping the changes to minimum. I was actually thinking recently to drop this dependency in sbt-s3-resolver and just write an analogous resolver in Scala. But I don't know yet when I will have time for doing it...

michaelahlers · 2017-09-24T18:52:34Z

src/test/scala/ohnosequences/ivy/S3UtilsSpec.scala

+    val scenarios =
+      ("s3://bucket/p0/p1", ("bucket", "p0/p1")) ::
+        ("s3://bucket/p0//p1", ("bucket", "p0/p1")) ::
+        Nil


For now, only includes basic and ohnosequences/sbt-s3-resolver#52 cases.

…rict

michaelahlers · 2017-09-24T21:52:04Z

build.sbt

+  "com.github.pathikrit" %% "better-files" % "2.17.1" % Test,
+  "org.scalamock" %% "scalamock-scalatest-support" % "3.6.0" % Test,
+  "org.scalatest" %% "scalatest" % "3.0.1" % Test
+)


Add test libraries with helpful support, and make Wartremover less strict from "ohnosequences" % "nice-sbt-settings" % "0.8.0" settings.

michaelahlers · 2017-09-24T21:52:56Z

src/main/java/ohnosequences/ivy/S3Repository.java

+    boolean serverSideEncryption,
+    StorageClass storageClass
+  ) {
+    this.s3Client = s3Client;


An unfortunate conceit. This is unrelated to the fix, but it makes life significantly easier to support injection of (mocked) AmazonS3 instances when S3Repository is under test.

I think it could be actually useful. It allows you to make whatever special S3 client directly with the Java SDK (being able to tune things besides credentials and region) and use resolver with it.
I don't know if it makes sense to make this constructor public right now if there's no explicit need, but it could be done in some future release.

michaelahlers · 2017-09-24T21:53:17Z

src/main/java/ohnosequences/ivy/S3Utils.java

@@ -49,7 +49,8 @@ public static String getKey(String uri) {

  private static URI getUri(String uri) {
    try {
-      return new URI(uri);
+      /* For ohnosequences/sbt-s3-resolver#52, has the effect of cleaning up redundant path delimiters (thereby fixing invalid S3 object keys). */
+      return new URI(uri).normalize();


Crux of the fix. Everything ultimately comes through here.

michaelahlers · 2017-09-24T21:53:46Z

src/test/scala/ohnosequences/ivy/S3MockableRepository.scala

+  acl,
+  serverSideEncryption,
+  storageClass
+)


Help make tests more concise where all they care about is the client.

michaelahlers · 2017-09-24T21:54:55Z

src/test/scala/ohnosequences/ivy/S3RepositorySpec.scala

+    }
+  }
+
+}


Test cases for each API mentioned in the review.

michaelahlers · 2017-09-24T21:55:24Z

src/test/scala/ohnosequences/ivy/S3ResourceSpec.scala

+
+  }
+
+}


This is used as a delegate by S3Repository.get, so ensure it's behavior is correct.

michaelahlers · 2017-09-24T21:56:57Z

src/test/scala/ohnosequences/ivy/S3UtilsSpec.scala

+
+  }
+
+}


Redundant, yes, but it'd take significant refactoring to remove various assumptions about URI parsing.

michaelahlers · 2017-09-24T21:57:18Z

src/test/scala/ohnosequences/ivy/Scenarios.scala

+      ("s3://bucket//path0//path1", ("bucket", "path0/path1")) ::
+      Nil
+
+}


Hard-coding these isn't my favorite thing, but it's a simple way to get scenarios we want to address with this pull-request.

michaelahlers · 2017-09-24T22:00:56Z

@laughedelic, ready for another review. There's a lot added to src/test/scala here, but I figured aggressively adding test coverage to essential functionality couldn't hurt while covering the specific bug. A lot of this test code could be removed with the introduction of a type representing normalized and validated S3 object identifiers, but that may be done in another PR (or not at all depending on whether or not this library's scrapped). Codacy isn't happy with a few ScalaMock conventions, so I'll try cleaning those up in a bit.

michaelahlers · 2017-09-24T22:11:20Z

src/test/scala/ohnosequences/ivy/S3RepositorySpec.scala

+                  summary
+                }
+              }
+              (listing.getNextMarker _).expects().once().returns(null)


Codacy rejects on this, but returning null is a necessary behavior of the com.amazonaws.services.s3.model.ObjectListing#getNextMarker API.

No problem with it. I told it to ignore this particular place.

laughedelic · 2017-09-24T23:51:23Z

@michaelahlers wow! 💯 I didn't expect that you will write tests for other parts besides getKey/getBucket. This is such a great contribution! I'm going to review the code tomorrow as soon as a have time.

laughedelic

Great! Thanks a lot @michaelahlers! 👏

I'm going to merge it and cut a new release quickly to bring the fix to the sbt plugin.

laughedelic · 2017-09-25T10:57:27Z

src/main/java/ohnosequences/ivy/S3Repository.java

+    boolean serverSideEncryption,
+    StorageClass storageClass
+  ) {
+    this.s3Client = s3Client;


I think it could be actually useful. It allows you to make whatever special S3 client directly with the Java SDK (being able to tune things besides credentials and region) and use resolver with it.
I don't know if it makes sense to make this constructor public right now if there's no explicit need, but it could be done in some future release.

laughedelic · 2017-09-25T12:26:36Z

Released in v0.12.0

I also added you to the collaborators. In case you'll want to hack around here some more, you're welcome to submit PRs directly 😉

michaelahlers · 2017-09-25T13:56:19Z

@laughedelic, what can I say? I love writing test code. 😉 You're welcome, I'm glad to help, and thanks for the invitation. (Accepted!) Your S3 resolver plugin has been extremely useful to me on a number of projects both professional and personal. Pretty thrilled to contribute back any way I can!

Ensure paths with no redundant delimiters

4036426

michaelahlers mentioned this pull request Sep 22, 2017

Redundant Object Key Slashes Preventing Resolution ohnosequences/sbt-s3-resolver#52

Closed

Document the normalization

9c58414

laughedelic self-requested a review September 22, 2017 19:48

laughedelic suggested changes Sep 23, 2017

View reviewed changes

laughedelic reviewed Sep 23, 2017

View reviewed changes

Michael Ahlers added 2 commits September 24, 2017 12:50

Include ScalaTest

ea7b43a

Verify bucket name and object key extraction (breaking change; TDD)

0d32007

michaelahlers commented Sep 24, 2017

View reviewed changes

Michael Ahlers added 13 commits September 24, 2017 14:28

Add ScalaMock and Better Files to tests, and make Wartremover less st…

edf06c7

…rict

Normalize the URI before returning (fixes tests)

277e832

Add constructor so the S3 client may be mocked under test

b02351e

Add tests for two key methods of the S3Repository

ad9efc6

Pull out the mockable repo. impl. for reuse

ff877b8

Authority must be alphanumeric characters

63a3992

Make this more readable

1f03e85

Restrict scope of mockable Resource and simplify

33de6e5

Verify initialization with normalized object keys

a4bf96e

Verify list behaviors

32fa7e2

Clean up comment and TODOs

ad00f76

DRY and expand cases

a510a50

Verify put object

35c961e

michaelahlers commented Sep 24, 2017

View reviewed changes

src/test/scala/ohnosequences/ivy/S3RepositorySpec.scala

}

}

}

Copy link

Author

michaelahlers Sep 24, 2017

Choose a reason for hiding this comment

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

Test cases for each API mentioned in the review.

michaelahlers commented Sep 24, 2017

View reviewed changes

Make compliant with Codacy recommendations

02d5c3b

michaelahlers commented Sep 24, 2017

View reviewed changes

laughedelic approved these changes Sep 25, 2017

View reviewed changes

laughedelic merged commit 5988633 into ohnosequences:master Sep 25, 2017

laughedelic mentioned this pull request Sep 25, 2017

Update to ivy-s3-resolver with the redundant delimiters fix ohnosequences/sbt-s3-resolver#53

Merged

michaelahlers deleted the normalize-object-keys branch September 25, 2017 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize Object Key when Getting Resources #19

Normalize Object Key when Getting Resources #19

michaelahlers commented Sep 22, 2017 •

edited

Loading

laughedelic left a comment •

edited

Loading

laughedelic Sep 23, 2017 •

edited

Loading

michaelahlers commented Sep 24, 2017

michaelahlers commented Sep 24, 2017

laughedelic commented Sep 24, 2017

michaelahlers Sep 24, 2017

michaelahlers Sep 24, 2017

michaelahlers Sep 24, 2017

laughedelic Sep 25, 2017

michaelahlers Sep 24, 2017

michaelahlers Sep 24, 2017

michaelahlers Sep 24, 2017

michaelahlers Sep 24, 2017

michaelahlers Sep 24, 2017

michaelahlers Sep 24, 2017 •

edited

Loading

michaelahlers commented Sep 24, 2017 •

edited

Loading

michaelahlers Sep 24, 2017

laughedelic Sep 24, 2017

laughedelic commented Sep 24, 2017

laughedelic left a comment

laughedelic Sep 25, 2017

laughedelic commented Sep 25, 2017

michaelahlers commented Sep 25, 2017

+                  }
+                }
+              }

Normalize Object Key when Getting Resources #19

Normalize Object Key when Getting Resources #19

Conversation

michaelahlers commented Sep 22, 2017 • edited Loading

laughedelic left a comment • edited Loading

Choose a reason for hiding this comment

laughedelic Sep 23, 2017 • edited Loading

Choose a reason for hiding this comment

michaelahlers commented Sep 24, 2017

michaelahlers commented Sep 24, 2017

laughedelic commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelahlers Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

michaelahlers commented Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laughedelic commented Sep 24, 2017

laughedelic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laughedelic commented Sep 25, 2017

michaelahlers commented Sep 25, 2017

michaelahlers commented Sep 22, 2017 •

edited

Loading

laughedelic left a comment •

edited

Loading

laughedelic Sep 23, 2017 •

edited

Loading

michaelahlers Sep 24, 2017 •

edited

Loading

michaelahlers commented Sep 24, 2017 •

edited

Loading