WycliffeAssociates / jdenticon-kotlin

19 stars 8 forks source link

Provide better error for invalid hash arg #5

Closed bseib closed 3 years ago

bseib commented 3 years ago

When calling Jdenticon.toSvg("012345", 80, null), an out of bounds exception is thrown, and the caller doesn't know what to do. Furthermore, passing a hash argument that is not a hexadecimal string, like Jdenticon.toSvg("hello world", 80, null), will throw a number format exception, contrary to the documentation:

        /**
         * Draws an identicon as an SVG string.
         * @param hash - A hexadecimal hash string or any value that will be hashed by Jdenticon.
         * @param size - Icon size in pixels.
         * @param padding - Optional padding in percents. Extra padding might be added to center the rendered identicon.
         * @returns SVG string
         */

The simplest thing we should do is change the documentation to:

        /**
         * Draws an identicon as an SVG string.
         * @param hash - A hexadecimal hash string of at least 11 characters.
         * @param size - Icon size in pixels.
         * @param padding - Optional padding in percents. Extra padding might be added to center the rendered identicon.
         * @returns SVG string
         */

And then validate the hash argument to actually be a hexadecimal string with length at least of at least 11, throwing IllegalArgumentException if not valid, and directly telling the caller what went wrong.

A PR is coming momentarily.

jsarabia commented 3 years ago

@bseib Thank you for discovering this issue and your subsequent pull request!

As this library was originally a port of a Javascript library, https://jdenticon.com/ I feel that the intent and public API behavior of the base library should be preserved as much as possible.

I looked to their codebase: https://github.com/dmester/jdenticon which, admittedly appears to have had refactors since this Kotlin port was made, including a refactor of the entrypoints.

Regardless, their toSvg API is as follows:

export function toSvg(hashOrValue, size, config) {
    const writer = new SvgWriter(size);
    iconGenerator(new SvgRenderer(writer), 
        isValidHash(hashOrValue) || computeHash(hashOrValue),
        config);
    return writer.toString();
}

As such I feel that the proper solution is to hash the input if the provided string is not a hash, rather than require the user of this library to provide a hash as input. Specifically their computeHash function uses sha1 and thus should be used here as well (for purposes of trying to hopefully produce the same identicon in both the kotlin and javascript libraries if that is still realistically attained; the libraries may have diverged too much at this point as this one is not actively developed).

bseib commented 3 years ago

Excellent points with regard to trying to keep parity, not only with the APIs between Kotlin and Javascript, but also their SVG output.

Want me to take another crack at it in this PR? Perhaps I can take a look this weekend.

jsarabia commented 3 years ago

Sure thing! I would appreciate that, if you have the time!

bseib commented 3 years ago

Regardless, their toSvg API is as follows:

Yeah, looks like we want to match this behavior:

https://github.com/dmester/jdenticon/blob/54fb9c1d1d66d5eb6849583cca219ae6ab986ee5/src/apis/toSvg.js#L6-L14

Updated PR being pushed now...