Add OpenSSL::DigestIO to wrap an IO while calculating a digest #4260

spalladino · 2017-04-09T16:24:43Z

Wraps an IO by calculating a specified digest on read and/or write operations

Example

require "openssl"

underlying_io = IO::Memory.new("foo")
io = OpenSSL::DigestIO.new(underlying_io, "SHA256")
buffer = Bytes.new(256)
io.read(buffer)
io.digest # => 2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae

RX14 · 2017-04-09T16:39:47Z

src/openssl/digest/digest_io.cr

+    def read(slice : Bytes)
+      read_bytes = io.read(slice)
+      if @digest_on_read
+        digest_algorithm.update(Bytes.new(slice.to_unsafe, read_bytes, read_only: true))


The usual way to do this is slice[0, read_bytes] instead of using Bytes.new and to_unsafe.

Good point. Fixed.

RX14 · 2017-04-09T16:41:29Z

src/openssl/digest/digest_io.cr

+    def initialize(@io : IO, @digest_algorithm : OpenSSL::Digest, *, @digest_on_read = true, @digest_on_write = true)
+    end
+
+    def initialize(@io : IO, algorithm : String, *, @digest_on_read = true, @digest_on_write = true)


I'm not sure I see a usecase for hashing read and write to the same hash, and it could cause some confusion. Maybe it would be best to make this exclusive (read or write digest).

Neither do I. I've changed both flags in favour of an enum Read/Write.

RX14 · 2017-04-09T16:42:23Z

src/openssl/digest/digest_io.cr

+    getter digest_on_write : Bool
+
+    delegate close, closed?, flush, peek, tty?, rewind, to: @io
+    delegate digest, to: @digest_algorithm


What about hexdigest and base64digest ?

Sija · 2017-04-09T18:22:17Z

src/openssl/digest/digest_io.cr

@@ -0,0 +1,55 @@
+require "./digest_base"
+
+# Wraps an IO by calculating a specified digest on read and/or write operations


Docs mention and, whereas right now it's either read or write.

It's amazing how fast docs can become outdated. Thanks for the attention to detail!

RX14 · 2017-04-09T18:08:01Z

src/openssl/digest/digest_io.cr

    delegate digest, hexdigest, base64digest, to: @digest_algorithm

-    def initialize(@io : IO, @digest_algorithm : OpenSSL::Digest, *, @digest_on_read = true, @digest_on_write = true)
+    enum DigestMode


Maybe rename this as just Mode as the surrounding class gives context. Maybe also bring out the members to the class level, so you only have to reference OpenSSL::DigestIO::Read.

RX14 · 2017-04-09T19:57:29Z

src/openssl/digest/digest_io.cr

+    delegate digest, hexdigest, base64digest, to: @digest_algorithm
+
+    enum DigestMode
+      Read,


Why a comma here? (I think the formatter should remove this)

RX14 · 2017-04-09T20:00:27Z

Also, maybe fix the typo (s/:/::/) in the commit message?

spalladino · 2017-04-10T11:38:45Z

@RX14 done and done!

Sija · 2017-04-10T11:41:17Z

src/openssl/digest/digest_io.cr

@@ -0,0 +1,55 @@
+require "./digest_base"
+
+# Wraps an IO by calculating a specified digest on read or write operations


Missing dot ending the sentence :)

Sija · 2017-04-10T11:41:48Z

io_deferred.cr

@@ -0,0 +1,175 @@
+module IO


Debug leftovers (along with the binary)?

Sija · 2017-04-10T11:42:01Z

foo.cr

@@ -0,0 +1,5 @@
+# puts "A"


Debug leftovers?

Certainly. Fixing now.

RX14 · 2017-04-10T12:07:17Z

This PR is going to be perfect after all these reviews haha

Sija · 2017-04-10T15:07:21Z

src/openssl/digest/digest_io.cr

+# io.read(buffer)
+# io.digest # => 2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae
+# ```
+#


Redundant empty comment line :)

luislavena · 2017-04-10T16:49:26Z

src/openssl/digest/digest_io.cr

+
+    def read(slice : Bytes)
+      read_bytes = io.read(slice)
+      if @mode == DigestMode::Read


Why not use @mode.read? Enum helper instead?

luislavena · 2017-04-10T16:49:41Z

src/openssl/digest/digest_io.cr

+    end
+
+    def write(slice : Bytes)
+      if @mode == DigestMode::Write


Why not use @mode.write? Enum helper instead?

luislavena

I think the only remaining piece here is the documentation block (and a rebase against latest master)

😄

luislavena · 2017-05-10T22:56:59Z

src/openssl/digest/digest_io.cr

@@ -0,0 +1,54 @@
+require "./digest_base"
+
+# Wraps an IO by calculating a specified digest on read or write operations.


Please move the documentation block to the DigestIO class instead. This will avoid OpenSSL module receive the digest-related documentation.

spalladino · 2017-05-11T15:54:18Z

Awesome! Will do that next week. I've fried my Mac's HD and are setting everything back up :-P

spalladino · 2017-05-18T12:44:17Z

@luislavena done! Rebase can be done without conflicts as well.

spalladino · 2017-06-02T14:58:12Z

Thanks @mverzilli!

spalladino added the pr:needs-review label Apr 9, 2017

RX14 requested changes Apr 9, 2017

View reviewed changes

spalladino force-pushed the feature/digest_io branch from 01d70ea to 75d7195 Compare April 9, 2017 18:03

Sija reviewed Apr 9, 2017

View reviewed changes

spalladino force-pushed the feature/digest_io branch from 75d7195 to 39e571d Compare April 9, 2017 19:38

RX14 requested changes Apr 9, 2017

View reviewed changes

RX14 reviewed Apr 9, 2017

View reviewed changes

spalladino force-pushed the feature/digest_io branch from 39e571d to 5bda0ef Compare April 10, 2017 11:37

Sija reviewed Apr 10, 2017

View reviewed changes

io_deferred.cr Outdated

@@ -0,0 +1,175 @@

module IO

Copy link

Contributor

Sija Apr 10, 2017

Choose a reason for hiding this comment

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

Debug leftovers (along with the binary)?

Sija reviewed Apr 10, 2017

View reviewed changes

Add OpenSSL::DigestIO to wrap an IO while calculating a digest

d2d61e8

spalladino force-pushed the feature/digest_io branch from 5bda0ef to 5658d21 Compare April 10, 2017 12:03

spalladino changed the title ~~Add OpenSSL:DigestIO to wrap an IO while calculating a digest~~ Add OpenSSL::DigestIO to wrap an IO while calculating a digest Apr 10, 2017

Sija reviewed Apr 10, 2017

View reviewed changes

luislavena reviewed Apr 10, 2017

View reviewed changes

spalladino force-pushed the feature/digest_io branch from 5658d21 to 944e148 Compare April 10, 2017 23:27

luislavena suggested changes May 10, 2017

View reviewed changes

Change DigestIO digest on read or write flags for exclusive mode enum

979d6c4

spalladino force-pushed the feature/digest_io branch from 944e148 to 979d6c4 Compare May 18, 2017 12:42

matiasgarciaisaia self-requested a review May 20, 2017 05:11

matiasgarciaisaia approved these changes May 20, 2017

View reviewed changes

RX14 approved these changes May 20, 2017

View reviewed changes

mverzilli merged commit 66ac243 into crystal-lang:master Jun 2, 2017

mverzilli added the topic:stdlib label Jun 2, 2017

mverzilli removed the pr:needs-review label Jun 2, 2017

mverzilli added this to the Next milestone Jun 2, 2017

Sija referenced this pull request Jun 27, 2017

Crystal 0.23.0 Changelog

3c3d3e2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenSSL::DigestIO to wrap an IO while calculating a digest #4260

Add OpenSSL::DigestIO to wrap an IO while calculating a digest #4260

spalladino commented Apr 9, 2017

RX14 Apr 9, 2017

spalladino Apr 9, 2017

RX14 Apr 9, 2017

spalladino Apr 9, 2017

RX14 Apr 9, 2017

spalladino Apr 9, 2017

Sija Apr 9, 2017

spalladino Apr 9, 2017

RX14 Apr 9, 2017

RX14 Apr 9, 2017

RX14 commented Apr 9, 2017

spalladino commented Apr 10, 2017

Sija Apr 10, 2017

Sija Apr 10, 2017

Sija Apr 10, 2017

spalladino Apr 10, 2017

RX14 commented Apr 10, 2017

Sija Apr 10, 2017

luislavena Apr 10, 2017

luislavena Apr 10, 2017

luislavena left a comment

luislavena May 10, 2017

spalladino commented May 11, 2017

spalladino commented May 18, 2017

spalladino commented Jun 2, 2017

		@@ -0,0 +1,55 @@
		require "./digest_base"

		# Wraps an IO by calculating a specified digest on read and/or write operations

		@@ -0,0 +1,54 @@
		require "./digest_base"

		# Wraps an IO by calculating a specified digest on read or write operations.

Add OpenSSL::DigestIO to wrap an IO while calculating a digest #4260

Add OpenSSL::DigestIO to wrap an IO while calculating a digest #4260

Conversation

spalladino commented Apr 9, 2017

Example

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

Choose a reason for hiding this comment

RX14 commented Apr 9, 2017

spalladino commented Apr 10, 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

RX14 commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luislavena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino commented May 11, 2017

spalladino commented May 18, 2017

spalladino commented Jun 2, 2017