Skip to content

Create EncryptedRedisStructStorage#4217

Merged
zachmargolis merged 7 commits intomitchellhenke/async-proofing-calls-lg-3330from
margolis-encrypted-redis-struct
Sep 17, 2020
Merged

Create EncryptedRedisStructStorage#4217
zachmargolis merged 7 commits intomitchellhenke/async-proofing-calls-lg-3330from
margolis-encrypted-redis-struct

Conversation

@zachmargolis
Copy link
Contributor

A mixin for storing structs in Redis that are encrypted and have an expiration

Re: https://github.com/18F/identity-idp/pull/4213/files#r490421107

This PR is just an extraction/refactor of the existing code and interface we wrote for DocumentCaptureSessionResult

I personally think .store might make for a good instance method instead of a class method (see below) but I didn't want to "rock the boat" that much

struct = MyStruct.new(id: '1', a: 'a', b: 'b')
struct.store

# vs today:
MyStruct.store(id: '1', a: 'a', b: 'b')

- A mixin for storing structs in Redis that are encrypted and have an expiration
Copy link
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

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

looks great! 👍🏼

@zachmargolis
Copy link
Contributor Author

zachmargolis commented Sep 17, 2020

Merging because it's just lint failures from the main branch that are failing

@zachmargolis zachmargolis merged this pull request into mitchellhenke/async-proofing-calls-lg-3330 Sep 17, 2020
@zachmargolis zachmargolis deleted the margolis-encrypted-redis-struct branch September 17, 2020 23:31
Copy link

@esparta esparta left a comment

Choose a reason for hiding this comment

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

The following suggestions are only useful iif EncryptedRedisStructStorage.load is a hot path, as in used very frequently in the system. If not, please disregard it.


def key(id, type:)
if type.respond_to?(:redis_key_prefix)
[type.redis_key_prefix, id].join(':')
Copy link

Choose a reason for hiding this comment

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

This method would create an array + string on every single call.

Suggested change
[type.redis_key_prefix, id].join(':')
type.redis_key_prefix + ":" id
# or
# "#{type.redis_key_prefix}:#{id}"

end

def key(id, type:)
if type.respond_to?(:redis_key_prefix)
Copy link

Choose a reason for hiding this comment

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

I would suggest to don't ask if the type respond to redis_key_prefix. IMO it would be programmatic error[1] by failing to follow the contract, and let it fall with something similar to this:

NoMethodError (undefined method `redis_key_prefix' for DocumentCaptureSessionResult:Class)

Is not as nice as the descriptive raise on line 53, but close enough to see where it's failing.

[1] Compared to user error.

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.

3 participants