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

*: Replace implicit hex hash encoding with Algorithm-based encoding #30

Closed
wants to merge 1 commit into from

Conversation

wking
Copy link
Contributor

@wking wking commented Mar 7, 2017

The docs for Algorithm (now algorithm) made it clear that the algorithm identifier was intended to cover both the hash and encoding algorithms. @stevvooe confirmed this interpretation in recent comments as well. The idea is that a future algorithm may chose a non-hex encoding like base 64.

The previous implementation, on the other hand, baked the hex encoding into key locations (e.g. in NewDigestFromBytes and Digest.Validate). This commit makes the encoding part of Agorithm instances, adding a new Encoding interface and an Algorithm.Encoding method. In order support external algorithms with different encoding, I've defined an Algorithm interface with the public API and made renamed the local implementation of that interface to algorithm. And now that Algorithm is a stand-alone interface, I've made the identifier-to-Algorithm registry public by renaming algorithms to Algorithm.

I've also split out the flag binding into its own AlgorithmFlag structure, because the old Algorithm.Set pointer assignment no longer works now that Algorithm is an interface. Having a separate interface for the flag.Value also simplifies the Algorithm interface, which makes it easier to write external Algorithm implementations while still benefiting from the flag.Value helper.

API changes:

  • Additions

    • Algorithms, a newly-public registry of known algorithm identifiers.
    • Algorithm.HashSize, which allows us to avoid a previously-hardcoded hex assumption in Digest.Validate.
    • Algorithm.Encoding, which allows us to implement NewDigester and avoid a previously-hardcoded hex assumption in NewDigestFromBytes.
    • Digest.Hash, as a better name for Digest.Hex.
    • Encoding, a new interface backing Algorithm.Encoding.
    • Hex, the lowercase base 16 Encoding.
    • NewDigester, as a more flexible replacement for NewDigest.
    • NewDigestFromHash, as a better name for NewDigestFromHex.
  • Adjustments

    • Algorithm.Hash now returns nil on unavailable algorithms, to mirror the old API for Algorithm.Digester.
  • Deprecations

    • NewDigest, because NewDigester is a more flexible form of the same thing that is just about as compact.
    • NewDigestFromHex, because NewDigestFromHash is a better name for this function.
    • Digest.Hex, because Hash is a better name for this method.
  • Removals

    • Algorithm.Set, because it is not possible to support this now that Algorithm is an interface.

I also switched to black-box testing (with digest_test package names) because I was getting Hex-undefined errors with white-box testing, and we don't actually need any white-box access in our test suite.

The diff here is fairly large, so if maintainers want me to break this up into smaller chunks (e.g. one PR adding the Encoding interface, another adding an Algorithm.Encoding method, etc. or whatever), just let me know. If the adjustments and removals are not acceptable, we can avoid them with creative naming. But some compatibility issues may be inevitable with the change from structs to interfaces in the Digest.Algorithm() return value.

The docs for 'Algorithm' (now 'algorithm') made it clear that the
algorithm identifier was intended to cover both the hash and encoding
algorithms.  Stephen confirmed this interpretation in recent comments
[1,2] as well.  The idea is that a future algorithm may chose a
non-hex encoding like base 64 [1].

The previous implementation, on the other hand, baked the hex encoding
into key locations (e.g. in NewDigestFromBytes and Digest.Validate).
This commit makes the encoding internal to Agorithm instances, adding
a new Encoding interface and an Algorithm.Encoding method.  In order
support external algorithms with different encoding, I've defined an
Algorithm interface with the public API and made renamed the local
implementation of that interface to 'algorithm'.  And now that
Algorithm is a stand-alone interface, I've made the
identifier-to-Algorithm registry public by renaming 'algorithms' to
'Algorithm'.

I've also split out the flag binding into its own AlgorithmFlag
structure, because the old Algorithm.Set pointer assignment no longer
works now that Algorithm is an interface.  Having a separate interface
for the flag.Value also simplifies the Algorithm interface, which
makes it easier to write external Algorithm implementations while
still benefiting from the flag.Value helper.

API changes:

* Additions

  * Algorithms, a newly-public registry of known algorithm
    identifiers.
  * Algorithm.HashSize, which allows us to avoid a
    previously-hardcoded hex assumption in Digest.Validate().
  * Algorithm.Encoding, which allows us to implement NewDigester and
    avoid a previously-hardcoded hex assumption in NewDigestFromBytes.
  * Digest.Hash, as a better name for Digest.Hex.
  * Encoding, a new interface backing Algorithm.Encoding.
  * Hex, the lowercase base 16 Encoding.
  * NewDigester, as a more flexible replacement for NewDigest.
  * NewDigestFromHash, as a better name for NewDigestFromHex.

* Adjustments

  * Algorithm.Hash now returns nil on unavailable algorithms, to
    mirror the old API for Algorithm.Digester.

* Deprecations

  * NewDigest, because NewDigester is a more flexible form of the same
    thing that is just about as compact.
  * NewDigestFromHex, because NewDigestFromHash is a better name for
    this function.
  * Digest.Hex, because Hash is a better name for this method.

* Removals

  * Algorithm.Set, because it is not possible to support this now that
    Algorithm is an interface.

I also switched to black-box testing (with digest_test package names)
because I was getting Hex-undefined errors with white-box testing, and
we don't actually need any white-box access in our test suite.

[1]: opencontainers#3 (comment)
[2]: opencontainers#6 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@stevvooe
Copy link
Contributor

stevvooe commented Mar 7, 2017

Please open up an issue to discuss design before submitting massive PRs.

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.

None yet

2 participants