Adding bookkeeping to keep secret series and secret content expiry in… #1177
Conversation
| public int setExpiration(long secretContentId, Instant expiration) { | ||
| Field<Long> minExpiration = decode() | ||
| .when(SECRETS_CONTENT.EXPIRY.eq(0L), val(expiration.getEpochSecond())) | ||
| .otherwise(least(SECRETS_CONTENT.EXPIRY, val(expiration.getEpochSecond()))); |
There was a problem hiding this comment.
decode() appears to be a method which allows you to insert conditional logic as a value in a Jooq query. As a consequence of the edits below (reading the record first rather than doing a blind update), this has been replaced with plain Java.
| int secretsUpdated = dslContext.update(SECRETS) | ||
| .set(SECRETS.EXPIRY, updatedExpiry) | ||
| .where(SECRETS.CURRENT.eq(secretContentId)) | ||
| .execute(); |
There was a problem hiding this comment.
This is the block which keeps the series expiration and content expiration in sync. However, note that it only applies when the content being updated is the current version of a secret i.e. updating the expiration for non-current content has no effect on the series.
| public int setCurrentVersion(long secretId, long secretContentId, String updater, long now) { | ||
| long checkId; | ||
| Record1<Long> r = dslContext.select(SECRETS_CONTENT.SECRETID) | ||
| SecretsContentRecord r = dslContext | ||
| .select( | ||
| SECRETS_CONTENT.SECRETID, | ||
| SECRETS_CONTENT.EXPIRY) | ||
| .from(SECRETS_CONTENT) | ||
| .where(SECRETS_CONTENT.ID.eq(secretContentId)) | ||
| .fetchOne(); | ||
| .fetchOneInto(SECRETS_CONTENT); |
There was a problem hiding this comment.
Although it is possible to select arbitrary column values into a generic record, I think it's a lot more clear to always map the results into a properly typed record, even if not every field is populated.
|
|
||
| checkId = r.value1(); | ||
| long checkId = r.getSecretid(); | ||
| if (checkId != secretId) { |
There was a problem hiding this comment.
Expanding on the above, I think getSecretId() is much more clear than value1().
| return dslContext.update(SECRETS) | ||
| .set(SECRETS.CURRENT, secretContentId) | ||
| .set(SECRETS.EXPIRY, r.getExpiry()) | ||
| .set(SECRETS.UPDATEDBY, updater) | ||
| .set(SECRETS.UPDATEDAT, now) | ||
| .where(SECRETS.ID.eq(secretId)) | ||
| .execute(); | ||
| } |
There was a problem hiding this comment.
Because we're explicitly setting the current content version, we always update the series expiration to match.
| public Optional<SecretSeries> getSecretSeriesById(long id) { | ||
| SecretsRecord r = | ||
| dslContext.fetchOne(SECRETS, SECRETS.ID.eq(id).and(SECRETS.CURRENT.isNotNull())); | ||
| SecretsRecord r = getSecretSeriesRecordById(id); | ||
| return Optional.ofNullable(r).map(secretSeriesMapper::map); | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| SecretsRecord getSecretSeriesRecordById(long id) { | ||
| return dslContext.fetchOne(SECRETS, SECRETS.ID.eq(id).and(SECRETS.CURRENT.isNotNull())); | ||
| } |
There was a problem hiding this comment.
So far I've chosen not to expose the series-level expiration in the Java model (SecretSeries), since it's more of an internal optimization for the database. As a result, in order to examine the series-level expiration we need access to the database model's record object, which we previously didn't have.
| private static long randomExpiration() { | ||
| long expiration = 0; | ||
| Random random = new Random(); | ||
|
|
||
| for (;;) { | ||
| expiration = random.nextLong(); | ||
|
|
||
| if (expiration <= 0) { | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| Instant.ofEpochSecond(expiration); | ||
| } catch (DateTimeException e) { | ||
| continue; | ||
| } | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| return expiration; | ||
| } |
There was a problem hiding this comment.
Because the Keywhiz APIs bounce back and forth between long and Instant, we have to ensure that the expiration we generate is valid for both (this came up during a test where the random long value was outside the bounds of what's allowed for Instant seconds).
… sync