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.

Sign up for GitHub

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

Jump to bottom

fix(graphql): avoid partial field declarations from inherited metadata with cli plugin #3270

Merged
merged 2 commits into from
Oct 23, 2024
Merged

fix(graphql): avoid partial field declarations from inherited metadata with cli plugin #3270

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6516311
fix(graphql): avoid partial field declarations from inherited metadat…
CarsonF Jul 24, 2024
690dfeb
chore(graphql): skip redundant metadata merges for common parents
CarsonF Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions packages/graphql/lib/schema-builder/storages/type-metadata.storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,20 +216,30 @@ export class TypeMetadataStorageHost {
}

loadClassPluginMetadata(metadata: ClassMetadata[]) {
const loadedClasses = new Set<Function>();
metadata
.filter((item) => item?.target)
.forEach((item) => this.applyPluginMetadata(item.target.prototype));
.forEach((item) => this.applyPluginMetadata(item.target.prototype, loadedClasses));
}

applyPluginMetadata(prototype: Function) {
applyPluginMetadata(prototype: Function, loadedClasses = new Set<Function>()) {
do {
if (!prototype.constructor) {
return;
}
if (!prototype.constructor[METADATA_FACTORY_NAME]) {
// Skip redundant metadata merges for common parents.
if (loadedClasses.has(prototype.constructor)) {
return;
}
loadedClasses.add(prototype.constructor);

const metadata = Object.getOwnPropertyDescriptor(
Copy link
Member

Choose a reason for hiding this comment

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

question: any specific reason why we use getOwnPropertyDescriptor now?

Copy link
Contributor Author

@CarsonF CarsonF Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes this is basically the fix. We only want the metadata factory from the referenced class. Static properties (of ES6 classes) are inherited, and we want to avoid this here.
This could be an Object.hasOwn check. I used this to do that check and get the own value at the same time so the constant METADATA_FACTORY_NAME only had to be referenced once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I clarified the PR description. The minimal reproduction is still a lot of boilerplate.
I looked at updating tests, but I didn't find a spot where I could using the plugin metadata and instantiate a GraphQLModule using it.
What would you like me to do?

prototype.constructor,
METADATA_FACTORY_NAME,
)?.value?.();
if (!metadata) {
continue;
}
const metadata = prototype.constructor[METADATA_FACTORY_NAME]();
const properties = Object.keys(metadata);
properties.forEach((key) => {
if (metadata[key].type) {
Expand Down