Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IMN-883 Add token-generation-readmodel-checker-verifier #1119

Draft
wants to merge 69 commits into
base: IMN-787_token-details-persister
Choose a base branch
from

Conversation

shuyec
Copy link
Contributor

@shuyec shuyec commented Oct 21, 2024

Closes IMN-583
Merge after #1094

Added the token-generation-readmodel-checker-verifier package to compare the token generation read model with the read model.

@shuyec shuyec changed the base branch from IMN-521_authorization-server to IMN-787_token-details-persister October 22, 2024 08:29
Comment on lines 25 to 28
// TODO: is exit code needed here?
if (differencesCount > 0) {
process.exit(1);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is process.exit(1) needed for the script?

Copy link
Contributor

Choose a reason for hiding this comment

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

on a second thought, despite the differences found, here the job has been completed successfully, so we can remove the process.exit(1) and let the job exit normally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Comment on lines +1037 to +1038
// TODO: are missing token entries considered errors or not?
if (!c || (c && (!a || b?.length === 0))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are missing token-generation-states entries considered errors or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, they are if the rows should exist.
For each client, we expect

  • if the client does not contain keys, the number of records in token-generation-states should be zero
  • if the client does not contain purposes, the number of records in token-generation-states should be equal to the number of keys in the client
  • if the client contains purposes, the number of records in token-generation-states should be equal to the number of keys in the client multiplied by the number of purposes

Comment on lines +1121 to +1127
// TODO: is this correct?
if (!tokenEntries || tokenEntries.length === 0) {
return {
status: true,
data: undefined,
};
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same question as this.

@shuyec shuyec marked this pull request as ready for review October 28, 2024 10:47
Copy link
Contributor

@galales galales left a comment

Choose a reason for hiding this comment

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

As rule of thumb keep things simple and leverage on the type system when you need to access a field on an existing type. It will make the code more readable and maintainable 😁

Comment on lines 25 to 28
// TODO: is exit code needed here?
if (differencesCount > 0) {
process.exit(1);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

on a second thought, despite the differences found, here the job has been completed successfully, so we can remove the process.exit(1) and let the job exit normally

await tokenGenerationService.readAllTokenGenerationStatesItems();
const tokenGenerationStatesClientPurposeEntries: TokenGenerationStatesClientPurposeEntry[] =
tokenGenerationStatesEntries
.map((e) => TokenGenerationStatesClientPurposeEntry.safeParse(e))
Copy link
Contributor

Choose a reason for hiding this comment

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

do we check also API clients?

Comment on lines 458 to 460
dataA: PlatformStatesPurposeEntry[],
dataB: TokenGenerationStatesClientPurposeEntry[],
dataC: Purpose[]
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to not use names like

Suggested change
dataA: PlatformStatesPurposeEntry[],
dataB: TokenGenerationStatesClientPurposeEntry[],
dataC: Purpose[]
platformStates: PlatformStatesPurposeEntry[],
tokenStates: TokenGenerationStatesClientPurposeEntry[],
purposes: Purpose[]

?

Comment on lines 186 to 194
function getIdentificationKey<T extends { PK: string } | { id: string }>(
obj: T
): string {
if ("PK" in obj) {
return unsafeBrandId(obj.PK.split("#")[1]);
} else {
return obj.id;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit over engineered, I suggest

Suggested change
function getIdentificationKey<T extends { PK: string } | { id: string }>(
obj: T
): string {
if ("PK" in obj) {
return unsafeBrandId(obj.PK.split("#")[1]);
} else {
return obj.id;
}
}
function getIdFromPrimaryKey(
record: PlatformStatesBaseEntry
): string {
return unsafeBrandId(record.PK.split("#")[1]);
}

and just access directly the field when you need the id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed function

Comment on lines +233 to +258
const parsedPurpose = PlatformStatesPurposeEntry.safeParse(e);
if (parsedPurpose.success) {
// eslint-disable-next-line functional/immutable-data
acc.platformPurposeEntries.push(parsedPurpose.data);
} else {
const parsedAgreement = PlatformStatesAgreementEntry.safeParse(e);
if (parsedAgreement.success) {
// eslint-disable-next-line functional/immutable-data
acc.platformAgreementEntries.push(parsedAgreement.data);
} else {
const parsedCatalog = PlatformStatesCatalogEntry.safeParse(e);
if (parsedCatalog.success) {
// eslint-disable-next-line functional/immutable-data
acc.platformCatalogEntries.push(parsedCatalog.data);
} else {
const parsedClient = PlatformStatesClientEntry.safeParse(e);
if (parsedClient.success) {
// eslint-disable-next-line functional/immutable-data
acc.platformClientEntries.push(parsedClient.data);
} else {
throw genericInternalError("Unknown platform-states type");
}
}
}
}
return acc;
Copy link
Contributor

Choose a reason for hiding this comment

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

same code, just generally more readable and without mutable data

Suggested change
const parsedPurpose = PlatformStatesPurposeEntry.safeParse(e);
if (parsedPurpose.success) {
// eslint-disable-next-line functional/immutable-data
acc.platformPurposeEntries.push(parsedPurpose.data);
} else {
const parsedAgreement = PlatformStatesAgreementEntry.safeParse(e);
if (parsedAgreement.success) {
// eslint-disable-next-line functional/immutable-data
acc.platformAgreementEntries.push(parsedAgreement.data);
} else {
const parsedCatalog = PlatformStatesCatalogEntry.safeParse(e);
if (parsedCatalog.success) {
// eslint-disable-next-line functional/immutable-data
acc.platformCatalogEntries.push(parsedCatalog.data);
} else {
const parsedClient = PlatformStatesClientEntry.safeParse(e);
if (parsedClient.success) {
// eslint-disable-next-line functional/immutable-data
acc.platformClientEntries.push(parsedClient.data);
} else {
throw genericInternalError("Unknown platform-states type");
}
}
}
}
return acc;
const parsedPurpose = PlatformStatesPurposeEntry.safeParse(e);
if (parsedPurpose.success) {
return {
...acc,
platformPurposeEntries: [...acc.platformPurposeEntries, parsedPurpose.data]
};
}
const parsedAgreement = PlatformStatesAgreementEntry.safeParse(e);
if (parsedAgreement.success) {
return {
...acc,
platformAgreementEntries: [...acc.platformAgreementEntries, parsedAgreement.data]
};
}
// Same for others
throw genericInternalError("Unknown platform-states type");

};
}

const foundEntries = tokenEntries.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

I may have misunderstood, but isn't this evaluating as error a valid record in token generation states that has a different purpose than the current one?

);
} else {
throw genericInternalError(
"Unexpected error while counting purpose differences"
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add more details to the error

@@ -34,6 +34,7 @@
"start:one-trust-notices": "turbo start --filter pagopa-interop-one-trust-notices",
"start:datalake-data-export": "turbo start --filter pagopa-interop-datalake-data-export",
"start:authorization-server": "turbo start --filter pagopa-interop-authorization-server",
"start:token-generation-readmodel-checker-verifier": "turbo start --filter pagopa-interop-token-generation-readmodel-checker-verifier",
Copy link
Contributor

Choose a reason for hiding this comment

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

may we rename the package to token-generation-readmodel-checker 🙏
(having both checker and verifier it's redundant)

Copy link
Contributor

Choose a reason for hiding this comment

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

note: comments on purpose logics apply also to the orthers

Comment on lines +1037 to +1038
// TODO: are missing token entries considered errors or not?
if (!c || (c && (!a || b?.length === 0))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, they are if the rows should exist.
For each client, we expect

  • if the client does not contain keys, the number of records in token-generation-states should be zero
  • if the client does not contain purposes, the number of records in token-generation-states should be equal to the number of keys in the client
  • if the client contains purposes, the number of records in token-generation-states should be equal to the number of keys in the client multiplied by the number of purposes

@shuyec shuyec marked this pull request as draft November 19, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants