Code review

[abjerner]

Based on commit: 6f4a1c49fb375cf7dfe265341e3c20105966758d

Formatting

Since we have the .editorconfig file in the VS solution, it might be a good idea to re-format the code. I think the shortcut is CTRL + K + CTRL + D even without ReSharper.

Naming

Generally the .NET convention is to use Pascal casing for type and member names - also for abbreviations, although Microsoft them selves doesn't always follow this. So a property like ThumbnailUrlSSL should ideally be named ThumbnailUrlSSL.

File name and file size are also separate words, so they should ideally be FileName and FileSize.

Remove setters

In most cases throughout the package, there isn't a need to set the properties from outside of the class the properties belong to, so it might be a good idea to remove the setter methods from those properties.

For instance, the SkyfishHttpService class currently has public SkyfishHttpClient Client { get; set; }, which could be changed to public SkyfishHttpClient Client { get; }.

Client vs Service

For our other integration packages, the purposes of the HTTP client class is to handle what I call the "raw" communication with the underlying API. Methods should therefore ideally return instances of IHttpResponse we can then deal with later in the HTTP service class.

The HTTP service class is then responsible for turning the IHttpResponses into object oriented models mapping the JSON model received from the API. Typically the two classes should have the same methods (if we ignore the return type).

I would prefer it this way as well here, although it's not something I'm going to mandate 😉

Separation of code

The package currently has some logic for getting the token and keeping it in memory. But this being an integration package, I believe it should fully support making the necessary calls to the Skyfish API, but then let it up to the user (eg. our Skybrud.VideoPicker.Skyfish package) to deal with how the token is stored/cached.

StringExtensions

The ByteToString extension method would be a good fit for moving to Skybrud.Essentials. You might want to rewrite it a bit to avoid string concatenation - see more here.

SkyfishHttpClient

Skybrud.Essentials (+.Http)

UNIX Timestamp

You can get the current UNIX timestamp from Skybrud.Essentials via (int) UnixTimeUtils.CurrentSeconds) - or perhaps even better in this case, use Skybrud.Essentials.Http's OAuthUtils.GetTimestamp method.

HMACSHA1

The HMACSHA1 class extends the HashAlgorithm class, so you should be able to replace parts of your hashing code with the SecurityUtils.GetHash method. We could even create an extension method that also initializes the HMACSHA1 so you can do the hashing with a single method call.

Notice that the methods in Skybrud.Essentials return the hex values in lower case.

HttpClient

Skybrud.Essentials.Http comes with a HttpClient class with is written for integration packages like this. So if you make SkyfishHttpClient extend HttpClient, you can change this:

private SkyfishVideo GetSkyfishVideoData(int videoId)
{
    var req = HttpRequest.Get($"https://api.colourbox.com/search?media_id={videoId}&return_values=unique_media_id+height+width+title+description+thumbnail_url+thumbnail_url_ssl+filename+file_disksize+file_mimetype");
    req.Headers.Authorization = $"CBX-SIMPLE-TOKEN Token={_token}";
    var result = req.GetResponse();

    return SkyfishVideo.Parse(JObject.Parse(result.Body));
}

private void CreateSkyfishStream(int videoId)
{
    var req = HttpRequest.Post($"https://api.colourbox.com/media/{videoId}/stream");
    req.Headers.Authorization = $"CBX-SIMPLE-TOKEN Token={_token}";
    req.GetResponse();
}

private IHttpResponse GetSkyfishVideo(int videoId)
{
    var req = HttpRequest.Get($"https://api.colourbox.com/media/{videoId}/metadata/stream_url");
    req.Headers.Authorization = $"CBX-SIMPLE-TOKEN Token={_token}";
    return req.GetResponse();
}

into this:

private SkyfishVideo GetSkyfishVideoData(int videoId) {
    var response = Get($"/search?media_id={videoId}&return_values=unique_media_id+height+width+title+description+thumbnail_url+thumbnail_url_ssl+filename+file_disksize+file_mimetype");
    return SkyfishVideo.Parse(JObject.Parse(response.Body));
}

private void CreateSkyfishStream(int videoId) {
    Post($"/media/{videoId}/stream");
}

private IHttpResponse GetSkyfishVideo(int videoId) {
    return Get($"/media/{videoId}/metadata/stream_url");
}

protected override void PrepareHttpRequest(IHttpRequest request) {

    // Append the scheme and domain of not already present
    if (request.Url.StartsWith("/")) request.Url = $"https://api.colourbox.com{request.Url}";

    // Set the "Authorization" header if the token is present
    if (!string.IsNullOrWhiteSpace(_token)) request.Authorization = $"CBX-SIMPLE-TOKEN Token={_token}";

}

The HttpClient class exposes methods for the different HTTP verbs - eg. the Get and Post methods. If you use these, the request are routed through the PrepareHttpRequest - which we can override and use to modify the request a bit before the request is sent to the destination.

In the example with Skyfish, we can append the correct scheme and domain to the URLs of the different requests, and we can add the Authorization here as well - both making the other methods a simpler.

JsonUtils.ParseJsonObject

Like we talked about on Slack, this isn't super necessary as Skyfish are using UNIX timestamp. But it might still be a good idea to use JsonUtils.ParseJsonObject over JObject.Parse - eg. replace this:

return SkyfishVideo.Parse(JObject.Parse(response.Body));

with this:

return JsonUtils.ParseJsonObject(response.Body, SkyfishVideo.Parse);

JsonUtils.ParseJsonObject disables Netwonsoft.Json's default date time handling (which we don't need here due to the UNIX timestamps), but IMO it also makes the code more readable.

SkyfishAuthResponse

To make the package easier to use, it might be a good idea to convert the ValidUntilUnix property from long to either DateTimeOffset or our own EssentialsTime.

For the parsing of the timestamp, you can replace ValidUntilUnix = long.Parse(obj.GetString("validUntil")) with ValidUntilUnix = obj.GetInt64("validUntil")) (Int64 is the CLR name of long).

or if we do change the type, change the line to:

ValidUntil = obj.GetInt64("validUntil", EssentialsTime.FromUnixTimestamp);

[abjerner]

Couldn't help adding the HMAC logic to Skybrud.Essentials, so now we can replace:

// Hmac hash needed for authing with Skyfish - https://api.skyfish.com/#sectionHead-21
int unixTimestamp = (int)(DateTime.UtcNow.Subtract(new DateTime(1970, 1, 1))).TotalSeconds;
string hmac = "";
var keyByte = Encoding.UTF8.GetBytes(SecretKey);
using (var hmacsha1 = new HMACSHA1(keyByte))
{
    hmacsha1.ComputeHash(Encoding.UTF8.GetBytes($"{ApiKey}:{unixTimestamp}"));

    hmac += StringExtensions.ByteToString(hmacsha1.Hash);
}

with this one liner:

// Hmac hash needed for authing with Skyfish - https://api.skyfish.com/#sectionHead-21
string hmac = SecurityUtils.GetHmacSha1Hash(SecretKey, $"{ApiKey}:{OAuthUtils.GetTimestamp()}");

😎

[abjerner]

Closing this issue a Jesper's follow up PR #2 is now merged...