Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Populating secret ownership via join rather than subquery #1169

Merged
merged 2 commits into from Apr 25, 2023

Conversation

mmontgomery-square
Copy link
Contributor

No description provided.

@mmontgomery-square mmontgomery-square requested a review from a team as a code owner December 10, 2022 17:25
@coveralls
Copy link

coveralls commented Dec 10, 2022

Coverage Status

Coverage: 75.104% (-0.007%) from 75.111% when pulling 1c53df3 on mmontgomery/jooq-oops into d865fa0 on master.

@mmontgomery-square
Copy link
Contributor Author

Update: I intentionally did not merge this because, while I believe it to be safe, it is also invasive, and I prioritized other more critical performance fixes over this one. It should still be merged eventually as I believe it to be a significant performance improvement.

Comment on lines -285 to 291
SelectQuery<Record> query = dslContext.select(SECRETS.fields())
.from(SECRETS)
.join(ACCESSGRANTS).on(SECRETS.ID.eq(ACCESSGRANTS.SECRETID))
.join(MEMBERSHIPS).on(ACCESSGRANTS.GROUPID.eq(MEMBERSHIPS.GROUPID))
.join(CLIENTS).on(CLIENTS.ID.eq(MEMBERSHIPS.CLIENTID))
.join(SECRETS_CONTENT).on(SECRETS_CONTENT.ID.eq(SECRETS.CURRENT))
.where(CLIENTS.NAME.eq(client.getName()).and(SECRETS.CURRENT.isNotNull()))
.getQuery();
SecretSeriesDAO secretSeriesDAO = secretSeriesDAOFactory.using(dslContext.configuration());

SelectQuery<Record> query = secretSeriesDAO.baseSelectQuery();
query.addJoin(ACCESSGRANTS, SECRETS.ID.eq(ACCESSGRANTS.SECRETID));
query.addJoin(MEMBERSHIPS, ACCESSGRANTS.GROUPID.eq(MEMBERSHIPS.GROUPID));
query.addJoin(CLIENTS, CLIENTS.ID.eq(MEMBERSHIPS.CLIENTID));
query.addJoin(SECRETS_CONTENT, SECRETS_CONTENT.ID.eq(SECRETS.CURRENT));
query.addConditions(CLIENTS.NAME.eq(client.getName()).and(SECRETS.CURRENT.isNotNull()));
query.addSelect(SECRETS_CONTENT.CONTENT_HMAC);
query.addSelect(SECRETS_CONTENT.CREATEDAT);
query.addSelect(SECRETS_CONTENT.CREATEDBY);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a violation of encapsulation, AclDAO replicates a bunch of queries for selecting various objects rather than leaning on the respective DAOs. What we've done here is move the base query into the secrets DAO but then allow AclDAO to customize it as needed.

Comment on lines 299 to 301
query.fetch()
.map(row -> processSanitizedSecretRow(row, client))
.map(row -> processSanitizedSecretRow(row, client, secretSeriesDAO))
.forEach(sanitizedSet::add);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we defer to the SecretSeriesDAO when it comes to mapping records to SecretSeries, we need to pass that class into the helper function.

@mmontgomery-square mmontgomery-square merged commit e74eb74 into master Apr 25, 2023
5 checks passed
@mmontgomery-square mmontgomery-square deleted the mmontgomery/jooq-oops branch April 25, 2023 18:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants